-
-
Notifications
You must be signed in to change notification settings - Fork 644
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
pin the PEX_PYTHON{,_PATH} running the pytest pex to avoid using incompatible pytest requirements #7563
pin the PEX_PYTHON{,_PATH} running the pytest pex to avoid using incompatible pytest requirements #7563
Conversation
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.
This is great, thanks!
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! would be nice to have some tests if they are not too expensive.
I think that's reasonable, especially given that some tests are failing in the current CI run. Investigating those failures might be able to produce a new test that checks for the corrected behavior. |
# Ensure the pex is invoked with the interpreter we selected beforehand to be compatible with | ||
# all the relevant targets. | ||
interpreter = self.context.products.get_data(PythonInterpreter) | ||
env['PEX_PYTHON'] = interpreter.binary |
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.
Te pexrc issue seems out of hand and this seems like a bandaid.
Is this more appropriate?
$ pex --help-variables
...
PEX_IGNORE_RCFILES: Boolean
Explicitly disable the reading/parsing of pexrc files (~/.pexrc).
Default: false.
...
It seems like we should be setting this when invoking any pex-based tool in pants, not just here.
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.
I thought that flag was on its way out: pex-tool/pex#673, but if it's okay to use it, then it looks like the thing we want to use.
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.
Yes, the flag, not the option. Flags for pex like --rcfile are only used at build time. options like PEX_IGNORE_RCFILES at runtime. For some options, specifying them at build time or run time both make sense. For this one, only run time makes sense.
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 for the clarification!
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.
The flag PEX_IGNORE_RCFILES
has not appeared to have an effect on the case I was looking at (and the case I was looking at didn't appear to be due to the use of a pexrc!). I was able to isolate a repro and convert it into the new test_succeeds_for_intersecting_unique_constraints()
, which this PR now fixes.
cadf7d2
to
471bbf3
Compare
471bbf3
to
75564dc
Compare
Refactored this after a while experimenting with the failure we were seeing internally, the result manages to fix the issue without introducing further breakage. It appears the issue was probably not due to a pexrc, and was instead a more general issue, which is now fixed. This should be ready for review again. Commits are reviewable independently. |
e3ac73e
to
c2f39ad
Compare
The CI failure is legit, I'm fixing that now. |
Ok, so I just increased the |
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.
I think you're still getting lucky here.
See my change to relase.sh here:
https://github.com/pantsbuild/pants/pull/7591/files#diff-9ed7102b7836807dc342cc2246ec4839R92:
pants/build-support/bin/release.sh
Lines 92 to 99 in a6f96ca
# We use `PEX_PYTHON_PATH="${PY}" "${PY}" "${pex}"` instead of either of: | |
# 1. PEX_PYTHON_PATH="${PY}" "${pex}" | |
# 2. "${PY}" "${pex}" | |
# This works around Pex re-exec-ing when it need not and subsequently losing constraints when | |
# it need not. The fixes for these are tracked in: | |
# 1. https://github.com/pantsbuild/pex/issues/709 | |
# 2. https://github.com/pantsbuild/pex/issues/710 | |
PEX_PYTHON_PATH="${PY}" "${PY}" "${pex}" "$@" |
That uses PEX_PYTHON_PATH=[python] [python] [pex] ...
to execute [pex]
ensuring the given [python]
interpreter is used. That workaround PR calls out the related pex issues of:
- Pex should not re-exec when the current interpreter satifies constraints pex-tool/pex#709
- Pex should not lose PEX_PYTHON or PEX_PYTHON_PATH when re-exec-ing pex-tool/pex#710
NB: That change does not deal with the possibility of pexrc. However, explicitly setting PEX_PYTHON_PATH overrides any pexrc setting of it. If you have pexrc files that specify both PEX_PYTHON and PEX_PYTHON_PATH, you really should PEX_IGNORE_RCFILES=1 instead of setting both PEX_PYTHON and PEX_PYTHON_PATH explicitly. To me at least - its more clear to say roughly:
- run the pex in a hermeticized pex environment (PEX_IGNORE_RCFILES=1)
- but mix in our setting for PEX_PYTHON_PATH
NB#2: All the work in this PR is regrettably whac-a-mole. It seems to me it would be a good idea to file an issue to absract out a ~PexExecutor
that all of pants can use to hermetically execute a pex using a selected interpreter just like the bits of changed code in this PR eventually will.
9d294f8
to
5a461db
Compare
5a461db
to
9a68c3c
Compare
Ok, the two pex issues you linked seem to match this failure perfectly and the larger context is clear.
Well, in this one I could at least move the functionality into a separate
I believe the pexrc config is not affecting the failure here, or at least, setting
I do. Unfortunately setting I think regardless of the eventual fix, generalizing it into a |
I encourage you to fight your better nature here and maintain scope. It's not just for you, it's for reviewers. It's nice for PRs to close out ~promptly as the norm. I'd just file an issue. |
…rpreter file path" This reverts commit 9a68c3c.
@jsirois I've broken out abstracting hermetic pex execution into #7640, and while I haven't determined why using |
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.
Would clarify and document the argument to create_pex
.
} | ||
self.context.log.debug('unique_constraints:\n{}'.format(unique_constraints)) | ||
|
||
if pin_selected_interpreter: |
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.
This flag seems to disable the unique_constraints
codepath entirely. should those be computed under that branch of the if instead?
It seems like rather than "pinning", this is more like: "use_global_python_interpreter" or something, because what it pins to is specifically the PythonInterpreter
product. It should definitely have a mention in the docstring.
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.
I've removed the extra argument to create_pex()
in favor of removing the unique_constraints
code path entirely, as every consumer of this method is always going to depend on requirements pexes resolved specifically for the single global selected interpreter. I've also added a docstring clarifying all of this.
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.
As noted in #7563 (comment), I've added back the pin_selected_interpreter
argument, but added a descriptive docstring. I'll be following up with a change that removes this.
1f56f37
to
dfef85c
Compare
I'm going to revert the changes to |
…create_pex()" This reverts commit 15195d3.
src/python/pants/backend/python/tasks/python_execution_task_base.py
Outdated
Show resolved
Hide resolved
36d15cd
to
ac20482
Compare
### Problem The problems observed in #7775 and #7563 had a third (and hopefully final: I'll audit before we land this) buddy in the `PythonRun` case. The tests in https://github.com/pantsbuild/pants/blob/817f7f6aedad8143fe7b2cf86559019254230da4/tests/python/pants_test/backend/python/tasks/test_python_run_integration.py#L134-L184 attempt to cover this case, but they were not using mixed contexts (contexts containing `(2-or-3, 3-only)` and `(2-only, 2-or-3)` constraints). ### Solution Similar to #7775 and #7563, do not include transitive constraints when `./pants run`ing a binary. Clarify the documentation of `PythonExecutionTaskBase`. Expand coverage of the mixed context case in existing tests. ### Result The tests linked above fail before this change for `PythonRun`, and succeed afterward.
The problems observed in #7775 and #7563 had a third (and hopefully final: I'll audit before we land this) buddy in the `PythonRun` case. The tests in https://github.com/pantsbuild/pants/blob/817f7f6aedad8143fe7b2cf86559019254230da4/tests/python/pants_test/backend/python/tasks/test_python_run_integration.py#L134-L184 attempt to cover this case, but they were not using mixed contexts (contexts containing `(2-or-3, 3-only)` and `(2-only, 2-or-3)` constraints). Similar to #7775 and #7563, do not include transitive constraints when `./pants run`ing a binary. Clarify the documentation of `PythonExecutionTaskBase`. Expand coverage of the mixed context case in existing tests. The tests linked above fail before this change for `PythonRun`, and succeed afterward.
Problem
PytestPrep
will generate a pex merging python sources and 3rdparty requirements. This pex will have all of the unique python interpreter constraints from all targets applied to the pex before being built. When executing that pex, it appears that the wrong python interpreter may be selected when the interpreter constraint set is intersecting -- see the added testtest_succeeds_for_intersecting_unique_constraints()
. This ends up looking like:Solution
PEX_PYTHON
andPEX_PYTHON_PATH
.['CPython>=2.7,<4', 'CPython>=3.6']
).Result
Running python tests targets with overlapping interpreter constraints should work!