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

Add support for python 3.6 #2836

Merged
merged 16 commits into from
May 25, 2022
Merged

Conversation

pseudo-rnd-thoughts
Copy link
Contributor

Previously python 3.6 was removed as a supported gym python version, this was primarily as the python foundation no longer supports python 3.6 with security or bug fixes.
This PR readds python 3.6 as a supported version through downgrading importlib_metadata that is used in the registration.py for importing third-party environments.
Looking at the release notes, there appears no good reason for why it was pinned to <=4.10 in eb6d826.
Therefore, we have reduced the strictness to the minimum version that python 3.6 can download, importlib_metadata>=4.8

@RedTachyon
Copy link
Contributor

I mean even if we can support 3.6 now with this simple change, should we? We need to be sure that any dependencies support it too (and if a new release e.g. of pygame with amazing features comes out, but doesn't support 3.6, we need to stick with an old one)

Also, this forbids us from using any 3.7 features. At a glance, this will already break some of the type annotation stuff (PEP 563 which we use in some files), we block ourselves from using dataclasses (or forces us to use weird workarounds; worth noting we already use dataclasses in registration), we can't "safely" use the property that dictionaries are ordered (it's only official since 3.7)

@@ -14,6 +14,7 @@ ENV LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/root/.mujoco/mujoco210/bin
COPY . /usr/local/gym/
WORKDIR /usr/local/gym/

RUN pip install .[noatari] && pip install -r test_requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to specify that it has to be 3.6.15? I'm not sure what the reason for this is exactly, but I'm guessing this should apply to all 3.6.x versions

Copy link
Contributor Author

@pseudo-rnd-thoughts pseudo-rnd-thoughts May 25, 2022

Choose a reason for hiding this comment

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

This is fine because the python foundation is not releasing any more python versions and this dockerfile is only called by the CI which uses the latest version of each python, i.e. only python 3.6.15 will be used

@jkterry1 jkterry1 merged commit 0263deb into openai:master May 25, 2022
@pzhokhov
Copy link
Collaborator

pzhokhov commented May 27, 2022

@pseudo-rnd-thoughts
I am with @RedTachyon on this one -- while we could support python 3.6 (and likely even earlier versions, like 3.5), I'd rather not do it without sufficiently good rationale, for all the reasons that @RedTachyon described, plus generally increased maintenance / development overhead, e.g. anyone developing or modifying a new feature needs to keep in mind that code needs to be python 3.6 compatible. What are the reasons for keeping the python 3.6 support?

@pseudo-rnd-thoughts
Copy link
Contributor Author

Hi @pzhokhov, thanks for asking.
One of the major users of gym requested that we support Python 3.6 and we would prefer them to utilise the latest versions of gym that includes several important new features than not. Therefore, we are trying to support python 3.6 as a minimal product, i.e. the core API is tested however mujoco is not.
If you join the discord and message me, I can give you more details. I realise that this PR does not explain why we made this choice as the discussions happened privately, hope that helps.

@jkterry1
Copy link
Collaborator

@pzhokhov we also warn users to not use 3.6 on import if they're using it, and like Mark said only a very small amount of the library will actually run with Python 3.6

@vwxyzjn
Copy link
Contributor

vwxyzjn commented Jun 4, 2022

Hey, @pseudo-rnd-thoughts there is one issue though with python 3.6 support. I am working on the poetry integration as discussed and note that ale-py at minimum supports 3.7 - one of the things to be found out during dependencies resolution :)

Same thing with mujoco https://pypi.org/project/mujoco/#files

image

This also means if the current user installs gym with python 3.6, it will work but will break if they try to install ale-py.

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.

5 participants