slightly demystify the relationship between platforms and interpreters in the library API and CLI #1047
Conversation
cosmicexplorer
added
enhancement
resolver
API
|
Ok, I've made another change to the docstring. In this case I have:
My final question is: is it possible to successfully build wheels for foreign platforms from pure-python libraries that are only provided as EDIT: ah, it appears the |
pex/bin/pex.py
Outdated
| "composed of fields: <platform>-<python impl abbr>-<python version>-<abi>. These fields " | ||
| "stem from wheel name conventions as outlined in " | ||
| "interpreter you can pass `current` " | ||
| "(which is the default value if this option is not provided). " |
There was a problem hiding this comment.
The parenthetical contradicts the default above and its not always true either. Consider the case where 1 interpreter is specified - say via --python python2.7 and yet the Pex CLI is running under Python 3.6. In that case the resulting PEX file will only be built using a local Python 2.7 interpreter and not additionally for current which is a local Python 3.6 interpreter. How about just dropping this addition?
There was a problem hiding this comment.
Thank you for explaining this.
There was a problem hiding this comment.
After reverting the change you noted, I modified this further to include the distinction you gave above:
If any distributions need to be built, use the --python or --interpreter-constraint argument instead, providing the corresponding interpreter.
pex/resolver.py
Outdated
| :keyword str platform: The exact target platform to resolve distributions for. If ``None`` or | ||
| ``'current'``, resolve for distributions appropriate for `interpreter`. | ||
| :keyword str platform: The exact PEP425-compatible platform string to resolve distributions for, | ||
| in addition to the platform of the given interpreter, if provided. The current interpreter |
There was a problem hiding this comment.
The current interpreter will be used to build any non-wheel distributions.
This is not true. If non-wheel distributions are encountered when resolving for the given platform, resolution will fail.
There was a problem hiding this comment.
Perhaps the confusion is that the CLI, which uses the API, will turn --platform ... arguments into PythonInterpreters when ---resolve-local-platforms. It then passes interpreters for those platforms it could resolve instead of passing platforms. Does that clear things up? I.E.: --resolve-local-platforms is a CLI feature and not a resolve API feature.
There was a problem hiding this comment.
What if the platform matches the supported_tags of the current interpreter? I'm looking at https://github.com/pantsbuild/pex/blob/e67412c31648f6e0d0f2a68b9d38dffabb258ebc/pex/distribution_target.py#L31-L34 and https://github.com/pantsbuild/pex/blob/e67412c31648f6e0d0f2a68b9d38dffabb258ebc/pex/interpreter.py#L61-L72, and it seems like tags.sys_tags() can return more than one element in its tuple: https://github.com/pantsbuild/pex/blob/e67412c31648f6e0d0f2a68b9d38dffabb258ebc/pex/vendor/_vendored/packaging/packaging/tags.py#L372-L374
There was a problem hiding this comment.
There was a problem hiding this comment.
What if the platform matches the supported_tags of the current interpreter? I'm looking at
We raced each other. I think you're confusing a CLI feature for an API feature. Let me know if I've guessed your confusion or if it persists.
There was a problem hiding this comment.
Which isn't exactly rare, now that I think of it -- I believe platform='current' would do the trick. I think it would be useful to add the sentence I proposed above, but if that seems like too much information I'm not beholden to it.
There was a problem hiding this comment.
Ah! But we have a specific check for that! https://github.com/pantsbuild/pex/blob/e67412c31648f6e0d0f2a68b9d38dffabb258ebc/pex/resolver.py#L972-L974
It seems like we might want to consider expanding that check to cover when the platform matches any of the given interpreters, in addition to checking if it's just None. I don't know if there's any deduplication later. I'll test out what happens if you give an explicit platform string matching the current interpreter.
There was a problem hiding this comment.
You're losing me. The block of code you point to just ensures that for platforms=[None] (which is how parsed_platforms translates platforms=['current']) and interpreters=None or interpreters=[] we use the current interpreter as the single interpreter to resolve with.
The general search you describe is what the CLI does in the code I pointed to above in pex/bin/pex.py when you sepcify --resolve-local-platforms.
There was a problem hiding this comment.
If you want to document the edge case where platforms=['X', ...] and 'X' happens to match the current interpreter accessing the Pex API, that's fine with me. But, to be clear, 'X' could either be 'current' in that case or a full platform string that just happens to match the current interpreter.
There was a problem hiding this comment.
Yes, thank you for piecing that together. Let me know if 2fc5078 seems sufficient to document that.
cosmicexplorer
force-pushed
the
make-platforms-more-obvious-in-resolve-docstring
branch
2 times, most recently
from
3e79102
to
ca80d0d
Compare
cosmicexplorer
force-pushed
the
make-platforms-more-obvious-in-resolve-docstring
branch
from
ca80d0d
to
2fc5078
Compare
pex/bin/pex.py
Outdated
| @@ -533,7 +536,8 @@ def configure_clp_pex_environment(parser): | |||
| "compatible Python version. Normally, when multiple interpreters match, Pex will " | |||
| "resolve requirements for each interpreter; this allows the resulting Pex to be " | |||
| "compatible with more interpreters, such as different Python versions. However, " | |||
| "resolving for multiple interpreters results in worse performance." | |||
| "resolving for multiple interpreters will take longer to build, and the resulting PEX " | |||
| "will be larger." | |||
There was a problem hiding this comment.
s/will be/may be/ - the tarpit again. It will only be larger if there are wheels that vary. If its a pure universal wheel resolve for example, this is false as is.
There was a problem hiding this comment.
My only concern with the original wording is that I might be worried upon reading the help for the first time that there's e.g. some weird bug, or some complex emergent effect, that causes multi-interpreter PEX files to run more slowly. I would have left it at "takes longer to build", but there is at least one runtime effect -- namely the file size (which as you pointed out is not guaranteed to differ -- have implemented your edit).
I think that too much information, especially in CLI help, can absolutely be overwhelming (and I know I am prone to causing this more than others). I would be fine reverting this too, although I think this one is much more defensible than the change to --platform help that I just reverted.
There was a problem hiding this comment.
Yeah - this is a smell of the feature. It was added for Pants use but does not make much sense for Pex use. It's in fact still buggy and will only be fixed #1020. That said, Eric and I came to light agreement on killing the feature since Pants no longer uses it.
There was a problem hiding this comment.
Ok, that sounds fine then. I'll perhaps leave this change in then just to be done with it, but from what I can tell killing the feature seems quite reasonable. In the future I suspect that Pants could make use of checked-in python scripts like ipex_launcher.py to make Pex tools that work with the process execution framework (and avoid churn in the upstream Pex project).
There was a problem hiding this comment.
Yep, I'm cool with removing it.
@jsirois read my mind in #1033 (comment) after I asked why Pex couldn't resolve for an interpreter it doesn't have locally. This was in fact already possible, and he suggested a documentation fix to make it more clear. These changes make it clear that:
platform{,s}takes precedence overinterpreter{,s}when resolving wheelsinterpreter{,s}are used to seedplatform{,s}if not providedinterpreter{,s}must be provided for allplatform{,s}if any distributions need to be built from sourceCloses #1033.