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

Support Python 3 #8352

Closed
21 tasks done
EricCousineau-TRI opened this issue Mar 13, 2018 · 24 comments
Closed
21 tasks done

Support Python 3 #8352

EricCousineau-TRI opened this issue Mar 13, 2018 · 24 comments

Comments

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Mar 13, 2018

I believe we've discussed this before, but I'm not sure if it's been captured in a Drake-specific issue. Just writing this down for now, so I've marked it as priority: low. It can be upped if need be.

Options:

  • Support both Python3 and Python2.7 (for System + ROS)
  • Support only Python3

Pros:

  • Infix @ for matrix multiplication NumPy
  • Just being up-to-date (e.g. Homebrew pivots would hopefully hurt us less?)
  • [Any other specific libraries?]
  • If only supporting Python3:
    • Do not have to worry about mixing __*div__ and __*truediv__ (\cc @rdeits)

Cons:

  • If supporting 2+3:
    • Shebang inflexibility? (we just had to switch to /usr/bin/env python2 from /usr/bin/env python2.7 from /usr/bin/env python)
    • More compatibility shims
    • More CI configurations
  • If only supporting 3:
    • Loss of ROS support for Python? (@clalancette May I ask what you know what the plan is for distros beyond Lunar?)

Requires:

UPDATE (2018/10/06): I am starting to look into this to see what challenges there are.
Current plan of attack:

  • Resolve issues between Python2 and Python3 for pydrake and its testing code (pydrake: Enable testing to pass under Python3 #9613).
  • Resolve issues with infrastructure tooling:
    • pycodestyle has really weird behavior on how it handles getting new functions.
    • Some of our linting codes need more explicit guidance with encoding / decoding between raw bytestreams and UTF-8.
    • buildifer chokes on certain items.
  • Determine appropriate support matrix, e.g. Python2 on Xenial only, Python3 on Bionic only.
  • Finish PRs below

PRs in flight:

PRs to submit:

EDIT (2018/11/21): I believe our goal is to support Python3 around mid-December.
EDIT (2018/12/10): Targeting before the holidays (possibly past mid-December).

\cc @RussTedrake

@jwnimmer-tri
Copy link
Collaborator

I would be surprised if we could get away with a python3 flag day, so I suspect that any plan would involve supporting both for a while (at least in our published APIs), even if eventually we drop python2.

@stonier
Copy link
Contributor

stonier commented Mar 13, 2018

Recommend writing the above:

Support both Python3 and Python2.7 (for System/ROS)

The primary reason to keep support for python 2.7 is because that is Xenial's default. Anyone working on the assumption that the system default is a good baseline for development across many packages will want this (ROS just happens to be such a user).

This of course can change when we switch to Bionic beaver (python 3 only), but a consideration would be whether we support the tax of supporting both in the interim.

@stonier
Copy link
Contributor

stonier commented Mar 13, 2018

Loss of ROS support for Python? (@clalancette May I ask what you know what the plan is for distros beyond Lunar?)

ROS officially supports whatever the system default is and actively works on having their core libraries python3 ready (i.e. 2.7 and 3 compatible) as well as publically encouraging others to do so as well.

From REP 3:

Python

Our intent with Python is to track the minimum version provided in the supported Ubuntu platforms, as well as survey other commonly used OS platforms that support ROS to determine a reasonable minimum target.

Melodic

Ubuntu will most likely stop supporting Python 2 in release 20.04. To make sure ROS will be able to support that version of Ubuntu, ROS Python packages starting with Melodic Morenia are highly encouraged to support both Python 2.7 and Python 3.5 or later. During the development of Melodic there will be work undertaken to support both Python 2 and Python 3 (including rosdep keys) so ROS package developers can more easily test with either version of Python.

@jwnimmer-tri
Copy link
Collaborator

Good points. For Ubuntu, I think it would make sense to keep Drake's Xenial support at python2 only, and only support python3 as of Bionic. We'd still need the source tree to support both, but only certain runtime configurations would be tested with CI.

@jamiesnape
Copy link
Contributor

Given everything that happened with Homebrew and Python 2/3, this is going to be tricky (and not necessarily dependent on whether we support Python 3 for Mac).

@rdeits
Copy link
Contributor

rdeits commented Mar 16, 2018

My personal feeling is that supporting python3 is extremely important, given that python2 will be completely out of support in ~1.5 years. Drake and director are the last two pieces of software I ever use that don't support python3 yet.

Fortunately, supporting python2 and 3 simultaneously is much easier than it used to be (difficulty supporting both versions was one of the problems in early releases of python3). The six module includes some useful compatibility shims for functions like range() and dict.items() that have changed their behaviors.

@rdeits
Copy link
Contributor

rdeits commented Mar 16, 2018

Also, problems with homebrew and system python conflicts could be completely eliminated by using python's virtual environments (which are even built-in on python3.5 and above). I can't recommend that highly enough.

@EricCousineau-TRI
Copy link
Contributor Author

@kevinleestone mentioned that his team is interested in transitioning to Python3 once the surrounding ecosystem is ready, and the teams should strive for Python3 compatible code. He pointed towards Tensorflow as an excellent example of a Python2+3 compatible library, which has the following style guide considerations: https://www.tensorflow.org/community/style_guide#python_2_and_3_compatible

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Apr 15, 2018

Quickly spike-tested what it'd take to support both Python2+3. At the pybind, it's generally straightforward if we only support one version of Python per build.
I did a quick hack to python/repository.bzl to be flexible on Python version (which can be set with --python_executable=, via PATH, or explicitly via defaults.bzl), warning if it's an unsupported version. I got symbolic_test and autodiffutils_test (without experimental NumPy features) and a few other things to pass with minor changes.

Some items that would pave the way:

  • Ensure upstream C extension libraries offer lib/python{version} artifacts for python 2.7 (both) + 3.5 (Ubuntu 16.04) + 3.6 (Mac Homebrew), e.g. lcm, vtk, drake_visualizer
  • Include whatever python3 dependencies in install_prereqs.sh (six included)
  • Upgrade codebase using six where appropriate

My guess is it'd be a man week of PEH to fully get it working, and more on top of that to ensure we have flavors of CI covering Python3.

Left off here:
branch | commit

Still no timeline on this, as I still have it at a relatively low priority. I will see if I can periodically make PRs which slowly make compatibility changes.

@EricCousineau-TRI
Copy link
Contributor Author

An additional use case library-wise: multiprocessings forks and cv2s threads don't play well together:
opencv/opencv#5150 (comment)

One of the workarounds is to use the spawn method for multiprocessing, which is not available in Python2. Also, from looking at some related issues, it seems like Python3 fixes a few other issues in this library.

@pvarin
Copy link
Contributor

pvarin commented Sep 13, 2018

Just wanted to +1 for Python3 support. I'm starting to use some libraries that are Python3 only (specifically OpenAI baselines) which is making development pretty difficult.

@jamiesnape
Copy link
Contributor

Python 3 is next on our TODO list of low priority items after Ubuntu Bionic support, so it will certainly happen in the medium term.

@EricCousineau-TRI
Copy link
Contributor Author

I am starting to focus on this to see what the challenges may be; this still takes a backseat to Bionic support in general.

That being said, we may want to make our story straight with Bionic + Python2 support, e.g. say that Python2 is experimental, and once Python3 comes online, will be the only supported version.
@jwnimmer-tri @jamiesnape Can I ask y'all's thoughts on this? Would that be too abrupt of a change for users?

@RussTedrake
Copy link
Contributor

I think as far as the MIT class is concerned... just need the docker instance (locked on 16.04) to continue to work through the fall with Python 2.

@jwnimmer-tri
Copy link
Collaborator

I don't see any principled reason we'd need to ever officially support Bionic + Python2. If we want to disclaim it as "experimental" for now in order to be able to withdraw it later, I don't object.

As to whether going straight to Bionic + Python3 directly is the best plan, that would depend on the details, and I haven't looked enough to know. We'll need some flavor of Bionic + pydrake within the next two weeks. If that can be Python3 that's fine. If we need to have experimental Bionic + Python2 as a transition step, that's fine too.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 7, 2018

Awesome! I have a prototype branch where I just got bazel test //... working under Python3 (albeit with some hacks for running this on Xenial).
branch | commit

Need to then make sure I haven't broken Python2, and will then start filtering in the simpler compatibility PRs.

UPDATE: Working Python2 + Python3: 962d5fa

@jamiesnape Can I ask how we should start handling Python3 dependencies?
If you're OK with supporting just Python3 on Bionic, should we factor that into the current procedures for Bionic packages with Python stuff? (e.g. only provide Python3 versions?)

I think as far as the MIT class is concerned... just need the docker instance (locked on 16.04) to continue to work through the fall with Python 2.

Yup! This should keep everything working as-is for Xenial + Python2.

@EricCousineau-TRI EricCousineau-TRI changed the title Consider supporting Python 3 Support Python 3 Oct 8, 2018
@jamiesnape
Copy link
Contributor

FWIW Bionic support is marked as "experimental" as a whole (not that "experimental" really means a great deal).

@jamiesnape Can I ask how we should start handling Python3 dependencies?

Dependencies are easy enough. I will fix them once we are in a reasonable situation with the Python docs.

@jamiesnape
Copy link
Contributor

Also, what is the reasoning for the change to medium priority? This was not even the highest low priority item a couple of days ago. This requires a lot of work on CI and distribution infrastructure that isn't going to happen overnight.

@jamiesnape
Copy link
Contributor

I would also add, that not sure I see much benefit in pure build scripts running in Python 2 on one platform and Python 3 on another. Sure, pydrake should not change to break Xenial users' workflows (yet), but everything would seem like fair to just move across to Python 3.

And if you want to make Mojave Python 3 only, that would be fine. Then we only have to support multiple Pythons for at most a year from now on Mac.

@EricCousineau-TRI
Copy link
Contributor Author

Also, what is the reasoning for the change to medium priority? [...]

Given Jeremy's above point, I would like to see if we can move to Python3-only on upcoming platforms (Bionic, Mojave as well), hopefully without any sort of flag day (e.g. before going from experimental to supported), and save on support matrix combinatorics (e.g. trying to do Python2+3 on Xenial+Bionic+Mojave+etc.).

I apologize that I hadn't acted on this sooner to incorporate in our prior plans.

[...] This requires a lot of work on CI and distribution infrastructure that isn't going to happen overnight.

Understood that it wouldn't happen overnight; however, I am interested in simplifying our support, if possible. I would be more than up for helping move it forward if we think it's still feasible to do in conjunction with official Bionic support; however, if it is infeasible, I can certainly place the priority back down.

I would also add, that not sure I see much benefit in pure build scripts running in Python 2 on one platform and Python 3 on another. [...]

If Bazel provided a better mechanism to run both Python2 and Python3 scripts out of the box as native targets, I'd be up for doing build scripts in Python2, and then consumables in Python2 or Python3.
However, I'm a bit hesitant seeing what was necessary to run mkdoc.py (even if it were minimal).

If you think it'd be trivial otherwise, I'd be fine with it, but to me, it seems easiest just to support one Python version, period, on a platform (at least to start)?

@jamiesnape
Copy link
Contributor

If Bazel provided a better mechanism to run both Python2 and Python3 scripts out of the box as native targets, I'd be up for doing build scripts in Python2, and then consumables in Python2 or Python3.
However, I'm a bit hesitant seeing what was necessary to run mkdoc.py (even if it were minimal).

I am talking things that rely on shebang lines.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 16, 2018

EDIT: Moved checklist items to overview.

If these land in time, we can consider how to handle the support matrix for Bionic + Py2/Py3. All else, we support both on Bionic?

@EricCousineau-TRI
Copy link
Contributor Author

Jamie has added some experimental jobs:
https://drake-jenkins.csail.mit.edu/view/Python%203/

They are currently failing due to deprecation_test (at least in the one I saw), so I'll need to fix those before nightlies are enabled, so it'll be after the holidays when we get this landed. Getting close!

@EricCousineau-TRI
Copy link
Contributor Author

Closed by checkbox-ness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants