Skip to content

Feature ray#27

Open
bradday4 wants to merge 11 commits intokoaning:mainfrom
bradday4:feature_ray
Open

Feature ray#27
bradday4 wants to merge 11 commits intokoaning:mainfrom
bradday4:feature_ray

Conversation

@bradday4
Copy link

@bradday4 bradday4 commented May 5, 2021

This PR address issue #26 .

Things to note.

  • I've updated the tests.yml to install ray in a local environment for testing. Also created a new pytest.ini file for ignoring deprecation warnings that ray throws. This is an issue that the maintainers of joblib will deal with so I don't see any reason to clutter pytest responses with a 100+ deprecation warnings.

  • Also made some minor modifications to the Runner docstring to reflect support for ray.
    I've performed the unit tests locally any everything checks out.

  • Code coverage has one caveat being the try - except block in Runner._run which catches the import error for ray. If we really wanted to run this bit of code we would need to alter tests.yml to run a specific test without ray installed, then install ray and run the normal test suite. I figured this was too much work for 5 lines of coverage.

  • other odds and ends.

Let me know if you need me to do anything else.

Comment on lines 63 to 69
if self.backend == "ray":
try:
from ray.util.joblib import register_ray
register_ray()
except ImportError:
NotInstalled("ray", "dev")

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we're not able to use ray.init() here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah we could do that. It would just require passing kwargs along to ray.init() through Runner.run then to Runner._run I thinks it's cleaner to let the user do ray.init() from their script / code. But, that brings ups a question though. Is the try except block even necessary if the user performs ray.init() in their code because they'll receive an import error well beforehand. The argument I then see for putting ray.init() here is to assist users with a customized error message that will give them a little bit more help.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial guess would be that the average user will have access to a laptop and not a compute cluster. Given that the main use-case will be merely a laptop it occurs to me as more plausible that the user doesn't want to be bothered with setting up a ray-cluster locally. Especially when using this package.

We could support both, something like ray and ray-cluster as backend strings but I'm wondering if that over-complicates thing. If we had to assume one I'd favor the ray.init() route. Mainly because I imagine that's how many folks will want to use it.

@bradday4 what are your thoughts?

Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly have a question on weather or not ray needs to be running as a service beforehand or if we can start ray just for our own run.

@bradday4
Copy link
Author

bradday4 commented May 6, 2021

Mainly have a question on weather or not ray needs to be running as a service beforehand or if we can start ray just for our own run.

Ray does not need to be running as a service. I.e called explicitly from the CLI before hand. We can use ray.init() in the code itself to start the server locally. I guess it would make things easier from a CI perspective since its one less build step during testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants