Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Python 3 upgrade #3522

Merged
merged 26 commits into from
Jul 7, 2020
Merged

Python 3 upgrade #3522

merged 26 commits into from
Jul 7, 2020

Conversation

Code0x58
Copy link
Contributor

@Code0x58 Code0x58 commented May 3, 2020

This upgrades to Python 3.6+ A summary from the commit messages:

  • update heronpy release scripts for python3-only release
  • update Dockerfiles to only use python3 (includes fixes to unbuildable images)
  • fix pylint: upgrade to latest + clean up new lint issues were fixed along the way
  • fix transitive dependencies not being captured in PEXs: upgrade PEX tool to latest
  • (1) patch PEX imports of pex_library not working: use pex with --zip-safe=false
  • (2) patch python proto_library: rewrite relative imports
  • (3) update cloudpickle code
  • update vagrant environment to Ubuntu 20.04 and add ./vagrant/local-ci.sh which runs CI matching Travis. It gives a fair chunk of resources to the VM and is tested on Mac + Linux, and makes debugging easier. Mesos/Aurora was dropped just to make the upgrade easier, with the assumption someone can update that if it's still supported

Issues that follow on from this:

  1. Investigate and fix zip_safe=False in PEX #3558 using zip-safe=False was a workaround for imports not working. More investigation needed
  2. Improve/clean up genproto rules #3557 use long term solution for patch in tools/rules/genproto.bzl which is from python: use relative imports in generated modules protocolbuffers/protobuf#1491, possibly one of:
  3. Use cloudpickle via pypi rather than vendoring #3556 look at unvendoring cloudpickle and using a package from pypi
  4. pkg_tar requires a python in the environment, so something like python2 or python-is-python3 has to be installed in build environments (e.g. docker/compile/Dockerfile.*). pkg_tar rules have been deprecated in core bazel and made external

areas to check/known risks:

  • / acted as // when both parts were ints, but now it always converts to a float - some places caught this change in tests (i.e. Metrics)
  • ordering in dicts has changed since python 3.6 - it appears cases of this were caught in executor tests
  • bytes vs str - paths with test coverage had this fixed
  • test coverage - some components may not be tested in CI, for example ZK state managers or non-local schedulers

Things noticed along the way for future clean up issues:

  • Use builtin python3 modules instead of externals #3559 enum34, mock, and unittest2 can be replaced with python3 built in libraries - these can be listed with bazel query 'attr('reqs', "(unittest|enum34|mock)", kind("pex_library", //heron/...))' //heron/tools/admin/src/python:admin-py
  • buildifier could be added to the scripts+tests (it can be acquired via http_file(...)) - turns out it's not that easy/nice because select(...) can't be used at the top level WORKSPACE
  • travis is taking nearly 3 hours and isn't necessarily a single pipeline, so could be broken up into slightly overlapping but concurrent parts (e.g. ./scripts/travis/build.sh and ./scripts/travis/test.sh) to cut down wall clock build times from what is nearly 3 hours at the moment (with some fail-fast that, but that may not be an issue). This would be a great place to have a separate lint stage to expose all issues in one go
  • it looks like there are several definitions of things like builds/releases from all the different scripts in scripts/... - I suspect they could use some consolidation and cleaning up
  • DRY up Dockerfiles by using build stages #3560 docker/{compile,dist,base,test}/* look like good candidates for consolidation with multi-stage builds

tidbits and trivia:

  • overview of pex_library.reqs: bazel query 'attr('reqs', ".{3,}", kind("pex_library", //heron/...))' --output=build | grep reqs | cut -d \[ -f2 | sed 's/\],//' | sed 's/, /\n/g' | sort | uniq -c
  • unused_deps isn't safe to use on the codebase at the moment, but could be something to look into
  • upgrading pylint may have been a lot less painful if all of the linting was done in one go

@Code0x58 Code0x58 marked this pull request as ready for review May 3, 2020 02:11
@Code0x58 Code0x58 force-pushed the python-3-upgrade branch 2 times, most recently from 8b00d54 to 0cc0004 Compare May 3, 2020 02:28
@Code0x58 Code0x58 changed the title Python 3 upgrade WIP: Python 3 upgrade May 3, 2020
@Code0x58 Code0x58 force-pushed the python-3-upgrade branch 6 times, most recently from af74b98 to 097b861 Compare May 4, 2020 01:17
 * update heronpy release scripts for python3
 * update dist Dockerfiles to only use python3
 * remove python2 from docker images
 * upgrade pylint for python3 support
 * upgrade PEX so transative dependencies are captured

Additionally:
 * fix Ubuntu 16.04 images
 * fix linting issues found by newer pylint

There is an issue with encapsulation in the builds where the global python3 environment is used
while PEX installs a nested transitive dependency of pylint: `pylint>astroid>wrapt`. This seems
to be because of logic in its setup.py which can be disabled with `WRAPT_INSTALL_EXTENSIONS=false`
@Code0x58 Code0x58 force-pushed the python-3-upgrade branch 11 times, most recently from 4d76281 to 1ace938 Compare May 4, 2020 23:54
@Code0x58 Code0x58 force-pushed the python-3-upgrade branch 3 times, most recently from 175387d to a9ce0a5 Compare May 9, 2020 14:12
 * use kind to create ephemeral clusters
 * start consolidating scripts with python
@Code0x58
Copy link
Contributor Author

Thanks @joshfischer1108 for checking it out! I pushed 5efa344 to fix that, which at least seems to run without any toplogies (yet to try with). I'm making some basic bests for the helm chart with everything scaled down to one, and can try adding extra steps you do

@joshfischer1108
Copy link
Member

joshfischer1108 commented Jun 6, 2020

Thanks @Code0x58 . I'll do some more checking/testing this weekend.

@Code0x58
Copy link
Contributor Author

Code0x58 commented Jun 6, 2020

I just pushed a change that should get you at least a little further. I'm still working on getting the integration to run against k8s, so will hopefully have some time for that this weekend

@joshfischer1108
Copy link
Member

@Code0x58 I haven't forgotten about your PR. I'm trying to wrap up some work I had ahead of this so that we can start a new release candidate vote. I assumed it would have been done sooner, but I had forgotten about one last task. Once I have that fixed. I'll come back to this. Hope this is ok.

@huijunwu
Copy link
Member

huijunwu commented Jul 3, 2020

Followed the steps in #3522 (comment), the Tracker and UI look good to me.
Tried a local simple Python topology, and the job worked well.
My environment: Ubuntu 20.04, Python 3.8.2, with #3555

@Code0x58 Code0x58 changed the title WIP: Python 3 upgrade Python 3 upgrade Jul 4, 2020
@huijunwu
Copy link
Member

huijunwu commented Jul 5, 2020

Test with minikue. Looks good

$ hostnamectl
         Icon name: computer-desktop
           Chassis: desktop
  Operating System: Ubuntu 20.04 LTS
            Kernel: Linux 5.4.0-40-generic
      Architecture: x86-64
$ docker --version
Docker version 19.03.8, build afacb8b7f0

git clone https://github.com/apache/incubator-heron.git
git remote add Code0x58 https://github.com/Code0x58/incubator-heron.git 
git fetch Code0x58
git checkout --track Code0x58/python-3-upgrade

./docker/scripts/build-artifacts.sh ubuntu20.04 0.0.0 ~/heron-release

$ kubectl version --client
Client Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.5", GitCommit:"e6503f8d8f769ace2f338794c914a96fc335df0f", GitTreeState:"clean", BuildDate:"2020-06-26T03:47:41Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}
$ minikube version
minikube version: v1.11.0
commit: 57e2f55f47effe9ce396cea42a1e0eb4f611ebbd

eval $(minikube -p minikube docker-env)
./docker/scripts/build-docker.sh ubuntu20.04 0.0.0 ~/heron-release

# cluster
DIR=./deploy/kubernetes/minikube
sed 's!heron/heron:latest!heron/heron:0.0.0!g' ${DIR}/zookeeper.yaml > /tmp/zookeeper.yaml
sed 's!heron/heron:latest!heron/heron:0.0.0!g' ${DIR}/tools.yaml > /tmp/tools.yaml
sed 's!heron/heron:latest!heron/heron:0.0.0!g' ${DIR}/apiserver.yaml > /tmp/apiserver.yaml

kubectl create -f /tmp/zookeeper.yaml
kubectl get pods
kubectl create -f ${DIR}/bookkeeper.yaml
kubectl create -f /tmp/tools.yaml
kubectl create -f /tmp/apiserver.yaml

# submit a job
kubectl proxy -p 8001 &

curl http://localhost:8001/api/v1/namespaces/default/services/heron-apiserver:9000/proxy/api/v1/version
heron config kubernetes set service_url http://localhost:8001/api/v1/namespaces/default/services/heron-apiserver:9000/proxy
heron submit kubernetes ~/.heron/examples/heron-api-examples.jar org.apache.heron.examples.api.AckingTopology acking
heron kill kubernetes acking

# clean
kubectl delete -f /tmp/zookeeper.yaml
kubectl delete -f ${DIR}/bookkeeper.yaml
kubectl delete -f /tmp/tools.yaml
kubectl delete -f /tmp/apiserver.yaml

@joshfischer1108
Copy link
Member

joshfischer1108 commented Jul 5, 2020

I'm testing this branch on the darwin/debian10 platforms today.

@joshfischer1108
Copy link
Member

All good on Darwin and Debian10 platforms. 👍 @Code0x58



$ docker --version
Docker version 19.03.8, build afacb8b

git clone https://github.com/apache/incubator-heron.git
git remote add Code0x58 https://github.com/Code0x58/incubator-heron.git 
git fetch Code0x58
git checkout --track Code0x58/python-3-upgrade

# Darwin

$ sw_vers
ProductName:	Mac OS X
ProductVersion:	10.15.5
BuildVersion:	19F101

$ heron submit local  ~/.heron/examples/heron-api-examples.jar org.apache.heron.examples.api.AckingTopology acking

$ heron kill local acking

# Debian10
./docker/scripts/build-artifacts.sh debian10 0.0.0 ~/heron-release

$ docker run -it --rm  -p 8889:8889 heron/heron:0.0.0 bash

#All of the following commands were ran from inside the running container
$ supervisord

$ heron submit sandbox  ./examples/heron-api-examples.jar org.apache.heron.examples.api.AckingTopology acking

$ heron kill sandbox acking

@joshfischer1108
Copy link
Member

@nwangtw / @nlu90 could both of you test this out before we merge?

@nwangtw
Copy link
Contributor

nwangtw commented Jul 6, 2020

@nwangtw / @nlu90 could both of you test this out before we merge?

ack.

Copy link
Contributor

@nwangtw nwangtw left a comment

Choose a reason for hiding this comment

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

Tested it on ubuntu18 and seems to be working good:

  • build ok
  • install tools successfully
  • tracker/ui start ok
  • submit test topology successfully
  • topology shows ok in UI
  • kill test topology successfully.

@huijunwu
Copy link
Member

huijunwu commented Jul 7, 2020

I think we are ready to merge.

@nicknezis nicknezis merged commit 4f7f90f into apache:master Jul 7, 2020
@Code0x58 Code0x58 deleted the python-3-upgrade branch July 7, 2020 00:34
nicknezis added a commit that referenced this pull request Sep 14, 2020
* Initial Python 3 upgrade effort

* Fixes towards python3 support

 * update heronpy release scripts for python3
 * update dist Dockerfiles to only use python3
 * remove python2 from docker images
 * upgrade pylint for python3 support
 * upgrade PEX so transative dependencies are captured

Additionally:
 * fix Ubuntu 16.04 images
 * fix linting issues found by newer pylint

There is an issue with encapsulation in the builds where the global python3 environment is used
while PEX installs a nested transitive dependency of pylint: `pylint>astroid>wrapt`. This seems
to be because of logic in its setup.py which can be disabled with `WRAPT_INSTALL_EXTENSIONS=false`

* Fix new pylint issues

* update setuptools

* Make pex_pytest non-zip-safe

* Rough proto_library fix

The issue encountered was protocolbuffers/protobuf#1491 which
may be fixed by a pending PR to protoc, or with a switch to the official protobuf rules
and the import_prefix parameter to proto_library.

* WIP: Fix python3 incompatibilities

 * bytes vs str issues
 * update kazoo
 * order of processes in executor test changed due to dict ordering?
 * some places needed / switched to // - may be more not caught by tests
 * add travis_wait as some stages going over 10 minutes without output in CI

TODO:
 * make sure the kazoo upgrade is correct, it was done only by updating package versoin

* Try fixing build time issue in travis

* Upgrade docker rules

* Upgrade to python3 in CI

* Fix python integration tests

* Fix more bytes vs str errors + update vagrant

* Update Travis to Python3.7 + fix Vagrant on mac

* Reduce requirement to python3.6 + py3 fixes

 * use universal_newline in popen instead of text in Popen for py3.6
 * fix bytes/str issues in deserialisation
 * fix file open modes
 * use set instead of sets.Set
 * fix __import__(level) default

* Update cloudpickle

* Fix python addressing of release.yaml

* Additions to get docker image builds working and tested

 * use new external pkg_* rules
 * add python to compile docker images until pkg_*
 * add --host_force_python=PY3 to other bazel.rc files

* WIP: Add CI for docker images/releases

 * use kind to create ephemeral clusters
 * start consolidating scripts with python

* Fix helm chart

* bytes vs str fix

* Mention Python 3.6 requirement in README.md

* updatedockerfile

Co-authored-by: Neng Lu <[email protected]>
Co-authored-by: Nicholas Nezis <[email protected]>
Co-authored-by: bed debug <[email protected]>
Co-authored-by: huijunwu <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants