-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
PEXEnvironment recursive runtime resolve. #1165
PEXEnvironment recursive runtime resolve. #1165
Conversation
This removes our dependency on pkg_resources Environment / WorkingSet in favor of performing our own recursive resolve of runtime distributions to activate using distribution metadata. This fixes an old test bug noticed by Benjy but, more importanty, sets the stage to fix pex-tool#899, pex-tool#1020 and pex-tool#1108 by equipping PEXEnvironment with the ability to resolve the appropriate transitive set of distributions from a root set of requirements instead of the current full set of transitive requirements stored post-resolve in PexInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
So, for my own edification: implementing resolve is scary, but this is supposed to be a simpler problem because we only ever consume the output(s) of valid PIP resolves?
@classmethod | ||
def create( | ||
cls, | ||
requirement, # type: Requirement | ||
required_by=None, # type: Optional[Distribution] | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can also provide defaults by overriding __new__
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - this style has always seemed cleaner / clearer to me and pervades Pex code.
# here. | ||
self._supported_tags_to_rank = { | ||
tag: rank | ||
for rank, tag in enumerate(reversed(self._interpreter.identity.supported_tags)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the ordering of the supported_tags
guaranteed by identity
? Should this ranking/ordering be implemented there instead to avoid relying on the guarantee outside the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it guaranteed by packaging.tags
so its not easy or wise to try to repro. The contract is PythonInterpreter / PythonIdentity grab their supported tags from packaging and do not disturb the ordering since it is vital.
https://github.com/pantsbuild/pex/blob/8d4f3db943ff0d3da47cd7225e73fb56bc0a99e5/pex/vendor/_vendored/packaging/packaging/tags.py#L833-L840
# We want to pick one requirement for each key (required project) to then resolve | ||
# recursively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make any guarantees in cases where there are multiple/redundant definitions of a particular requirement? i.e.
isort==4.3
isort==4.4
Will the ranking extend into taking a particular version? Or is the universe smaller here, such that that case is impossible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the ranking extend into taking a particular version?
The rank is 1st by tags (the more platform specific the higher), then by version (greater version higher).
for ranked_dist in self._available_ranked_dists_by_key[ | ||
requirement.key | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this dict lookup fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This ties into your worry about resolves. By definition there is a successful resolve stored in the PEX file - by Pip right now. At runtime we can only fail to find a version of a requirement applicable to a given interpreter. So the key is guaranteed to be there, just a matching (platform specific) distribution is missing when we fail a resolve like this.
Yes. This resolve is at runtime over the contents of a PEX, which is populated by a real resolver - Pip for now - at build time. Its no different than the prior runtime resolve - which happened inside the otherwise innocuous looking WorkingSet.resolve call. For maximum clarity - Pex has been performing runtime resolves of this character since at least day 1 of supporting multiplatform PEXes if not earlier. The only thing new in this PR is we take over from WorkingSet.resolve (it was also recursive). The next PR will actually insert a real change and the Pex build time will switch from storing 1 requirement string per resolved distribution to just the root requirement strings passed on the CLI (and through -r). |
Got it. Which is also not PIP. |
Exactly. |
This removes our dependency on pkg_resources Environment / WorkingSet in
favor of performing our own recursive resolve of runtime distributions
to activate using distribution metadata. This fixes an old test bug
noticed by Benjy but, more importanty, sets the stage to fix #899, #1020
and #1108 by equipping PEXEnvironment with the ability to resolve the
appropriate transitive set of distributions from a root set of
requirements instead of the current full set of transitive requirements
stored post-resolve in PexInfo.