Skip to content

[tune] Remove rllib dep again, and add a test#1792

Merged
ericl merged 4 commits into
ray-project:masterfrom
ericl:rllib-dep
Mar 29, 2018
Merged

[tune] Remove rllib dep again, and add a test#1792
ericl merged 4 commits into
ray-project:masterfrom
ericl:rllib-dep

Conversation

@ericl

@ericl ericl commented Mar 27, 2018

Copy link
Copy Markdown
Contributor

What do these changes do?

Load rllib as late as possible and only if needed (for RLlib agents). Add a test to make sure this doesn't regress again.

cc @vlad17

@richardliaw

Copy link
Copy Markdown
Contributor

what was the current issue? don't the failed imports get caught in register_all?

@ericl

ericl commented Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

Oh, it looks like the try catch is gone there. Hm, we could put it back, but this is probably a cleaner fix.

@AmplabJenkins

Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/4526/
Test FAILed.

@AmplabJenkins

Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/4525/
Test PASSed.

@AmplabJenkins

Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/4528/
Test PASSed.

Comment thread python/ray/tune/trial.py Outdated
raise TuneError("Unknown trainable: " + trainable_name)
if not has_trainable(trainable_name):
# Make sure rllib agents are registered
from ray import rllib # noqa # pylint: disable=unused-import

@robertnishihara robertnishihara Mar 28, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit, the comment should just be # noqa: F401

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed

@@ -0,0 +1,26 @@
#!/usr/bin/env python

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is fine if this is the only thing that's ever going to be in this test file. However in general new tests should all use pytest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah this one kinda has to be its own file since it tests import state.

@AmplabJenkins

Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/4532/
Test PASSed.

@ericl ericl merged commit 4116c64 into ray-project:master Mar 29, 2018
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.

4 participants