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

The --interpreter-constraint option should OR #655

Closed
jsirois opened this issue Jan 25, 2019 · 4 comments
Closed

The --interpreter-constraint option should OR #655

jsirois opened this issue Jan 25, 2019 · 4 comments
Assignees

Comments

@jsirois
Copy link
Member

jsirois commented Jan 25, 2019

Right now, the full list of interpreter constraints is ANDed. This is boh redundant (the comma operator already can AND constraints inside a single constraint string) and restrictive. There are common important cases where disjoint interpreter constraints are desirable; eg: A "universal" pex that supports being run under python 2.7 or python 3.4-3.7. Pex itself is an example of this.

@jsirois
Copy link
Member Author

jsirois commented Jan 25, 2019

Discovered in #654 and pantsbuild/pants#7097

@CMLivingston
Copy link
Contributor

CMLivingston commented Feb 28, 2019

But why wouldn't the author user just widen the single constraint in this case? Is this for when they want an incompatible "gap" in the middle?

A "universal" pex that supports being run under python 2.7 or python 3.4-3.7. Pex itself is an example of this.

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this issue Mar 7, 2019
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 pex-tool/pex#655, which is that Pex does not properly OR interpreter constraints.

Because this PR is blocking pantsbuild#7235 (specifying Py2 ABI) and thus releasing Py3 wheels, we skip the impacted tests until it gets fixed upstream.
Eric-Arellano added a commit to Eric-Arellano/pex that referenced this issue Mar 7, 2019
@Eric-Arellano Eric-Arellano self-assigned this Mar 7, 2019
@Eric-Arellano
Copy link
Contributor

In #678, I try to resolve this by following the advice from
#654 (comment). If PEX_PYTHON or PEX_PYTHON_PATH are specified, we keep the default AND behavior, else we OR.

This feels wrong to me. I would expect as a user consistent behavior regardless of those env vars being set. However, it sounds like Twitter has some use cases where they need a list of constraints to AND.

What do you all think of this proposal: introduce a new setting/env var, e.g. PEX_AND_INTERPRETER_CONSTRAINTS, that will turn on the AND behavior regardless of what values PEX_PYTHON and PEX_PYTHON_PATH have. With deprecation policy, we can do something like default this to be true for two releases, perhaps.

cc @stuhood @jsirois @CMLivingston

@CMLivingston
Copy link
Contributor

We should be able to just OR them in both cases and things should be fine. It should not break Twitter usage. The only reason it makes sense to AND for PPP is because with PPP you will deterministically select the lowest compatible interpreter on PPP if we OR the constraints. This shouldn't be an issue if users expect this and set constraints on pants.ini and Python targets accordingly within Pants context.

Eric-Arellano added a commit to Eric-Arellano/pex that referenced this issue Mar 7, 2019
Per pex-tool#655 (comment), Chris is okay with always ORing. This is a much more consistent and simpler approach, so is ideal over the earlier conditional logic.
Eric-Arellano added a commit to pantsbuild/pants that referenced this issue Mar 7, 2019
…erpreter constraints requested (#7285)

### 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 pex-tool/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 pex-tool/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 pex-tool/pex#678 is merged into PEX and Pants upgrades its PEX to the new version #7186.
Eric-Arellano added a commit that referenced this issue Mar 10, 2019
## Problem
Resolves #655. 

We would expect a list of constraints, such as `['CPython>=2.7,<3', 'CPython>=3.4']`, to OR the separate list entries, i.e. allow an interpreter that satisfies any of these distinct requirement strings. Instead, we always AND lists of constraints.

## Solution
Always `OR` a list of interpreter constraints. If the user wants `AND`, they may do so within the single requirement string via `,`, e.g. `'CPython>=2.7,<3'`.
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

3 participants