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

vec_envs fix seed() causing a reset #1486

Merged
merged 9 commits into from
May 20, 2023
Merged

Conversation

Kallinteris-Andreas
Copy link
Contributor

@Kallinteris-Andreas Kallinteris-Andreas commented May 5, 2023

Description

fixes issue :#1481
by keeping a self.seeds variable inside the env

  • removed compat code
  • added some tests to check for deterministic behavior
  • fixed the type hints for subprocess vec env
  • updated the env checker as seed parameter is now required
  • updated the tests accordingly

Motivation and Context

  • I have raised an issue to propose this change (required for new features and bug fixes)

closes #1481

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)

Checklist

  • I've read the CONTRIBUTION guide (required)
  • I have updated the changelog accordingly (required).
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.
  • I have opened an associated PR on the SB3-Contrib repository (if necessary)
  • I have opened an associated PR on the RL-Zoo3 repository (if necessary)
  • I have reformatted the code using make format (required)
  • I have checked the codestyle using make check-codestyle and make lint (required)
  • I have ensured make pytest and make type both pass. (required)
  • I have checked that the documentation builds using make doc (required)

Note: You can run most of the checks using make commit-checks.

Note: we are using a maximum length of 127 characters per line

@Kallinteris-Andreas Kallinteris-Andreas marked this pull request as draft May 5, 2023 18:22
@Kallinteris-Andreas
Copy link
Contributor Author

Note: I have not added new tests. (If you have a test suggestion i can put it in)

Neither updated the DOC. (as I am changing it to the already documented behavior)

@Kallinteris-Andreas
Copy link
Contributor Author

  1. thanks for the review, (I will fix them)
  2. @araffin is it possible to enable GitHub CI for my commits, I can not run pytest locally (some tensor board related issue)

@araffin
Copy link
Member

araffin commented May 8, 2023

is it possible to enable GitHub CI for my commits

I will for the next commits.

(some tensor board related issue)

what exactly?

You can still run mypy and ruff though, that should catch most issues.

PS: please use make commit-checks too (see PR template), reformating was missing too

@Kallinteris-Andreas
Copy link
Contributor Author

@araffin do you need help with debugging why pytest fails or do you understand why it fails

@araffin
Copy link
Member

araffin commented May 14, 2023

@araffin do you need help with debugging why pytest fails or do you understand why it fails

Thanks for the offer, but the TypeError was pretty clear.
Because of the changes, seed parameter at reset is now required for gym envs.
I updated the env checker and the tests too.

@araffin araffin marked this pull request as ready for review May 14, 2023 16:53
Copy link
Collaborator

@andregonz andregonz left a comment

Choose a reason for hiding this comment

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

Reviewed changes, seem to solve the problem. Tests correctly updated.

@araffin
Copy link
Member

araffin commented May 19, 2023

(Note: need to do a release on PyPi as soon as it is merged, otherwise it would break SB3 contrib)

return vec_env_cls([make_env(i + start_index) for i in range(n_envs)], **vec_env_kwargs)
vec_env = vec_env_cls([make_env(i + start_index) for i in range(n_envs)], **vec_env_kwargs)
# Prepare the seeds for the first reset
vec_env.seed(seed)
Copy link
Contributor

@ReHoss ReHoss Aug 6, 2023

Choose a reason for hiding this comment

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

Simple comment: in this context, the method vec_env.seed affects at the next reset a unique seed for each of the environments in the same manner as env.action_space.seed(seed + rank)

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.

[Bug]: DummyVecEnv.seed() resets the environment
4 participants