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 unit tests for updating episode data in base_env #17137

Merged
merged 4 commits into from
Sep 24, 2021

Conversation

mvindiola1
Copy link
Contributor

Why are these changes needed?

@sven1977, This is a follow up to issue #16683.

I had created a unit test for this issue but krfricke beat me to the PR so it did not make it. It is in this PR if you want it. It basically tests to make sure that the last_{obs,reward,done,info,action} are being set correctly for both single and multi-agent envs.

I also made one change to the base_env.py poll method. There is an extra clearing of self.last_info at the end of the method that I do not thin should be there. It is not there for any of the other last_x so I think it was put there incorrectly and should have been removed in yesterdays PR but was not.

Checks

  • [ x ] I've run scripts/format.sh to lint the changes in this PR.
  • [N/A] I've included any doc changes needed for https://docs.ray.io/en/master/.
  • [ x ] 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
    • [ x ] Unit tests
    • Release tests
    • This PR is not tested :(

@mvindiola1 mvindiola1 force-pushed the 16683_missing_info_dictionary branch from 7084832 to da423e2 Compare July 16, 2021 02:19
@@ -0,0 +1,143 @@
import ray
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! we just have to add this into rllib/BUILD to actually get this to run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@richardliaw It has been added to rllib/BUILD and is passing

@mvindiola1 mvindiola1 force-pushed the 16683_missing_info_dictionary branch from da423e2 to ea89d0e Compare July 19, 2021 12:53
@juliusfrost juliusfrost requested a review from sven1977 August 5, 2021 02:12
@juliusfrost
Copy link
Member

@mvindiola1 Could you resolve the merge conflict and formatting errors? Thanks!

@mvindiola1 mvindiola1 force-pushed the 16683_missing_info_dictionary branch from ea89d0e to da90238 Compare August 5, 2021 02:36
@mvindiola1
Copy link
Contributor Author

mvindiola1 commented Aug 5, 2021

@juliusfrost, I just rebased and pushed. I am not sure what to do about the formatting issues. When I run format.sh it complains about two formatting issues. If I adjust those lines to try and fix the complaint then the next time I run format.sh it changes them back and complains again. 🤷

@juliusfrost
Copy link
Member

@mvindiola1 Did you install the right linting dependencies? pip install -r python/requirements_linters.txt

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 @mvindiola1 for this test!

…3_missing_info_dictionary

# Conflicts:
#	rllib/BUILD
@sven1977
Copy link
Contributor

Just waiting for LINT tests to pass now ...

@sven1977 sven1977 merged commit 62f5da0 into ray-project:master Sep 24, 2021
@sven1977
Copy link
Contributor

@mvindiola1 ^
Sorry for the delay!

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