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

[RLlib] Always attach latest eval metrics. #21011

Merged
merged 7 commits into from
Dec 15, 2021

Conversation

gjoliver
Copy link
Member

Why are these changes needed?

Attach latest available eval metrics with every step result dict. So Tune doesn't get confused when most of the step results don't have eval metrics.
This allows us to bring back CQL tests.

Related issue number

Checks

  • 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 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 :(

@gjoliver
Copy link
Member Author

@sven1977 sven1977 changed the title Always attach latest eval metrics [RLlib] Always attach latest eval metrics. Dec 13, 2021
@sven1977 sven1977 self-assigned this Dec 13, 2021
Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for this PR @gjoliver!
So this was the reason CQL didn't "learn"? :)

"evaluation_config": {
"gamma": 0.98,
},
"always_attach_evaluation_results": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@gjoliver
Copy link
Member Author

Tests look pretty good now.
setting-up-botsrap-env was retried and has succeeded.
FlakyTests doesn't seem to have anything to do with this.

Can you please help merge? @sven1977

@gjoliver
Copy link
Member Author

Awesome, thanks for this PR @gjoliver! So this was the reason CQL didn't "learn"? :)

it's definitely part of the problems.
I am not sure how well it learns actually. but we get a working baseline now, at least :)

@sven1977 sven1977 merged commit 767f78e into ray-project:master Dec 15, 2021
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.

2 participants