Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rllib] stopper support for python based training #29972

Merged
merged 6 commits into from
Nov 4, 2022
Merged

Conversation

maxpumperla
Copy link
Contributor

@maxpumperla maxpumperla commented Nov 3, 2022

rllib train python files are now cleaner, split out Tune-specific stop conditions. So, you can now write:

rllib train file cartpole_a3c.py -t python  --stop={'timesteps_total': 20000, 'episode_reward_mean': 150}"

I updated a couple of example scripts / tuned examples to show-case this (and added a test). I think at this point running Python files feels like the best way to go (as opposed to YAML etc.).

Bonus: assuming we can pull off the conversion of all current yaml files to python, I think this would open up a really nice way of exposing these algo configs as proper examples in our docs. This section still bothers me.

@@ -102,10 +102,12 @@ def run(example_id: str = typer.Argument(..., help="Example ID to run.")):
example_file = get_example_file(example_id)
example_file, temp_file = download_example_file(example_file)
file_type = example.get("file_type", SupportedFileType.yaml)
stop = example.get("stop", "{}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, sorry, unrelated to this PR, BUT can we automate the file_type recognition (instead of making it yaml by default, detect type from file extension).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the PR.
Please address the questions and nits before merging. Thx @maxpumperla !

Signed-off-by: Max Pumperla <[email protected]>
Signed-off-by: Max Pumperla <[email protected]>
Signed-off-by: Max Pumperla <[email protected]>
@gjoliver gjoliver merged commit 0ec1c3a into master Nov 4, 2022
@gjoliver gjoliver deleted the mp_stop_cli branch November 4, 2022 19:41
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
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.

3 participants