-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[tune] Allow to set buffer_length via tune.run #15810
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
One thing I'm wondering is if we should really include this in the top level tune.run()
API, or if this should be an option for the RayTrialExecutor only (which can be passed to tune.run()
). The reason is that we try to prevent the tune.run()
parameter list from growing too much, and technical parameters such as the result buffer length (especially when there's another way to set them, e.g. via environment variables) are prime candidates to exclude from the high level API.
Would you be happy with that as well?
Also cc @richardliaw would like to get your opinion here
python/ray/tune/tune.py
Outdated
@@ -175,6 +176,8 @@ def run( | |||
``ray.tune.Stopper``, which allows users to implement | |||
custom experiment-wide stopping (i.e., stopping an entire Tune | |||
run based on some time constraint). | |||
result_buffer_length (int): Maximum number of results to buffer. | |||
If set, env var 'TUNE_RESULT_BUFFER_LENGTH' will be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If set, env var 'TUNE_RESULT_BUFFER_LENGTH' will be ignored. | |
If set, env var ``TUNE_RESULT_BUFFER_LENGTH`` will be ignored. |
I think it's okay to only include this in RayTrialExecutor. In my case, I just want to be able to infer and set the value of buffer_length from other parameters such as validation period. |
That sounds great. Would you be able to update the PR? I'm happy to approve afterwards and merge once CI passes |
a750003
to
de58dbe
Compare
updated and rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
See discussion here.
Checks
scripts/format.sh
to lint the changes in this PR.