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

Propagate global interpreter constraints when building PEXes with interpreter constraints requested #7285

Merged

Conversation

Projects
5 participants
@Eric-Arellano
Copy link
Contributor

commented Feb 26, 2019

Problem

When running ./pants binary, we do not consider the global interpreter constraints in passing them to the Pex builder.

This resulted in a bug where the interpreter used to build a PEX in CI differed from the Python used to run the PEX. See pantsbuild/pex#676. To resolve this issue, we need some way to specify the Pex builder must use the specific Python version 2.7.13 (UCS4). This blocks
#7235.

Solution

Modify PexBuilderWrapper to use compatibility_or_constrains(). This will first try to get the compatibility constraints from the target itself, and then resort to using the PythonSetup subsystem's constraints (for the global instance, resolved from pants.ini, PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS, and --interpreter-constraints).

Result

Users of the function add_interpreter_constraints_from() will now add the global --interpreter-constraints to the Pex builder if the targets do not themselves specify a constraint. At the moment, this only impacts ./pants binary, as python_binary_create.py is the only file which uses this function.

Note this is still not a great solution. It would be better to kill add_interpreter_constraint() and add_interpreter_constraints_from() to instead automatically set the interpreter constraints from the targets' graph. This PR does not make that change to avoid scope creep.

Skipped tests

Due to pantsbuild/pex#655, we must now skip several tests that now fail. PEX does not correctly OR interpreter constraints, so these tests complain that they cannot find an appropriate interpreter to satisfy ['CPython>=3.6,<4', 'CPython>=2.7,<3'].

Because this PR blocks #7235, which blocks #7197, we skip these tests until the upstream fix pantsbuild/pex#678 is merged into PEX and Pants upgrades its PEX to the new version #7186.

Eric-Arellano added some commits Feb 23, 2019

Constrain ci.sh to the exact Python interpreter version
Earlier we allowed the patch version to float. We discovered in #7235 with the CI run https://travis-ci.org/pantsbuild/pants/jobs/497208431#L891 that PEX was building with 2.7.10 but running with 2.7.13.

The fix will require having Pants pass interpreter constraints to Pex. Even with that change though, the CI shard would still have the issue because the constraint would be floating.

Now, we have the constraint be exact.

@Eric-Arellano Eric-Arellano added this to In progress in Adding Python 3 support Feb 26, 2019

@jsirois
Copy link
Member

left a comment

We have a mess here. The add_interpreter_constraints_from method should probably be using

def compatibility_or_constraints(self, target):
"""
Return either the compatibility of the given target, or the interpreter constraints.
If interpreter constraints are supplied by the CLI flag, return those only.
"""
if self.get_options().is_flagged('interpreter_constraints'):
return tuple(self.interpreter_constraints)
return tuple(target.compatibility or self.interpreter_constraints)

Bandaids are piled on bandaids it seems.

Generify solution by using compatibility_or_constraints()
Instead of applying a bandaid for only `./pants binary`, John proposed fixing the issue with our PexBuilderWrapper itself. So, we use `compatibility_or_constrains()`, which will first try to return the target's compatibility, else will return the Python Setup subystem's value.

The wrapper still is not ideal and John proposes killing add_interpreter_constraint() and add_interpreter_constraints_from() to instead automatically be setting the interpreter constraints from the targets graph. This PR does not make that change for the scope, but this should be noted.

@Eric-Arellano Eric-Arellano changed the title Set Pex interpreter constraints with global constraints when running `./pants binary` Propagate Python Setup subsystem's interpreter constraints when building PEXes Feb 26, 2019

Remove bad import
My bad for not catching this before pushing.

@Eric-Arellano Eric-Arellano requested review from jsirois, benjyw and stuhood Feb 26, 2019

Revert "Constrain ci.sh to the exact Python interpreter version"
This reverts commit 887a8ef.

This change is necessary to fix the original motivation for this PR, but it does not really belong in this PR anymore. Instead, it should be in the Py2 ABI PR (7235). This PR should be kept more generic, and there is no logical connection to the changes being made with ci.sh, beyond that original motivating problem.

Eric-Arellano added some commits Feb 27, 2019

Fix issue with compatibility_or_constraints() returning a tuple
add_interpreter_constraints() expects a str, so we must unpack the tuple when calling it.

@stuhood stuhood requested a review from CMLivingston Feb 27, 2019

@stuhood
Copy link
Member

left a comment

This makes sense to me, but would be good to have Chris confirm before landing. I'll ping him.

Set tests as hermetic
I'm not able to reproduce the failure locally. It appears the test is failing though because it's picking up the global interpreter constraint which is causing a failure? We'll see if this works.

Eric-Arellano added some commits Feb 27, 2019

Improve comment for python_binary_create.py
Benjy had a fair point that this could have been more descriptive. Thanks for the suggestion Benjy!
Revert "Set tests as hermetic"
This reverts commit 76649e6.

Making the tests hermetic did not fix anything. In fact, it looks like it introduced new issues

@Eric-Arellano Eric-Arellano changed the title Propagate Python Setup subsystem's interpreter constraints when building PEXes Propagate global interpreter constraints when building PEXes with interpreter constraints requested Feb 27, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

I realized the scope is less significant than I had suggested with the original title and PR description. While we are indeed changing an API function for PexBuilderWrapper, at the moment the only user of add_interpreter_constraints_from() is python_binary_create.py. So the only command that should (theoretically) be impacted by this change is ./pants binary.

As shown in CI with the failing test_pytest_run_integration.py, however, this one change seems to have greater implications even though it does only touch ./pants binary.

@CMLivingston
Copy link
Contributor

left a comment

LGTM, thanks for catching this and fixing.

Eric-Arellano added some commits Mar 7, 2019

Audit if tests fail on pure Py2
Danny figured out that these tests are failing due to the Pexrc issues we've been encountering.

Does this failrue still hit us when running with pure Py2 instead of Py2 w/ Py3 constraints? Hopefully not, so that we can skip the tests during daily CI and still run them during cron job.
Skip tests failing due to upstream Pex issues
Now that we're setting interpreter constraints for the built binary, some of the tests are running into upstream Pex issues that occur when interpreter constraints are set. Specifically, we seem to be hitting pantsbuild/pex#655, which is that Pex does not properly OR interpreter constraints.

Because this PR is blocking #7235 (specifying Py2 ABI) and thus releasing Py3 wheels, we skip the impacted tests until it gets fixed upstream.
Revert "Audit if tests fail on pure Py2"
This reverts commit f55a87e.

Looks like they still fail on pure Py2. This is what I would have expected given the error.
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Reviewers: @cosmicexplorer pointed out the test failures were resulting from pantsbuild/pex#655. We now set interpreter constraints for the generated ./pants.pex, so when using the PEX in our CI are running into PEX's issue with not correctly ORing requirements, resulting in a failure to find an appropriate interpreter (example).

We must decide if it is acceptable to skip the failing tests or if we must instead fix upstream first. I would strongly prefer to allow the temporary skip, as this PR is blocking #7235, which itself blocks releasing Python 3 wheels (#7197). However, I want to be careful to not make a rash decision.

Update TODO to link to pex upgrade and OR pr
Now that we have the OR constraints PR up for Pex, we can make the TODO more descriptive by linking to it + linking to the Pex upgrade PR. Linking to the latter will help to ensure we remove the test skip once the upgrade goes through.
@stuhood

stuhood approved these changes Mar 7, 2019

Copy link
Member

left a comment

Thanks. Given that the fix is already posted and green upstream, I'm fine with merging this with these tests skipped.

@Eric-Arellano Eric-Arellano merged commit 4d63f8a into pantsbuild:master Mar 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:pex-interpreter-constraints branch Mar 7, 2019

@Eric-Arellano Eric-Arellano moved this from In progress to Done in Adding Python 3 support Mar 8, 2019

Eric-Arellano added a commit that referenced this pull request Mar 9, 2019

Specify ABI for pantsbuild.pants wheel and build with both UCS2 and U…
…CS4 (#7235)

### Problem
We should be marking the [ABI (application binary interface)](https://docs.python.org/3/c-api/stable.html) for the `pantsbuild.pants` wheel because it uses native code. Currently, we mark the ABI as `none`, which is incorrect per https://www.python.org/dev/peps/pep-0513/#ucs-2-vs-ucs-4-builds.

In particular, in Python 2, Python may be installed with either UCS2 (UTF-16) or UCS4 (UTF-8). We should be marking the wheel as either `cp27m` for UCS2 or `cp27mu` for UCS4.

As a result of marking the ABI, we must now produce more wheels. macOS defaults to UCS2. For Linux, "ucs4 is much more widespread among Linux CPython distributions." We do not want to rely on these assumptions, however, when releasing, as some users may not have these default unicode settings. So, instead we must release `pantsbuild.pants` as both a `cp27m` and `cp27mu` wheel, and rely on Pip to resolve which the user should use.

### Solution
At a high level, this PR does two things:
1. Marks that the ABI should be specified, rather than `none`.
1. Sets up 4 Travis shards so that we build both a `cp27m` and `cp27mu` wheel for both Linux and OSX. See https://travis-ci.org/pantsbuild/pants/builds/503639333 for the end result of this.

To setup the new shards, we use Pyenv to install new versions of Python 2 with the appropriate unicode settings, thanks to the env var `PYTHON_CONFIGURE_OPTS=--enable-unicode=ucs{2,4}` (https://stackoverflow.com/a/38930764). 

Because both the OSX UCS4 shard and Linux UCS2 shard already have Python 2.7 installed, we must install a Python 2.7.x version different than what is already there, and use `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS` to ensure Pants and PEX are using the exact interpreter we want. For this reason, this PR was blocked by #7285 to propagate interpreter constraints to PEX.

Specifically, we make these changes to achieve these two high level goals:

1. Modify [`src/python/pants/BUILD`](https://github.com/pantsbuild/pants/pull/7235/files#diff-3ce39309d74098493a1f3c8107292a8d) so that `bdist_wheel` knows it needs to mark the ABI. This achieves goal 1.
1. Change [`release.sh`](https://github.com/pantsbuild/pants/pull/7235/files#diff-9ed7102b7836807dc342cc2246ec4839) to allow pre-setting `$PY` and to also set `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS` in order to use the specific Python interpreter we are targeting.
1. Create [`travis_ci_py27_ucs2/Dockerfile`](https://github.com/pantsbuild/pants/pull/7235/files#diff-b90425bcccb6969a98b9d1c4066422a8) to get Python 2 w/ UCS2 onto Linux.
1. Extract out `.travis.yml` `env` entries to get OpenSSL and Homebrew-installed Python working properly, along with launching a Docker image, in order to avoid duplication: [`env_osx_with_pyenv.mustache`](https://github.com/pantsbuild/pants/pull/7235/files#diff-c2ab029a1887e2e99f2391fc7568cc5f), [`launch_docker_image.mustache`](https://github.com/pantsbuild/pants/pull/7235/files#diff-3d4a0cecc373a624f233521f76d66999), and [`generate_travis_yml.py`](https://github.com/pantsbuild/pants/pull/7235/files#diff-c11e2f109e12527d2e1ac2c62161edf6).
1. Modify [`travis.yml.mustache`](https://github.com/pantsbuild/pants/pull/7235/files#diff-88af3146f5cc486b749ed790399bde46) to set up the 4 distinct wheel building shards. Similar to how we created a Dockerfile to use Pyenv to install Python 2 with UCS2 on Linux, we use Pyenv to install Python 2 with UCS4 on OSX. We also move the wheel building shards below unit tests.

#### Ensuring the correct abi is used
We need to ensure the `pants.pex` used by `release.sh` has the correct abi for its dependencies, and that `release.sh` is using the correct Python interpreter. We introduce a new script [`check_pants_pex_abi.py`](https://github.com/pantsbuild/pants/pull/7235/files#diff-8b857b8cee6cb9784bf37950220c587b) that inspects the pex's `PEX-INFO` to ensure the targeted abi was used.

An even better test would test the result of `release.sh` to ensure the built `pantsbuild.pants` wheel has the correct ABI and can be consumed properly. Currently `release.sh` verifies the wheel is valid, but it does not enforce which ABI it was built with. This could be a good followup PR.

### Result
We now properly mark the ABI for Python 2. Beyond the new script `check_pants_pex_abi.sh` proving this, we performed a run of this PR with verbose PEX logging turned on: https://travis-ci.org/pantsbuild/pants/builds/503639333. Inspecting the logs for the wheel building shards and searching for `Using the current platform` proves the 4 wheel building shards are using the correct interpreter and abi.

In addition to correctness for Python 2, this unblocks releasing Python 3 wheels (#7197).

Note this should have no significant impact on the end user, as Pip will resolve to the current ABI for their interpreter. It will change the name of our `pantsbuild.pants` wheel and will prevent using that wheel with an interpreter that uses a different UCS setting, but all users should be able to pull down whichever wheel they need as we provide wheels for both UCS2 and UCS4 on both OSX and Linux.

#### Downside: wheel building explosion
We currently are building more wheels than necessary. For wheels that are universal / platform-independent, we only need them to be built once, but we build them every time. See #7258.

This PR adds two new shards so adds ~30 unnecessary core wheels we build, in addition to 3rd party wheels that are universal / platform-independent.

Eric-Arellano added a commit that referenced this pull request Mar 31, 2019

Remove backend/python/tasks tests from Python 3 CI blacklist and refa…
…ctor their BUILD entries to be more granular (#7463)

### Problem
Currently, you can only run `./pants test tests/python/pants_test/backend/python/tasks:tasks` or `./pants test tests/python/pants_test/backend/python/tasks:integration`. 

This lack of granularity makes it difficult to develop in this folder, as the former has a test timeout of 10 minutes and the latter of 40 minutes. While you can use `./pants test.pytest -- -k test_name`, this does not get the specific file but rather the names of individual tests.

It also makes it harder to tell which tests are causing the long timeouts for this folder.

### Solution
Each test file gets its own BUILD entry, as we do in most folders.

#### Also remove tests from Py3 blacklist
All tests now pass with Python 3. This is hypothesized to be thanks to:
1) Performance improvement thanks to #7447. As we recall, some of the tests were timing out, and this performance fix reduces the risk of timeouts.
2) Skipping Pexrc-related tests thanks to #7285.

Note all integration tests were ran four times in CI to check for flakiness.

#### Also refactor `test_python_run_integration.py`'s interpreter selection
Deduplicate the constraints and update the constraints to reflect our modern interpreter usage. For example, currently Py37 may not be chosen, which means our cron job would potentially fail.
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.