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

Release more flexible pex binaries. #654

Closed
jsirois opened this issue Jan 24, 2019 · 11 comments
Closed

Release more flexible pex binaries. #654

jsirois opened this issue Jan 24, 2019 · 11 comments

Comments

@jsirois
Copy link
Member

jsirois commented Jan 24, 2019

Currently we release pex27 and pex37. Since pex is dependency free and supports both python 2.7 and python 3.4-3.7, we would ideally release one binary that could run under all these interpreters.

Typical systems will have on the PATH:

  1. python - the default python
  2. python2 - the default python 2.*
  3. python3 - the default python 3.*
  4. python3.7, python2.7, ... - the ~specific pythons

The current release process creates binaries with shebangs from the fourth category which has the upshot of not providing a binary for pythons 3.4, 3.5 or 3.6 even though both the pex27 and pex37 could otherwise run under these pythons.

We should release binaries to support 3.4, 3.5 and 3.6 either by releasing one binary that uses a shebang with python and interpreter constraints to ensure that python falls in the acceptable ranges or else by releasing two binaries as we do today but with more flexible python2 and python3 shebangs (really only needed for python3 since python2.7 is actually all we support in the 2 series). The third alternative of releasing one binary per minor supported version seems wasteful.

@jsirois
Copy link
Member Author

jsirois commented Jan 24, 2019

Urgh, --interpreter-constraint is buggy in a few ways foiling ideas one and two.

For idea one, passing multiple --interpreter-constraint does not OR, it ANDS - which the , operator already does and so is not useful. If instead multiple --interpreter-constraint OR'd then we could say --interpreter-constraint='>=2.7,<3' --interpreter-constraint='>=3.4,<4', which combined with a #!/usr/bin/env python shebang would allow for a safe universal pex. See here and note all calls pass meet_all_constraints=True:
https://github.com/pantsbuild/pex/blob/a16f0ecb21ce3526f7d243778507d68219c3a91c/pex/interpreter_constraints.py#L24-L26

For idea one and two, --interpreter-constraint only takes effect if PEX_PYTHON or PEX_PYTHON_PATH is set:
https://github.com/pantsbuild/pex/blob/a16f0ecb21ce3526f7d243778507d68219c3a91c/pex/pex_bootstrapper.py#L141-L150

@jsirois
Copy link
Member Author

jsirois commented Jan 24, 2019

I've gone ahead and brute-forced for the 1.6.1 release by including pex{27,3{4,5,6,7}}: https://github.com/pantsbuild/pex/releases/tag/v1.6.1

I'll leave this issue open until I break out --interpreter-constraint issues to track the prerequisites for doing all this the right way.

@jsirois
Copy link
Member Author

jsirois commented Jan 25, 2019

@CMLivingston if you can comment on this in the form of either - ack, sounds unintended or, to the contrary, that there was intent behind these behaviors, I'd be grateful.

@CMLivingston
Copy link
Contributor

CMLivingston commented Jan 25, 2019

This was intended. The confusion and issues we are seeing that stem from OR vs AND are largely due to my ignorance of pex use cases (I was relatively new to the codebase when I made these changes), and I was having difficulty trying to understand why anyone would OR at all within the context of the change I was making at the time, which was to move from a single PEX_PYTHON (use this interpreter) -> PEX_PYTHON_PATH (use one of these based on constraints of the binary). That was the use case I was targeting with meet_all_constraints=true - to target a specific interpreter on PPP, assuming that the use case there is to have control over the exact interpreter you need (iirc there was some issues with the , AND once you started getting complicated and went beyond simple ones like CPython>=2.7,<3, but I am not completely sure).

Long story short, it was intended for the use case and I was confused at the time / thought that ORing constraints was a bug, so I added that option in to be more explicit and give finer grain control. Ultimately, I think the custom nature of PPP and PP is different from what you are describing here because it is much more pointed, custom setting regarding which interpreter to use vs. giving the freedom and flexibility to encompass a wide range (this being largely because it grew out of the singular PEX_PYTHON). By design, PPP-based selection will not fall back to $PATH, and will error out if the AND is not satisfied. Admittedly, this was heavily influenced by monorepo philosophy / pants, where we were trying to go from one canonical interpreter to two, feature-incompatible canonical interpreters.

I think one possible solution here would be to fragment the call to matched_interpreters based on this condition https://github.com/pantsbuild/pex/blob/a16f0ecb21ce3526f7d243778507d68219c3a91c/pex/pex_bootstrapper.py#L80 and use meet_all_constraints=False, reserving the AND for PPP/PP only. I need to ramp up on other places this bites us (so there may be a case for never ANDing), but given my current knowledge level I think it would make sense to fragment.

@jsirois
Copy link
Member Author

jsirois commented Jan 27, 2019

Thanks @CMLivingston - that helps me move forward without breaking Twitter things.

@jsirois
Copy link
Member Author

jsirois commented Jan 30, 2019

Noting that full support for multi-python pex binaries will also require fixing #415.

@stuhood
Copy link

stuhood commented Feb 6, 2019

Hey @jsirois ! Is there any separable work that could be broken out of this that we (Twitter) could prioritize in order to help get this fixed?

@jsirois
Copy link
Member Author

jsirois commented Feb 6, 2019

Yes - all the linked issues here that are not labelled in-progress, so:

If anyone does pitch in, please assign the issue to yourself and label in-progress.

@stuhood
Copy link

stuhood commented Feb 6, 2019

@jsirois : Ok, thank you! Is there a best choice issue for someone to start with? If you add a few more details to "the most separable" issue, I can commit one of us to finishing it before the end of next week.

@jsirois
Copy link
Member Author

jsirois commented Feb 6, 2019

#656 seems teed up already to me. I'm happy to answer questions if that somehow is not explicit enough.

@jsirois
Copy link
Member Author

jsirois commented Feb 6, 2019

And fwiw - @CMLivingston should be intimately familiar with #655 and #656 both.

@jsirois jsirois mentioned this issue Feb 11, 2019
2 tasks
Eric-Arellano added a commit to Eric-Arellano/pex that referenced this issue Mar 7, 2019
It looks like we need to keep in some places the behavior of ANDing a list of constraints per pex-tool#654 (comment). But in general we want to OR.

If PEX_PYTHON or PEX_PYTHON_PATH are not set, OR.
@jsirois jsirois mentioned this issue Mar 10, 2019
3 tasks
Eric-Arellano added a commit to pantsbuild/pants that referenced this issue Mar 14, 2019
### Problem
The final step in #6062 is to add support for releasing Pants and its contrib packages as compatible with Python 2.7 _and_  3.6 and 3.7. Specifically, universal wheels should be marked as `py27.py36.py37` and `pantsbuild.pants` should be marked as `cp27-cp27{m,mu}` or as `cp36-abi3` because it uses native code (see [comment in packages.py](https://github.com/pantsbuild/pants/pull/7197/files#diff-cccea5c0944f25138aa83f5815236ea2R95) for an explanation on `abi3`).

`packages.py` and `release.sh` had several issues preventing building Python 3 wheels. Further, we needed to add shards to build Python 3 wheels in CI.

This resolves the wheels portion of #6450.

### Solution
* Add a `-3` CLI flag to `release.sh` to indicate it should build wheels compatible with Py3. Also add `--py3` to `packages.py`.
* Update the default `Package` config to setup support for `py27.py36.py37`, meaning any Python implementation that is one of those 3 versions.
* Update the `pantsbuild.pants` flags to indicate either `cp27` or `cp36-abi3`. The former is only compatible with CPython2.7, and the latter is compatible with any CPython implementation >= 3.6 due to the ABI constraint (see https://docs.python.org/3/c-api/stable.html).
* Add Python 3 wheels shards to CI.
* Pull down either a pex27 or pex36 release, rather than alway using pex27.
* Modernize Python code, such as fixing bad `subprocess32` import.
* Remove the compatibility tags from `PantsRequirement`. John explained it was wrong to put here, I think because it should be marked in` packages.py` when we actually build the wheels? This results in also tweaking the corresponding unit tests.

### Result
`./build-support/bin/release.sh -3n` builds Python 3 compatible wheels ready for release.

Note we still cannot release a Python 3 compatible PEX, which is blocked by pex-tool/pex#654.
Eric-Arellano added a commit to pantsbuild/pants that referenced this issue Mar 20, 2019
### Problem
While wheels are now released with Python 3 support thanks to #7197, this is only one of two ways people consume Pants. The other way is through a Pex released to Github. 

We decided to release both a Py27 and Py36 Pex. 

We will not be releasing a Py37 Pex, as it is would require Py37 wheel building shards which would require a Centos7 docker image. We decided this is not worth the work for how few people we anticipate would use Py37 with Pex combined.

We will also not be releasing a more flexible Pex that works with either Py27 or Py36, as it is blocked by pex-tool/pex#654. We also want organizations pinning their Python version, so a flexible release is not particularly useful in this instance.

This implements #6450 (comment).

### Solution
Release both `pants.${VERSION}.py{27,36}.pex`.

This requires changing how we run `release.sh -p` and adding new CI shards to deploy on both Py2 and Py3.

### Result
The next release, we will release both `pants.1.15.0.rc1.py27.pex` and  `pants.1.15.0.rc1.py36.pex`.
stuhood pushed a commit to pantsbuild/pants that referenced this issue Mar 21, 2019
### Problem
While wheels are now released with Python 3 support thanks to #7197, this is only one of two ways people consume Pants. The other way is through a Pex released to Github. 

We decided to release both a Py27 and Py36 Pex. 

We will not be releasing a Py37 Pex, as it is would require Py37 wheel building shards which would require a Centos7 docker image. We decided this is not worth the work for how few people we anticipate would use Py37 with Pex combined.

We will also not be releasing a more flexible Pex that works with either Py27 or Py36, as it is blocked by pex-tool/pex#654. We also want organizations pinning their Python version, so a flexible release is not particularly useful in this instance.

This implements #6450 (comment).

### Solution
Release both `pants.${VERSION}.py{27,36}.pex`.

This requires changing how we run `release.sh -p` and adding new CI shards to deploy on both Py2 and Py3.

### Result
The next release, we will release both `pants.1.15.0.rc1.py27.pex` and  `pants.1.15.0.rc1.py36.pex`.
jsirois added a commit to jsirois/pex that referenced this issue Apr 3, 2019
This was referenced Apr 3, 2019
jsirois added a commit that referenced this issue Apr 3, 2019
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