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] Remove unneeded args from offline learning examples #26666

Merged

Conversation

ArturNiederfahrenhorst
Copy link
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst commented Jul 18, 2022

Why are these changes needed?

We largely got rid of replay buffers in the context of offline RL in RLlib, but the examples have not beed changed.
This PR gets the offline learning example back on track.

#26665

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

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
…apex test

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
@ArturNiederfahrenhorst
Copy link
Contributor Author

I am actually not a fan of learning_starts -> min_size. min_size seems to imply the size of the RB, not how many samples there are in the buffer.

This comment is directed at another PR that moves the learning_starts parameters but needed to be merged into this one because they interfere. The comment has been resolved in the other PR.

Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
Signed-off-by: Artur Niederfahrenhorst <artur@anyscale.com>
@@ -262,7 +262,7 @@ def training(
if grad_clip is not None:
self.grad_clip = grad_clip
if optimization_config is not None:
self.optimization_config = optimization_config
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch man!!

Copy link
Contributor

@kouroshHakha kouroshHakha left a comment

Choose a reason for hiding this comment

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

Approved conditioned on all tests passing.

@sven1977
Copy link
Contributor

Hey @gjoliver , could you approve this, so it can be merged, now that questions have been addressed?

@ArturNiederfahrenhorst ArturNiederfahrenhorst dismissed gjoliver’s stale review August 17, 2022 13:44

This comment is directed at another PR that moves the learning_starts parameters but needed to be merged into this one because they interfere. The comment has been resolved in the other PR.

@sven1977 sven1977 merged commit f7b4c5a into ray-project:master Aug 17, 2022
@ArturNiederfahrenhorst ArturNiederfahrenhorst deleted the removebufferconfigs branch September 21, 2022 10:22
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.

4 participants