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] Add lr_schedule support to SimpleQ and PG. #28381

Merged

Conversation

mgerstgrasser
Copy link
Contributor

@mgerstgrasser mgerstgrasser commented Sep 8, 2022

Why are these changes needed?

SimpleQ and PG currently don't support a learning rate schedule. SimpleQ does list a lr_schedule key in its default config, but will simply ignore it, which could lead to user confusion. PG does not accept a lr_schedule key, but there is no reason why it shouldn't support a learning rate schedule.

Related issue number

Closes #28266

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

@ArturNiederfahrenhorst
Copy link
Contributor

Thanks for this PR! I'm adding some testing and minor stuff and then we can have a review and merge :)

Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
@mgerstgrasser
Copy link
Contributor Author

Thanks for this PR! I'm adding some testing and minor stuff and then we can have a review and merge :)

Oh thank you! Anything I can do to help?

@ArturNiederfahrenhorst
Copy link
Contributor

Not for now, thanks! @sven1977 will have to review next.

@sven1977 sven1977 changed the title Add lr_schedule support to SimpleQ and PG [RLlib] Add lr_schedule support to SimpleQ and PG. Sep 14, 2022
@sven1977
Copy link
Contributor

Hey @mgerstgrasser , thanks for the PR! This looks great.
One test case seems to be failing: rllib/tests/test_nested_action_spaces.py

I'm getting this error here:

image

Signed-off-by: mgerstgrasser <[email protected]>
@mgerstgrasser
Copy link
Contributor Author

Hey @mgerstgrasser , thanks for the PR! This looks great. One test case seems to be failing: rllib/tests/test_nested_action_spaces.py

I'm getting this error here:

image

@sven1977 Should be fixed now, thanks for flagging this. Issue was in the test - it's passing a PG config into BC. I'm just removing the lr_schedule from the config now in that test.

Signed-off-by: mgerstgrasser <[email protected]>
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.

[RLlib] SimpleQ ignores lr_schedule
3 participants