-
Notifications
You must be signed in to change notification settings - Fork 7k
[rllib, train] Add support for nested metrics in Result.get_best_checkpoint
#58537
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, train] Add support for nested metrics in Result.get_best_checkpoint
#58537
Conversation
…st_checkpoint` Signed-off-by: Mark Towers <[email protected]>
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.
Code Review
This pull request adds support for nested metrics in Result.get_best_checkpoint by using unflattened_lookup. The changes are correct and are accompanied by good tests covering nested metrics, different modes, and backward compatibility. I've suggested one improvement to the error message when an invalid metric is provided, to make it more helpful for users of nested metrics.
Signed-off-by: Mark Towers <[email protected]>
| import pyarrow | ||
|
|
||
| import ray | ||
| from ray._private.dict import unflattened_lookup |
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.
@justinvyu do we want to support this for Train V2 as well, or should we diverge for Tune?
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'm ok to support this in Train. This is only needed if users self-report nested dicts.
Result.get_best_checkpointResult.get_best_checkpoint
| import pyarrow | ||
|
|
||
| import ray | ||
| from ray._private.dict import unflattened_lookup |
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'm ok to support this in Train. This is only needed if users self-report nested dicts.
Co-authored-by: Justin Yu <[email protected]> Signed-off-by: Mark Towers <[email protected]>
Signed-off-by: Mark Towers <[email protected]>
justinvyu
left a comment
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!
Result.get_best_checkpointResult.get_best_checkpoint
…ckpoint` (ray-project#58537) RLlib uses nested metric structure (like `"{ENV_RUNNER_RESULTS}/{EPISODE_RETURN_MEAN}"`) which `Result.get_best_checkpoint` doesn't support. Following `ResultGrid.get_best_result()` to use `unflattened_lookup`, I've added that to `get_best_checkpoint` along with testing for nested structures (and its backward compatibility) --------- Signed-off-by: Mark Towers <[email protected]> Signed-off-by: Mark Towers <[email protected]> Co-authored-by: Mark Towers <[email protected]> Co-authored-by: Justin Yu <[email protected]>
…ckpoint` (ray-project#58537) RLlib uses nested metric structure (like `"{ENV_RUNNER_RESULTS}/{EPISODE_RETURN_MEAN}"`) which `Result.get_best_checkpoint` doesn't support. Following `ResultGrid.get_best_result()` to use `unflattened_lookup`, I've added that to `get_best_checkpoint` along with testing for nested structures (and its backward compatibility) --------- Signed-off-by: Mark Towers <[email protected]> Signed-off-by: Mark Towers <[email protected]> Co-authored-by: Mark Towers <[email protected]> Co-authored-by: Justin Yu <[email protected]> Signed-off-by: YK <[email protected]>
…ckpoint` (ray-project#58537) RLlib uses nested metric structure (like `"{ENV_RUNNER_RESULTS}/{EPISODE_RETURN_MEAN}"`) which `Result.get_best_checkpoint` doesn't support. Following `ResultGrid.get_best_result()` to use `unflattened_lookup`, I've added that to `get_best_checkpoint` along with testing for nested structures (and its backward compatibility) --------- Signed-off-by: Mark Towers <[email protected]> Signed-off-by: Mark Towers <[email protected]> Co-authored-by: Mark Towers <[email protected]> Co-authored-by: Justin Yu <[email protected]>
Description
RLlib uses nested metric structure (like
"{ENV_RUNNER_RESULTS}/{EPISODE_RETURN_MEAN}") whichResult.get_best_checkpointdoesn't support.Following
ResultGrid.get_best_result()to useunflattened_lookup, I've added that toget_best_checkpointalong with testing for nested structures (and its backward compatibility)Reproduction script
Related issues
#57533