[tune] Ray Tune API cleanup #1454
Conversation
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
| @@ -30,6 +31,7 @@ Getting Started | |||
|
|
|||
| register_trainable("my_func", my_func) | |||
|
|
|||
| ray.init() | |||
| run_experiments({ | |||
There was a problem hiding this comment.
I believe you need to modify rllib.rst too
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
ericl
changed the title
ericl
changed the title
ericl
changed the title
|
Sorry for merging the other PR here, thought that was best since Travis isn't very happy right now. |
python/ray/tune/hyperband.py
Outdated
| @@ -295,7 +297,9 @@ def current_trials(self): | |||
|
|
|||
| def continue_trial(self, trial): | |||
| result = self._live_trials[trial] | |||
| if self._get_result_time(result) < self._cumul_r: | |||
| if self._get_result_time(result) > self._max_t: | |||
There was a problem hiding this comment.
@richardliaw is this the right way to limit to max t? If not we can leave it for another pr.
There was a problem hiding this comment.
it's a little bit more involved than this..
There was a problem hiding this comment.
Ok, let me revert this then.
|
Test FAILed. |
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
|
Build finished. Test FAILed. |
|
Test FAILed. |
|
Build finished. Test PASSed. |
|
Test PASSed. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
python/ray/tune/tune.py
Outdated
| @@ -30,6 +30,9 @@ def _make_scheduler(args): | |||
|
|
|||
|
|
|||
| def run_experiments(experiments, scheduler=None): | |||
| # Make sure rllib agents are registered | |||
| from ray import rllib # noqa # pylint: disable=unused-import | |||
There was a problem hiding this comment.
Does this make sense? We should probably only import if flagged.
One problem here is that the teaser example in http://ray.readthedocs.io/en/latest/tune.html calls run_experiments, and nowhere do we list the rllib dependencies, so the user will probably get a bunch of installation errors in trying to run the simplest example.
There was a problem hiding this comment.
Importing rllib actually does not have any hard dependencies -- if some dependency is not installed you just get a warning on the console.
There was a problem hiding this comment.
OK, I see. I'm trying to import run_experiments on a clean env.
I'm getting an import error telling me that I still need to install TensorFlow (for the Logger) - this should be documented somewhere. Could also just disable the TensorBoard logger if Tensorflow not installed. This can be done on a separate PR (or in the current one)
Also, if a user is trying to use tune out of the box (say for HyperBand), it probably makes sense to preface the warnings with something like Trying to import default rllib agents... instead of a sudden block of warnings.
There was a problem hiding this comment.
That's fine for now, we can improve this in a separate PR.
|
|
|
Merged build finished. Test FAILed. |
|
Test FAILed. |
|
Merged build finished. Test PASSed. |
|
Test PASSed. |
|
Builds look good. |
What do these changes do?
Remove rllib dep: trainable is now a standalone abstract class that can be easily subclassed.
Clean up hyperband: fix debug string and add an example.
Remove YAML api / ScriptRunner: this was never really used.
Move ray.init() out of run_experiments(): This provides greater flexibility and should be less confusing since there isn't an implicit init() done there. Note that this is a breaking API change for tune.
Related issue number
#1429