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.

Sign up for GitHub

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

Jump to bottom

slightly demystify the relationship between platforms and interpreters in the library API and CLI #1047

Merged
merged 10 commits into from
Sep 28, 2020
Merged

slightly demystify the relationship between platforms and interpreters in the library API and CLI #1047

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c11a607
update docstrings on resolve and download methods
cosmicexplorer Sep 27, 2020
ac19fb7
update the --platform and --resolve-local-platforms docs
cosmicexplorer Sep 27, 2020
097b61e
revert prior docstring changes, and add :raises ValueError:
cosmicexplorer Sep 27, 2020
cf2a276
update CLI help
cosmicexplorer Sep 28, 2020
5184435
now try to change the docstrings helpfully again
cosmicexplorer Sep 28, 2020
7b6b0fc
revert changes to --platform CLI arg help
cosmicexplorer Sep 28, 2020
2889556
use john's suggestion to distinguish platforms and interpreters
cosmicexplorer Sep 28, 2020
e614e7d
also add the clarification on built distributions to the --platform C…
cosmicexplorer Sep 28, 2020
2fc5078
document tiny edge case with platform matching
cosmicexplorer Sep 28, 2020
300cea7
respond to review comments
cosmicexplorer Sep 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions pex/bin/pex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,9 +500,11 @@ def configure_clp_pex_environment(parser):
callback=process_platform,
help="The platform for which to build the PEX. This option can be passed multiple times "
"to create a multi-platform pex. To use the platform corresponding to the current "
"interpreter you can pass `current`. To target any other platform you pass a string "
"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). "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explaining this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"To target any other platform you pass a string composed of fields: "
"<platform>-<python impl abbr>-<python version>-<abi>. "
"These fields stem from wheel name conventions as outlined in "
"https://www.python.org/dev/peps/pep-0427#file-name-convention and influenced by "
"https://www.python.org/dev/peps/pep-0425. For the current interpreter at {} the full "
"platform string is {}. To find out more, try `{} --platform explain`.".format(
Expand All @@ -518,8 +520,9 @@ def configure_clp_pex_environment(parser):
callback=parse_bool,
help="When --platforms are specified, attempt to resolve a local interpreter that matches "
"each platform specified. If found, use the interpreter to resolve distributions; if "
"not, resolve for the platform only allowing matching binary distributions and failing "
"if only sdists or non-matching binary distributions can be found.",
"not (or if this option is not specified), resolve for each platform only allowing "
"matching binary distributions and failing if only sdists or non-matching binary "
"distributions can be found.",
)

group.add_option(
Expand All @@ -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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I'm cool with removing it.

),
)

Expand Down
36 changes: 23 additions & 13 deletions pex/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,11 +733,13 @@ def resolve(
prerelease or development versions will override this setting.
:keyword bool transitive: Whether to resolve transitive dependencies of requirements.
Defaults to ``True``.
:keyword interpreter: The interpreter to use for building distributions and for testing
distribution compatibility. Defaults to the current interpreter.
:keyword interpreter: If specified, distributions will be resolved for this interpreter, and
non-wheel distributions will be built against this interpreter. If both `interpreter` and
`platform` are ``None`` (the default), this defaults to the current interpreter.
:type interpreter: :class:`pex.interpreter.PythonInterpreter`
: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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code here:
https://github.com/pantsbuild/pex/blob/cff40893dd1b0ae185201f4871baae87a9fe1335/pex/bin/pex.py#L758-L786

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jsirois jsirois Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you for piecing that together. Let me know if 2fc5078 seems sufficient to document that.

will be used to build any non-wheel distributions.
:keyword indexes: A list of urls or paths pointing to PEP 503 compliant repositories to search for
distributions. Defaults to ``None`` which indicates to use the default pypi index. To turn off
use of all indexes, pass an empty list.
Expand Down Expand Up @@ -765,6 +767,8 @@ def resolve(
:raises Unsatisfiable: If ``requirements`` is not transitively satisfiable.
:raises Untranslatable: If no compatible distributions could be acquired for
a particular requirement.
:raises ValueError: If a foreign `platform` was provided, and `use_wheel=False`.
:raises ValueError: If `build=False` and `use_wheel=False`.
"""
# TODO(https://github.com/pantsbuild/pex/issues/969): Deprecate resolve with a single interpreter
# or platform and rename resolve_multi to resolve for a single API entrypoint to a full resolve.
Expand Down Expand Up @@ -824,12 +828,14 @@ def resolve_multi(
prerelease or development versions will override this setting.
:keyword bool transitive: Whether to resolve transitive dependencies of requirements.
Defaults to ``True``.
:keyword interpreters: The interpreters to use for building distributions and for testing
distribution compatibility. Defaults to the current interpreter.
:keyword interpreters: If specified, distributions will be resolved for these interpreters, and
non-wheel distributions will be built against each interpreter. If both `interpreters` and
`platforms` are ``None`` (the default) or an empty iterable, this defaults to a list
containing only the current interpreter.
:type interpreters: list of :class:`pex.interpreter.PythonInterpreter`
:keyword platforms: An iterable of PEP425-compatible platform strings to resolve distributions
for. If ``None`` (the default) or an empty iterable, use the platforms of the given
interpreters.
for, in addition to the platforms of any given interpreters. The current interpreter will be
cosmicexplorer marked this conversation as resolved.
Show resolved Hide resolved
used to build any non-wheel distributions.
:type platforms: list of str
:keyword indexes: A list of urls or paths pointing to PEP 503 compliant repositories to search for
distributions. Defaults to ``None`` which indicates to use the default pypi index. To turn off
Expand Down Expand Up @@ -858,6 +864,8 @@ def resolve_multi(
:raises Unsatisfiable: If ``requirements`` is not transitively satisfiable.
:raises Untranslatable: If no compatible distributions could be acquired for
a particular requirement.
:raises ValueError: If a foreign platform was provided in `platforms`, and `use_wheel=False`.
:raises ValueError: If `build=False` and `use_wheel=False`.
"""

# A resolve happens in four stages broken into two phases:
Expand Down Expand Up @@ -1047,12 +1055,12 @@ def download(
prerelease or development versions will override this setting.
:keyword bool transitive: Whether to resolve transitive dependencies of requirements.
Defaults to ``True``.
:keyword interpreters: The interpreters to use for building distributions and for testing
distribution compatibility. Defaults to the current interpreter.
:keyword interpreters: If specified, distributions will be resolved for these interpreters.
If both `interpreters` and `platforms` are ``None`` (the default) or an empty iterable, this
defaults to a list containing only the current interpreter.
:type interpreters: list of :class:`pex.interpreter.PythonInterpreter`
:keyword platforms: An iterable of PEP425-compatible platform strings to resolve distributions
for. If ``None`` (the default) or an empty iterable, use the platforms of the given
interpreters.
for, in addition to the platforms of any given interpreters.
:type platforms: list of str
:keyword indexes: A list of urls or paths pointing to PEP 503 compliant repositories to search for
distributions. Defaults to ``None`` which indicates to use the default pypi index. To turn off
Expand All @@ -1075,8 +1083,10 @@ def download(
:keyword str dest: A directory path to download distributions to.
:keyword int max_parallel_jobs: The maximum number of parallel jobs to use when resolving,
building and installing distributions in a resolve. Defaults to the number of CPUs available.
:raises Unsatisfiable: If the resolution of download of distributions fails for any reason.
:returns: List of :class:`LocalDistribution` instances meeting ``requirements``.
:raises Unsatisfiable: If the resolution of download of distributions fails for any reason.
:raises ValueError: If a foreign platform was provided in `platforms`, and `use_wheel=False`.
:raises ValueError: If `build=False` and `use_wheel=False`.
"""

local_distributions, download_results = _download_internal(
Expand Down