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

[train/tune] Load the latest unflattened metrics with Result.from_path #40684

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

justinvyu
Copy link
Contributor

Why are these changes needed?

A trial's config gets saved as part of an iteration's reported metric. It gets saved by the default JsonLoggerCallback in each row of result.json as an unflattened dict.

The deserialization logic in Result.from_path reads this result.json file to reconstruct the config, but it flattens the dict along the way, result in keys like "config/a", leading to "config" not being found.

This PR fixes this by loading the metrics from json without doing extra operations on it.

cc: @daturkel

Limitations

progress.csv is another auto-generated file created by the default CSVLoggerCallback. There is no need for both of these files -- we may want to just only save the result.json in the future. One difference is that progress.csv does not contain the config as part of each row. In the case that a user disables the JSON logger callback, or deletes the result.json file, Result.from_path falls back to load info from this csv file, which doesn't include config information. result.config would return None in this case.

Related issue number

Closes #40535

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@daturkel
Copy link

Thank you for integrating this so quickly!

Copy link
Member

@woshiyyya woshiyyya left a comment

Choose a reason for hiding this comment

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

Thank you @justinvyu! LGTM

One small question is, the keys in latest_metrics and metrics_df will be mismatched. Can we update the docstring of class Result and provide an simple example?

@justinvyu justinvyu merged commit aef1c53 into ray-project:master Oct 27, 2023
27 of 30 checks passed
@justinvyu justinvyu deleted the flatened_result_config branch October 27, 2023 23:13
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.

[Air] Ray Result object not properly deserialized
4 participants