Select PEX runtime interpreter robustly. #1770
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously two proxies for interpreter applicability were used: 1. The shebang selected interpreter or the explicit interpreter used to invoke the PEX. 2. Any embedded interpreter constraints. This could lead to selecting an interpreter that was not actually able to resolve all required distributions from within the PEX, which is the only real criteria, and a failure to boot. Fix the runtime interpreter resolution process to test an interpreter can resolve the PEX before using it or re-execing to it. In the use case, the resolve test performed is cached work and leads to no extra overhead. In the re-exec case the resolve test can cost O(100ms), but at the benefit of ensuring either the selected interpreter will definitely work or no interpreters on the search path can work. Fixes pex-tool#1020
jsirois
commented
May 18, 2022
| target=LocalInterpreter.create(interpreter), | ||
| ) | ||
| try: | ||
| pex_environment.resolve() |
There was a problem hiding this comment.
FYI: This resolve test right here was what indirectly caused the need for #1768 and killing pkg_resources use. That used a bunch of pkg_resources.Requirement.parse and pkg_resources.Distribution finding and loading code and led to the boot-time vendor meta_path importer clash with Pex said same.
jsirois
commented
May 18, 2022
| target = target or targets.current() | ||
| return cls(pex=pex, pex_info=pex_info, target=target) | ||
| key = (pex_file, pex_hash, target) | ||
| mounted = cls._CACHE.get(key) |
There was a problem hiding this comment.
This new mount caching combines with the pre-existing instance-level resolve() caching to avoid resolving twice in the happy path of the interpreter that tries to run a PEX 1st being a compatible one.
|
This change probably means shebang could be defaulted to |
Which use case? I'm not following. |
|
Yooze, not yoos. Languages are dumb. In the case where the tested interpreter is the current interpreter and no re-exec is needed, there is no introduced overhead. The In the case where the current interpreter is not used and we instead re-exec using the foreign tested interpreter, the test will be run a second time. |
Eric-Arellano
approved these changes
May 18, 2022
The same as it's been for a while when no interpreter meets the criteria: That's as opposed to before this change where the interpreter nominally met the critera, but failed: Let me see how bad it it to get: Be something like: The full detail of the: Error above is available for each and every interpreter that fails the resolve test, but that's > 6000 lines of error output to have to read through in this example. |
|
I'll follow-up here with the error message change since it's semi-significant. It won't go in the release notes but will go in the release. |
|
Hrm, not awesome: |
|
The error message fix above is in: #1772 |
benjyw
reviewed
May 20, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously two proxies for interpreter applicability were used:
to invoke the PEX.
This could lead to selecting an interpreter that was not actually able
to resolve all required distributions from within the PEX, which is the
only real criteria, and a failure to boot.
Fix the runtime interpreter resolution process to test an interpreter
can resolve the PEX before using it or re-execing to it.
In the use case, the resolve test performed is cached work and leads to
no extra overhead. In the re-exec case the resolve test can cost
O(100ms), but at the benefit of ensuring either the selected interpreter
will definitely work or no interpreters on the search path can work.
Fixes #1020