-
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] add more stoppers and stopper documentation #12750
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.
nice! thanks a bunch for doing this. Left a couple docs comments.
std (float): The minimal standard deviation after which | ||
the tuning process has to stop. |
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.
Instead, we should have something like an absolute threshold instead of std; std can have instabilities at small values anyways.
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.
But maybe it is out of scope :)
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.
I kept std as it was used in the ExperimentPlateauStopper
. If we change this we should change this for both. Let's chat about this really quick offline later.
|
||
Args: | ||
metric (str): Metric to check for convergence. | ||
std (float): Maximum metric standard deviation to decide if a |
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.
also similar comment here about std vs threshold
Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
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.
LGTM! Just left one small comment
python/ray/tune/stopper.py
Outdated
@@ -43,6 +46,27 @@ def stop_all(self): | |||
|
|||
|
|||
class CombinedStopper(Stopper): | |||
"""Combine several stoppers. |
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.
Is it worth mentioning that this combines Stoppers via OR and not AND, i.e stopping conditions are determined based on if any of the stopping conditions are met rather than all?
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.
Hmm, this is internal API now (not publicly listed on the documentation), so I'm guessing users will need to read source code anyways to read this. I'll add a small note though.
elif isinstance(stop, list): | ||
if any(not isinstance(s, Stopper) for s in stop): |
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.
be sure to document this later, and also note list
as a possible return-type in the error message at end of the switch-case block.
Why are these changes needed?
Stoppers are currently not documented. This PR adds stoppers to the documentation.
Also, we introduce two new stoppers, the maximum iteration stopper and the trial plateau stopper. The former is important for use in conjunction e.g. with the combined stopper. The latter is a utility used in
tune-sklearn
and might be of interest for other users.Tests were added as well.
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.