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] check if self.env is not None explicitly #15634

Merged
merged 5 commits into from
Jun 21, 2021

Conversation

benjamindkilleen
Copy link
Contributor

@benjamindkilleen benjamindkilleen commented May 4, 2021

Why are these changes needed?

The rollout worker stop() method uses if self.env to evaluate whether self.env is None. This can cause errors if the user's env class implements __len__(). I discovered this because I use the same class as a Gym env and as a PyTorch dataset for pretraining, which requires implementing __len__().

Related issue number

Checks

Since I changed only one line, I did not run the unit tests locally, sorry :(

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

@benjamindkilleen
Copy link
Contributor Author

I committed the changes made by the linter, but I don't think I was supposed to. That's why the linter checks are failing, I believe.

Signed-off-by: Richard Liaw <rliaw@berkeley.edu>
Signed-off-by: Richard Liaw <rliaw@berkeley.edu>
Signed-off-by: Richard Liaw <rliaw@berkeley.edu>
@richardliaw richardliaw changed the title check if self.env is not None explicitly [rllib] check if self.env is not None explicitly Jun 21, 2021
@richardliaw richardliaw merged commit 50049f8 into ray-project:master Jun 21, 2021
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

3 participants