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

Re-evaluate that platforms turns off interpreter_constraints #13904

Open
Eric-Arellano opened this issue Dec 16, 2021 · 6 comments
Open

Re-evaluate that platforms turns off interpreter_constraints #13904

Eric-Arellano opened this issue Dec 16, 2021 · 6 comments
Labels
backend: Python Python backend-related issues bug

Comments

@Eric-Arellano
Copy link
Contributor

TODO: find the original ticket and motivation for why we made this change

John has suggested this is a mistake. TODO: find references to his feedback.

This caused two users to be confused in Slack. When building a binary with the platform set to Py39 and Py310, the PEX was being run with Python 3.8. This is because the PEX-INFO shows interpreter_constraints is set to [] and the shebang is not set based on platforms. They worked around it by setting the shebang field, but that's not optimal.

@Eric-Arellano Eric-Arellano self-assigned this Dec 16, 2021
@kaos
Copy link
Member

kaos commented Dec 16, 2021

Make it three. Only I never followed up on it..

@jsirois
Copy link
Contributor

jsirois commented Dec 17, 2021

Here's a shot at explaining it all:

  • Without ICs the interpreter selected by the shebang is the one that will be used, full stop. If its not compatible with the various sets of distributions embedded in the PEX given the original requirements which are also embedded in the PEX-INFO, the PEX will fail to boot.

... That's really it. So if your PEX should only be compatible with 1 Python major/minor version, you might get away with the shebang alone if its both at major/minor precision. If your PEX should be compatible with more than one major/minor version but not all minors for a given major (Say 3.8 and 3.9 only), then shebang won't cut it, even #!/usr/bin/env python3 could select Python 3.7 on a machine that otherwise has Python 3.8 on the PATH.

No mention of platforms yet. But given the above, you can apply to the presence of one or more platforms or ICs or combinations thereof.

The only mitigation to this reality today that I can think of is encapsulated in pex-tool/pex#1020. At the cost of startup latency, that would allow the most generic #!/usr/bin/env python shebang to boot any PEX with any combination of platforms and not require ICs to guide interpreter re-selection post shebang startup.

@jsirois
Copy link
Contributor

jsirois commented Dec 17, 2021

Short of pex-tool/pex#1020 there is a bit that could be done to auto-correct / auto-prevent some forms of shebang-platform-IC misalignment. Filed pex-tool/pex#1540 for that. That said, platforms is an advanced feature and will remain one if only because of the pre-built wheel restriction. To practically use platforms, an organization has to invest and prop up their own wheel building infrastructure. So this comes down to, turn off a feature that's useful to some organizations but impossible to make fool-proof, or support the feature but make it clear it requires you to know what you're doing or be willing to read up.

@Eric-Arellano
Copy link
Contributor Author

Ah ha, I found the reason we turned this off: --interpreter-constraint means that current will be used as a platform. #9563 and pex-tool/pex#957.

@jsirois would you be open to revisiting Pex's behavior? Iiuc, I'd expect the behavior of pex --platform=linux-x86_64-cp-37-cp37m --interpreter-constraint 'CPython>=3.6' --output-file test.pex cryptography to be only resolving for the specified linux platform and also updating PEX-INFO with the interpreter-constraints. I would not expect it to resolve for the current platform unless --resolve-local-platforms was set.

@jsirois
Copy link
Contributor

jsirois commented Jan 7, 2022

I would. The correct fix is described here: pex-tool/pex#1020

If it's unclear why that is needed, forget platforms and consider an IC range spanning 3.7 and 3.8 where the local machine only has 3.8. Build time and run time constraints rarely will align, and ICs are a blunt instrument for runtime validation.

If it's still unclear or you think I've missed something, a detailed scenario or two might be good.

@Eric-Arellano
Copy link
Contributor Author

pex-tool/pex#1020 looks great, thanks!

Until then, I'm thinking we keep Pants's behavior as is. It's broken and has tripped up some users. But also I think it's a non-starter for current to always be included in the platforms, hence why we changed this in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues bug
Projects
None yet
Development

No branches or pull requests

4 participants