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

[Feature Request] Drop the double gymnasium/gym dependency #240

Open
vmoens opened this issue Feb 9, 2023 · 11 comments
Open

[Feature Request] Drop the double gymnasium/gym dependency #240

vmoens opened this issue Feb 9, 2023 · 11 comments
Assignees

Comments

@vmoens
Copy link

vmoens commented Feb 9, 2023

Motivation

When installing the lib, both gym and gymnasium get installed.
This is a disaster for libs like torchrl which support both and need to choose a backend depending on the libraries that are installed in the virtual env.
For instance, running

if isinstance(spec, gym.spaces.discrete.Discrete):
   etc

will return False if gym does not point to the right library.

I think that one should avoid at all cost to have multiple versions of the same library installed at the same time in a venv. The renaming of gym in gymnasium is already a major pain point, but we should make everything we can to avoid adding an extra level of complexity.

@Trinkle23897
Copy link
Collaborator

@Markus28 @vwxyzjn what's the best practice here?

@Markus28
Copy link
Collaborator

@vmoens I'm not completely sure I correctly understand: In your example, would gym be either OpenAI Gym or Gymnasium, depending on what is installed?

I think it is quite realistic to assume that many users of Gymnasium will simultaneously have Gym installed, e.g. to use legacy environments (I'm not 100% sure whether this has been fixed but Atari required Gym, for instance). That's why I think it might be more sustainable to accommodate these cases.

In Tianshou we took the approach of only using Gymnasium environments internally. If a Gym environment is passed to tianshou, it is immediately wrapped in a compatibility layer. This might be a viable approach on your side.

On the envpool side, one possibility would be to make gym/gymnasium an extra that does not install the other dependency.

@vmoens
Copy link
Author

vmoens commented Feb 10, 2023

I think it is quite realistic to assume that many users of Gymnasium will simultaneously have Gym installed, e.g. to use legacy environments (I'm not 100% sure whether this has been fixed but Atari required Gym, for instance). That's why I think it might be more sustainable to accommodate these cases.

I tend to disagree. I think that renaming the library to gymnasium wasn't a bright idea exactly because of this. The fact that they follow the versioning of gym demonstrates that it is indeed the same library.

One thing is to support multiple versions of gym (which we do) another is to encourage the installation of multiple versions at the same time or worse, put them in the library requirements.

I have no problem against wrapping one with the other if that solves problems, but I would not put both in the library requirements.
Something like

try:
    import gym
except ImportError as err:
    GYM_ERR=err
else:
    GYM_ERR=None
try:
    import gymnasium
except ImportError as err:
    GYMNASIUM_ERR=err
else:
    GYMNASIUM_ERR=None

and then the rest of the code would follow as needed.

The contentious point on our side is that if we do a check like if isinstance(foo, gym.something) we will need to adapt the entire code base since one could import both libraries at the same time, hence this will become

if isinstance(foo, (gym.something, gymnasium.something, ))

This hampers code readability and, it boils down to supporting something that should never happen (IMHO).
Using the example above, we can do

try:
    from gymnasium import something
except:
    try:
        from gym import something
    except:
        etc

Does that make sense?

I can see multiple settings where having these two libs installed will cause headaches to the users and I would strongly recommend against it personally.

Also I'm curious about what could gymnasium not support that gym does? To me gymnasium follows the lifecycle of gym and any deprecation is done to improve the library. You can't use features from gym 0.23 in gymnasium for the same reason that you can't install both torch 1.10 and torch 1.11 in the same virtual env.

@araffin
Copy link
Contributor

araffin commented Feb 19, 2023

@vmoens you might have a look at https://github.com/DLR-RM/stable-baselines3/blob/feat/gymnasium-support/stable_baselines3/common/vec_env/patch_gym.py

With this piece of code, we support gym 0.21/0.26 and gymnasium (using Gymnasium/vec env as primary backend).

EDIT: i have some additionnal patches here: https://github.com/DLR-RM/rl-baselines3-zoo/blob/feat/gymnasium-support/rl_zoo3/gym_patches.py (for pybullet and timelimit)

@vmoens
Copy link
Author

vmoens commented Feb 20, 2023

Thanks
We support gym and gymnasium through this

My point isn't that this is a problem, but that a library should not be shipped with 2 versions of a same library (since gym and gymnasium are the same library).

@alex-petrenko
Copy link

@vmoens I hope there was an excellent reason for renaming the gym package to gymnasium, such as access rights to the old pypi package were lost or something like that.

Gym 0.26 upgrade was a huge pain already, and mostly unnecessary. The community already figured out ways to do everything the new API supports. But at least I can get behind the reasoning for it, the new API IS more transparent and clear.

This renaming does not add any value at all as far as I can tell, and adds a bunch of new complexity: basically, every single RL library needs to carry and maintain this compatibility layer, a new dependency on "shimmy", etc. As a community, we should do everything to prevent these things from happening in the future.

@vmoens
Copy link
Author

vmoens commented Feb 24, 2023

Indeed,
All my envs now have 2 libraries with the same capability. Funny enough: if I install gymnasium, it requires mujoco AND mujoco-py, which requires mujoco 2.1.0 and... gym!
so gymnasium requires gym!
We've gone full circle now... it's a pure engineering disaster
[EDIT] I was mislead by an error msg, mujoco-py does not require gym
Still I think that the double dependency harms code readability and is bug-prone.

@Markus28
Copy link
Collaborator

I very much understand your frustration. The hacks that are required to accommodate the recent changes make me feel uneasy, too. I don't know what the long-term perspective looks like, but I will bring this discussion up with the Gymnasium maintainers.

For envpool, I have looked into a solution where we provide one extra for gymnasium, one for gym, then enforce that the user specifies exactly one of these extras during installation. Unfortunately, I'm not really familiar with bazel or the intricacies of envpools build system in general, so if someone else could look into this, that would be great. I'd probably miss something (@Trinkle23897).

I still believe that the simultaneous installation of Gym and Gymnasium is something that will happen all the time (as @vmoens has pointed out, too). Of course, I agree that it's not something that should happen, but it will. It's a good idea to fix this double dependency for envpool, but ultimately, it will be out of your control what your users install, so (imo) you shouldn't make assumptions about that in your code.

@pseudo-rnd-thoughts
Copy link

pseudo-rnd-thoughts commented Mar 1, 2023

Hi all, Im only now looking at this thread and will try to respond to each problem individually

Indeed, All my envs now have 2 libraries with the same capability. Funny enough: if I install gymnasium, it requires mujoco AND mujoco-py, which requires mujoco 2.1.0 and... gym! so gymnasium requires gym! We've gone full circle now... it's a pure engineering disaster

I don't seem to be replicate this issue. Installing gymnasium[mujoco-py] or gymnasium[mujoco] doesn't include gym as any of the installs or sub-packages. Could you provide some code to replicate this issue?

For envpool, I have looked into a solution where we provide one extra for gymnasium, one for gym, then enforce that the user specifies exactly one of these extras during installation.

I agree with @Markus28 and with most people in this thread that this is the easiest solution to fix the issue. Or at least, have gymnasium as a core dependency (if the internal backend requires it as I understand) with gym as an optional extra.

@vmoens
Copy link
Author

vmoens commented Mar 1, 2023

Indeed, All my envs now have 2 libraries with the same capability. Funny enough: if I install gymnasium, it requires mujoco AND mujoco-py, which requires mujoco 2.1.0 and... gym! so gymnasium requires gym! We've gone full circle now... it's a pure engineering disaster

I don't seem to be replicate this issue. Installing gymnasium[mujoco-py] or gymnasium[mujoco] doesn't include gym as any of the installs or sub-packages. Could you provide some code to replicate this issue?

You are right I edited my comment :)

@jkterry1
Copy link

jkterry1 commented Mar 1, 2023

Just to offer a brief comment- the solution we're encouraging packages to adopt (and at least most packages I'm personally aware of are planning to adopt) is to only have a dependency on Gymnasium and to point users towards our compatibility wrappers for when they still need Gym.

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

No branches or pull requests

8 participants