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

Install Python 2.7 and 3.6 on Centos6 base image through Pyenv #7064

Merged

Conversation

Projects
None yet
6 participants
@cosmicexplorer
Copy link
Contributor

commented Jan 12, 2019

Problem

We have used a python 3 interpreter in CI for a while now, using pyenv to install this in our Dockerfiles instead of relying on CentOS's scl features, which don't allow for multiple pythons at a time. Using pyenv to install Python 2.7 as well instead of relying on the package manager allows us to upgrade or modify the image without screwing up our Python installation.

That work began in this PR and ended up in #6981, #7352, #7418, #7483, and more, but it then led to having to duplicate code across travis_ci and travis_ci_py36 Dockerfiles. Using pyenv to install python in our travis_ci images also involves a significant setup time in bootstrap phases to install our desired pythons. Installing Python in the CentOS 6 base image avoids having to install Python at all in our CI, which saves us minutes in the bootstrap phase.

Solution

  • Use Pyenv to install Python 2.7 and Python 3.6 in the centos6 Dockerfile.
  • Remove travis_cy_py36.
  • Modify the travis_ci Dockerfile to bootstrap pyenv itself if not available.

Result

We get some CI time back in the bootstrap phase.

TODO

  • merge this PR
  • publish the new centos6 image to dockerhub at pantsbuild/centos6:latest
  • merge a followup PR removing the manual pyenv bootstrapping from the travis_ci image.
@Eric-Arellano
Copy link
Contributor

left a comment

Thank you Danny for cleaning this up! Looks great.

We’ll need to figure out how to publish this to Docker once it’s merged.

Show resolved Hide resolved build-support/docker/centos6/Dockerfile
@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

As the CI run was shown to work on Travis in 05e7b67, I've moved the pyenv changes from travis_ci/Dockerfile to centos6/Dockerfile so that the CentOS6 base image has a python 2 and a python 3 install ready to go. This will need a docker release for pantsbuild/centos6:latest once merged.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

I pushed e641c3e first with changes made to both centos6 and travis_ci dockerfiles to get an idea of whether this change would work once we push the centos6 container, then pushed the final commit for the code which will actually be checked in. I made #7072 to consider caching docker builds in CI avoid this conundrum by allowing us to build both images in travis without any extra CI time.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

Was having trouble with Docker on my local machine not having enough space even after I deleted all of the images locally. Will look at this with fresh eyes tomorrow.

@illicitonion
Copy link
Contributor

left a comment

Looks reasonable, but I don't know enough about pyenv to be useful - leaving to other folks who know what they're talking about :) Thanks!

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jan 14, 2019

Remove changes to centos6/Dockerfile and copy travis_ci/Dockerfile
Realized we don't need to be blocked by pantsbuild#7064, although it should certainly be addressed soon after by a followup PR.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:python3-centos6-dockerfile branch 3 times, most recently from 12c12db to 1be1a25 Jan 15, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

pyenv-installer doesn't actually modify ~/.bashrc -- see #7064 (comment). Also, docker silently drops any changes made to a directory after it is marked as a VOLUME, which would have been fantastic if it had been mentioned in any level of bold text. Both of these fun surprises were added as comments.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

With #6981 squash-merged on top of the current commit, we get this travis log for building the py3 linux pex (from this travis run on my fork), which shows it failing at importing sqlite, which is expected until the changes to centos6/Dockerfile are pushed, and I believe demonstrates that a valid python 3 interpreter is indeed available.

EDIT: After talking to @Eric-Arellano, that appears not to be the case. Will make another squash merge temporarily adding the sqlite-devel package in the travis_ci dockerfile to verify it works with the pants3-in-ci branch.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

There are failures in the branch I've pushed my to own fork which squash merges pants3-in-ci, one for py2 and py3, which I'll get to tomorrow.

Eric-Arellano added a commit that referenced this pull request Jan 16, 2019

Use ./pants3 for majority of CI (#6981)
### Problem
Right now our CI exclusively uses Python 2 under-the-hood. It tries to test Python 3 support by setting`--setup-python-interpreter-constraints` to Python 3, but this only constrains subprocesses like tests and is not the same thing as fully supporting Python 3. 

We want our CI to test using both Python 2 under-the-hood and Python 3 under-the-hood, because for the next two releases are promising users that either interpreter is fully supported. 

Daily CI should run primarily with Python 3, and the nightly cron job should check for Python 2 regressions.

### Solution
Use the new `./pants3` script added in #6959.

#### Installing Python 3 on each shard
* OSX: git clone pyenv to consistently install 3.6.8. Necessary because some older images come with an outdated version of pyenv. We don't use Travis's `language` feature because several shards are supposed to be `language: general`.
* Linux build engine, temporarily modify `travis_ci/Dockerfile` to download pyenv and install Python 3. This change is being pulled out the `centos6` image in #7064 to avoid rebuilding Py3 every CI run, but it must be published to Docker hub so will be addressed in a followup PR.
* Normal Linux shards, use Travis's `language: python` feature

#### Add integration test blacklist
23 integration test targets are failing when ran with Py3 under-the-hood, so we restore support for an integration test blacklist. This allows us to track Py3 fixes we make and to collaborate more easily.

Further, the contrib tests, linter, and osx rust shards when ran with Py3. So, we run these 3 shards and the blacklist with a Py2 pex but Py3 subprocesses, which is what we currently do in master.

#### Expand Cron job
We add non-integration test jobs to the nightly cron run to check for any regressions in shards like `Linux build wheels`, now that we run daily with a Py3 PEX and need to check that they still work with a Py2 PEX. 

#### Default to Python 3 in CI
Defaulting to Py3 reflects that we are transitioning towards exclusive Py3 use.

This is not user-facing, and only impacts the `.travis.yml` code, so this change is safe to make, whereas we want to wait to rename `./pants3` -> `./pants` until it is a bit more robust.

### FYI - new .travis.yml idiom
This required a major rewrite of `.travis.yml` to avoid duplication. The new idiom we use when defining a shard is to have a `base_my_new_shard` which defines the minimum amount of config necessary for that job, and then to have a `py2_my_new_shard` and `py3_my_new_shard` that extend that base along with their corresponding OS + python version config.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:python3-centos6-dockerfile branch 3 times, most recently from 776cb9b to 13f7b4c Jan 21, 2019

cosmicexplorer added some commits Mar 10, 2019

add openssl and other required libs to the travis_ci dockerfile
add openssl and other required libs to the travis_ci dockerfile

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:python3-centos6-dockerfile branch from afea5f2 to a06bfbe Apr 4, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

I have pushed changes which address the recent review comments and also allow the travis_ci Dockerfile to work with or without the centos6 dockerhub push, which breaks the dependency cycle of pushing the centos6 image vs merging this PR. The plan:

  1. Merge this PR.
  2. Push the updated centos6 dockerfile to dockerhub
  3. Followup PR removing the section marked BEGIN CENTOS6 DOCKERFILE CHANGES in the travis_ci dockerfile
@Eric-Arellano
Copy link
Contributor

left a comment

Good idea with the plan to make this robust to when we publish to Dockerhub!

Show resolved Hide resolved build-support/docker/centos6/Dockerfile Outdated
Show resolved Hide resolved build-support/docker/centos6/Dockerfile Outdated
Show resolved Hide resolved build-support/docker/centos6/Dockerfile Outdated
Show resolved Hide resolved build-support/docker/centos6/Dockerfile Outdated

@cosmicexplorer cosmicexplorer changed the title install python 3.6.8 on the centos6 base image for ./pants3 install pyenv in the centos6 base image Apr 4, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Re-reviewed code changes and they all look good.

Some suggestions for the PR title and PR description:

  1. Rename to "Install Python 2.7 and 3.6 on Centos6 base image through Pyenv". Pyenv is more of an implementation detail than the important takeaway.
  2. The cache from #7470 does not apply to the docker images. For the Docker images, we rebuild Python 3 every time.
  3. In solution section, change to Use Pyenv to install Python 2.7 and Python 3.6
  4. Maybe worth mentioning why we use Pyenv to install Python 2.7 rather than the apt installed Python. I think the main motivation is symmetry.

@cosmicexplorer cosmicexplorer changed the title install pyenv in the centos6 base image Install Python 2.7 and 3.6 on Centos6 base image through Pyenv Apr 4, 2019

@cosmicexplorer cosmicexplorer merged commit 20c497c into pantsbuild:master Apr 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -21,10 +21,12 @@ RUN yum install sqlite-devel -y
ENV PYENV_ROOT "${PYENV_ROOT:-/pyenv-docker-build}"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 5, 2019

Contributor

@cosmicexplorer can you please remove lines 19 and 20: RUN yum install sqlite-devel -y. We don't need them anymore.

cosmicexplorer added a commit that referenced this pull request Apr 5, 2019

remove pyenv bootstrapping from travis_ci/Dockerfile (#7505)
### Problem

The conclusion of #7064 was, after `pantsbuild/centos6:latest` was updated, to remove the pyenv bootstrapping in `travis_ci/Dockerfile` as it was now being done in the centos6 base image.

### Solution

- Remove the marked section.

### Result

This will speed up all Docker shards that had Py3 installed by 2-3 minutes. So, Linux build engine Py36 and Linux build wheels abi3.

This means overall CI is about 5 minutes faster, and the bootstrap stage 2-3 minutes faster, which is especially significant since that blocks starting other tests.

Finally, this means everywhere we run Pants in CI, we have Python 3 installed now (instead of having to install it again!).

@Eric-Arellano Eric-Arellano added this to the 1.15.x milestone Apr 6, 2019

stuhood added a commit that referenced this pull request Apr 8, 2019

Prepare Linux UCS2 shard for upgrade to Centos6 base image in #7064 (#…
…7418)

### Problem
We will in #7064 start using Pyenv to install Python 2.7 in the base Centos6 image. 

This will conflict with our Linux UCS2 shard, which currently uses Pyenv to install Python 2.7 with UCS2 instead of the default UCS4.

### Solution
Tweak the Dockerfile to avoid any ambiguity with Centos6 image:

* Rename the Python version env var to not conflict by marking it as `UCS2`.
* Check if `PYENV_ROOT` is already defined.
* Check if `PYENV_ROOT` already exists before git cloning to avoid a git failure.

The key line is `${PYENV_BIN} global ${PYTHON_27_VERSION_UCS2}`, which will override whatever `${PYENV_ROOT}/shims/python2.7` used to point to.

stuhood added a commit that referenced this pull request Apr 8, 2019

Install Python 2.7 and 3.6 on Centos6 base image through Pyenv (#7064)
### Problem

We have used a python 3 interpreter in CI for a while now, using `pyenv` to install this in our Dockerfiles instead of relying on CentOS's `scl` features, which don't allow for multiple pythons at a time. Using `pyenv` to install Python 2.7 as well instead of relying on the package manager allows us to upgrade or modify the image without screwing up our Python installation.

That work began in this PR and ended up in #6981, #7352, #7418, #7483, and more, but it then led to having to duplicate code across `travis_ci` and `travis_ci_py36` Dockerfiles. Using `pyenv` to install python in our `travis_ci` images also involves a significant setup time in bootstrap phases to install our desired pythons. Installing Python in the CentOS 6 base image avoids having to install Python at all in our CI, which saves us minutes in the bootstrap phase.

### Solution

- Use Pyenv to install Python 2.7 and Python 3.6 in the `centos6` Dockerfile.
- Remove `travis_cy_py36`.
- Modify the `travis_ci` Dockerfile to bootstrap pyenv itself if not available.

### Result

We get some CI time back in the bootstrap phase.

### TODO

- [ ] merge this PR
- [ ] publish the new `centos6` image to dockerhub at `pantsbuild/centos6:latest`
- [ ] merge a followup PR removing the manual pyenv bootstrapping from the `travis_ci` image.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.