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] Fixed 'rollout_fragment_length' in pong-example by setting it to 'auto'. #39552

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

simonsays1980
Copy link
Collaborator

@simonsays1980 simonsays1980 commented Sep 11, 2023

Why are these changes needed?

Pong example did not run due to a rollout_fragment_length that did not fit the train_batch_size. By setting the rollout_fragment_length to auto the rollout_fragment_length adapts to the train_batch_size.

Related issue number

Closes #38968

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: Simon Zehnder <simon.zehnder@gmail.com>
Copy link
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the fix

Signed-off-by: Artur Niederfahrenhorst <attaismyname@googlemail.com>
rllib/BUILD Outdated Show resolved Hide resolved
@simonsays1980 simonsays1980 force-pushed the pong-benchmark-value-error branch 2 times, most recently from 8663b28 to 0a9b440 Compare September 12, 2023 23:06
Signed-off-by: Simon Zehnder <simon.zehnder@gmail.com>
Signed-off-by: Simon Zehnder <simon.zehnder@gmail.com>
@simonsays1980
Copy link
Collaborator Author

simonsays1980 commented Sep 13, 2023

@ArturNiederfahrenhorst Looking at the failed tests I think that the test is running on the wrong cluster: it requests a GPU (in the YAML) but there is none.

Shall we add "gpu" to the tags of the Pong test?

rllib/BUILD Outdated Show resolved Hide resolved
@sven1977 sven1977 changed the title Fixed 'rollout_fragment_length' in pong-example by setting it to 'auto'. [RLlib] Fixed 'rollout_fragment_length' in pong-example by setting it to 'auto'. Sep 13, 2023
Signed-off-by: Simon Zehnder <simon.zehnder@gmail.com>
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.

LGTM. Thanks! :)

@sven1977 sven1977 merged commit 8d80377 into ray-project:master Sep 13, 2023
37 of 41 checks passed
@ArturNiederfahrenhorst
Copy link
Contributor

ArturNiederfahrenhorst commented Sep 13, 2023

@sven1977 I was the one who added the test. The reason we stumbled across this issue was that we don't test this code today - the issue was raised by a user who read the docs. In fact, all of our fine-tuned examples listed under doc/source/rllib/rllib-algorithms.rst are not tested.
Imho, they should be release tests given that RLlib's most used Algorithm is PPO by a good margin and that we refer to these as examples of it in our documentation.

@ArturNiederfahrenhorst
Copy link
Contributor

I agree that the non-gpu learning test is not a good place, but instead, there should be another place where we execute this test. After all, if we can't prove learning on it, it should not be referred to as a fine-tuned example

@ArturNiederfahrenhorst
Copy link
Contributor

I've opened an issue #39639

simonsays1980 added a commit to simonsays1980/ray that referenced this pull request Sep 15, 2023
… to 'auto'. (ray-project#39552)

Signed-off-by: Simon Zehnder <simon.zehnder@gmail.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
… to 'auto'. (ray-project#39552)

Signed-off-by: Victor <vctr.y.m@example.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.

RLlib: Pong benchmark example throws a ValueError
3 participants