Minimize interpreter bootstrapping in tests. #571
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
Master is currently timing out trying to fetch and build 7 interpreters. Since We have no need for seven, constrain the interpreter count. Along the way fix a flaky test that exposed production code not setting up all interpreters.
|
I'll TBR this as needed when it goes green to get master green. |
|
I'm not very familiar with the PEX directory structure. Is this all test code, or is any source code? Will review next few minutes! |
|
The only production code change is in pex/bin/pex.py here: https://github.com/pantsbuild/pex/pull/571/files#diff-178808e5641b15e64b2d09f744976f03 |
| interpreters = find_compatible_interpreters(pex_python_path, ['<3']) | ||
| assert interpreters[0].binary == pex_python_path.split(':')[0] # 2.7.9 | ||
| assert [py35, py36] == find_interpreters('>3') | ||
| assert [py27] == find_interpreters('<3') |
There was a problem hiding this comment.
Nice, I really like how readable these tests are.
|
This has brought the cold cache CI times on OSX down from 50+ minutes to 30- minutes - so looks like it'll do. |
Eric-Arellano
approved these changes
Oct 2, 2018
CMLivingston
reviewed
Oct 2, 2018
|
|
||
| interpreters = find_compatible_interpreters(pex_python_path, ['>3.5.4']) | ||
| assert interpreters[0].binary == pex_python_path.split(':')[5] # 3.6.2 | ||
| assert [py36] == find_interpreters('>{}'.format(PY35)) |
There was a problem hiding this comment.
Part of the reason I made these tests at this level of granularity was to assure that patch-level version constraints between constraints with the same major versions are respected, however I would imagine that is covered in testing elsewhere so this change is fine with me.
There was a problem hiding this comment.
Understood. Yeah, the underlying comparison is done by PythonIdentity.matches which is actually not unit-tested, but it could and should be. The testing here was just way too expensive.
CMLivingston
reviewed
Oct 2, 2018
jsirois
added a commit
to jsirois/pex
that referenced
this pull request
Oct 8, 2018
PR pex-tool#571 regressed the half-broken state of having `--interpreter_constraint` selected interpreters not setup to also having `--python` selected interpreters also not setup. In addition, PR pex-tool#568 incorrectly classified the current Platform passed by `resolve_multi` as a user-specified extended platform specification breaking custom interpreter resolution. Fix both and add tests that failed prior to this combination of fixes. A more comprehensive fix is tracked in part by pex-tool#579.
jsirois
added a commit
that referenced
this pull request
Oct 8, 2018
PR #571 regressed the half-broken state of having `--interpreter_constraint` selected interpreters not setup to also having `--python` selected interpreters also not setup. In addition, PR #568 incorrectly classified the current Platform passed by `resolve_multi` as a user-specified extended platform specification breaking custom interpreter resolution. Fix both and add tests that failed prior to this combination of fixes. A more comprehensive fix is tracked in part by #579.
Laevos
pushed a commit
to Laevos/pex
that referenced
this pull request
Oct 9, 2018
# The first commit's message is: pex-tool#572: Allow import of ctypes to be skipped if use_manylinux is false # This is the 2nd commit message: Narrow the env marker test. (pex-tool#578) The jupyter dist is just a meta-dist with fully unconstrained deps on ~6 other dists. This test was added to test environment marker support in pex, which ipython - not jupyter - leverages heavily. # This is the 3rd commit message: Fix resolve regressions introduced by the 1.4.8. (pex-tool#580) PR pex-tool#571 regressed the half-broken state of having `--interpreter_constraint` selected interpreters not setup to also having `--python` selected interpreters also not setup. In addition, PR pex-tool#568 incorrectly classified the current Platform passed by `resolve_multi` as a user-specified extended platform specification breaking custom interpreter resolution. Fix both and add tests that failed prior to this combination of fixes. A more comprehensive fix is tracked in part by pex-tool#579. # This is the 4th commit message: Cleanup `PexInfo` and `PythonInterpreter`. (pex-tool#581) Kill an unused type in `PexInfo` as well as our last remaining use of `pkg_resources.get_platform`. Also kill unused `COMPATIBLE_SETUPTOOLS` constants in `PythonInterpreter`. # This is the 5th commit message: Support environment markers during pex activation. (pex-tool#582) We've had support for environment markers on the resolve side for a while and with just a little plumbing we can now support multi-python pexes with environment-specific requirements. Fixes pex-tool#456 # This is the 6th commit message: Revert "Support environment markers during pex activation. (pex-tool#582)" This reverts commit 5f1f00f. We want to do a 1.4.9 bugfix release before this ~API change. # This is the 7th commit message: Prepare the 1.4.9 release. (pex-tool#588) Work towards pex-tool#583 # This is the 8th commit message: Revert "Revert "Support environment markers during pex activation. (pex-tool#582)"" This reverts commit 44ff463. This restores pex-tool#582 for the 1.5.0 release tracked by pex-tool#585.
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.
Master is currently timing out trying to fetch and build 7 interpreters.
Since We have no need for seven, constrain the interpreter count. Along
the way fix a flaky test that exposed production code not setting up all
interpreters.