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] Fixes MARWIL release tests #26586

Merged

Conversation

ArturNiederfahrenhorst
Copy link
Contributor

Why are these changes needed?

MARWIL release tests are broken because OPE is enabled by default, requiring action distributions to be present in our offline data, which they are not for the previously collected SAC file.

Related issue number

#26579

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

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.

thanks for the fix @ArturNiederfahrenhorst .

@avnishn
Copy link
Member

avnishn commented Jul 15, 2022

so this pr fixes learning tests f-z

and #26581 potentially fixes learning tests a-e, or a-e is flaky and requires more debugging. Nonetheless this pr fixes a deterministic bug in f-z, we should merge ASAP

@@ -25,4 +25,5 @@ marwil-halfcheetahbulletenv-v0:
evaluation_interval: 3
evaluation_config:
input: sampler
off_policy_estimation_methods:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put something like null instead of leaving it blank?

Copy link
Member

Choose a reason for hiding this comment

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

Its my understanding that we have to leave it blank in order for it to map to None unfortunately with the way that our YAML is processed.

@richardliaw richardliaw merged commit 4ce9686 into ray-project:master Jul 15, 2022
xwjiang2010 pushed a commit to xwjiang2010/ray that referenced this pull request Jul 19, 2022
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
@ArturNiederfahrenhorst ArturNiederfahrenhorst deleted the marwilreleasetestfix branch September 21, 2022 10:21
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.

None yet

4 participants