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

Drop python2 support #1819

Merged
merged 11 commits into from Feb 25, 2020
Merged

Drop python2 support #1819

merged 11 commits into from Feb 25, 2020

Conversation

pzhokhov
Copy link
Collaborator

No description provided.

Copy link
Contributor

@christopherhesse christopherhesse left a comment

Choose a reason for hiding this comment

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

LGTM, does this mean six can be dropped as well?

I noticed you didn't add 3.8 to the classifiers: 'Programming Language :: Python :: 3.7', (not sure that does anything)

@christopherhesse
Copy link
Contributor

Also I'm a little worried that the 3.8 tests are segfaulting, can just the mujoco ones be disabled?

@zuoxingdong
Copy link
Contributor

Should we remove importing that involves from __future__ import *?

@pzhokhov
Copy link
Collaborator Author

@christopherhesse

I noticed you didn't add 3.8 to the classifiers: 'Programming Language :: Python :: 3.7', (not sure that does anything)

  • that's because the tests for 3.8 are segfaulting :) I think it only adds a category in pypi, nothing else. I'll disable mujoco and add 3.8 tests

@pzhokhov
Copy link
Collaborator Author

@christopherhesse I think removing six should be its own, bigger PR, because it is used in a bunch of places. Created an issue (#1823) for that.

@pzhokhov pzhokhov merged commit 639264c into master Feb 25, 2020
@pzhokhov pzhokhov deleted the drop_python2 branch February 25, 2020 13:12
@pzhokhov
Copy link
Collaborator Author

pzhokhov commented Feb 25, 2020

@zuoxingdong

Should we remove importing that involves from future import *?

yeah, but let's do that in a separate PR (maybe the same one that removes six - I added removal of these imports to #1823 issue as well)

@@ -45,11 +45,10 @@
tests_require=['pytest', 'mock'],
python_requires='>=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*, !=3.4.*',
Copy link
Contributor

@tvalentyn tvalentyn Feb 26, 2020

Choose a reason for hiding this comment

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

I think you'd want to update python_requires as well, otherwise as soon as you add Python code that is not Py2-compatible, you will break Python 2 users who don't set an upper bound on your dependency (gym>=0.10), or expect compatibility w.r.t. semantic versioning (gym>=0.15, <1.0 for example) - note that this is a minor version change as per VERSION = '0.17.0'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @cclauss , I've seen your drive-by comments on quite a few projects re: Python 3 migration. FYI - this is a common error when dropping Py2. Another project I saw this: hamcrest/PyHamcrest#131. Perhaps this is something to keep an eye on in the projects that you come across.

Copy link
Contributor

@cclauss cclauss Feb 26, 2020

Choose a reason for hiding this comment

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

I would use the python_requires=">=3.5" approach when dropping legacy Python.

Like #1827 has done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, fully agree; I meant that more projects should be following this pattern, and I thought perhaps you are in the position to raise awareness of the proper way to drop Py2 support in other projects.

zlig pushed a commit to zlig/gym that referenced this pull request Apr 24, 2020
* drop python 2 support

* remove python 2.7 logic from Dockerfile

* Revert "remove python 2.7 logic from Dockerfile"

This reverts commit 43ceabf.

* remove python 2.7 logic from Dockerfile

* use python 3.7.3 (3.7.6 segfaults with mujoco for some reason?

* include cmake into system level packages

* add swig to system packages

* python3.8 build still segfaults on mujoco tests, disabling for now

* add python 3.8 and 3.9 to the build

* python 3.9.1 -> 3.9.0

* oops i did not realize python 3.9 is still in alpha
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

5 participants