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

[Proposal] Explicitly requires to allow closing a closed environment #531

Closed
1 task done
qgallouedec opened this issue Jun 1, 2023 · 2 comments · Fixed by #564
Closed
1 task done

[Proposal] Explicitly requires to allow closing a closed environment #531

qgallouedec opened this issue Jun 1, 2023 · 2 comments · Fixed by #564
Labels
enhancement New feature or request

Comments

@qgallouedec
Copy link
Contributor

qgallouedec commented Jun 1, 2023

Proposal

  • Add a warning in the env checker when the custom environment that doesn't allow to be closed multiple times.
  • Mention that it is recommended to be able to call the close function on an already closed environment.

Motivation

It's usually possible to reset an environment several times.

env = gym.make("CartPole-v1")
env.close()
env.close()

Is it good practice to do so? Since there's no simple way of checking whether an environment is closed, I propose to consider that calling the close method on a closed environment should not raise an error.

Pitch

Give your opinion, I'll handle the PR if you're ok.

Alternatives

Add an is_closed method, and always raise an error when the user tries to close a closed environment.

(But it would break so many things...)

Additional context

See DLR-RM/stable-baselines3#1525, and especially DLR-RM/stable-baselines3#1525 (comment)

Checklist

  • I have checked that there is no similar issue in the repo
@qgallouedec qgallouedec added the enhancement New feature or request label Jun 1, 2023
@qgallouedec qgallouedec changed the title [Proposal] Proposal title [Proposal] Explicitly requires to allow closing a closed environment Jun 1, 2023
@pseudo-rnd-thoughts
Copy link
Member

Yes, I think this is a good idea and yes on the PR, thanks for offering
Would we prevent the second close in the PassiveEnvCheckerWrapper from being forwarded to the base environment?

@qgallouedec
Copy link
Contributor Author

Would we prevent the second close in the PassiveEnvCheckerWrapper` from being forwarded to the base environment?

I would say no. From a user's perspective, I expect the PassiveEnvChecker to check the environment passively. Preventing a second close can't be considered "passive" anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants