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

[air/output] Fix excluded keys in results and config #36764

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

krfricke
Copy link
Contributor

Why are these changes needed?

We are currently applying the exclude keys on the flattened results, but this will not work if the excluded keys are dictionaries, such as the config value in the results dict. Instead, we should operate on the original results dict and only flatten from the second level onwards.

Related issue number

Closes #36756

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: Kai Fricke <kai@anyscale.com>
str
Signed-off-by: Kai Fricke <kai@anyscale.com>
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

One question: now it's only possible to exclude a full top-level key. Is the exclude key configurable by users or is it just used internally? I remember the trial output table can be configured to only show a certain set of columns, but I think that's a separate path.

@krfricke
Copy link
Contributor Author

That's a good point - looks like this won't work for nested metrics (e.g. this will break rllib). I'll update the PR early next week

@krfricke krfricke added the do-not-merge Do not merge this PR! label Jun 23, 2023
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke
Copy link
Contributor Author

@justinvyu updated, PTAL

@krfricke krfricke requested a review from justinvyu June 26, 2023 15:02
@krfricke krfricke removed the do-not-merge Do not merge this PR! label Jun 26, 2023
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

Thanks!

A few other points:

  1. Can we test the include behavior as well?
  2. The only usage of include is here:
    include=self._metrics,

@krfricke
Copy link
Contributor Author

Thanks!

A few other points:

1. Can we test the `include` behavior as well?

2. The only usage of `include` is here: https://github.com/ray-project/ray/blob/9f258506bbcd58763b032b416a90cd00152bfe33/python/ray/tune/experimental/output.py#L1017


* The metrics field is populated by: https://github.com/ray-project/ray/blob/9f258506bbcd58763b032b416a90cd00152bfe33/python/ray/tune/tune.py#L958C55-L958C55

* This gets a `_progress_metrics` attribute from the `Trainable`, but I don't think this thing is ever set anywhere. Can we remove the usage of `include` here? We can keep the include functionality for the future (ex: make it customizable as a user config).

It's actually used in rllib, e.g. here:

https://github.com/ray-project/ray/blob/master/rllib/algorithms/algorithm.py#L211-L218

Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke merged commit bb19073 into ray-project:master Jun 27, 2023
@krfricke krfricke deleted the air/output/exclude branch June 27, 2023 20:04
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
We are currently applying the `exclude` keys on the flattened results, but this will not work if the excluded keys are dictionaries, such as the `config` value in the results dict. Instead, we should operate on the original results dict and only flatten from the second level onwards.

Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
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 Output] Repeated configuration in training result for each epoch
2 participants