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.

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

Merged

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 15, 2019

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 test test_succeeds_for_intersecting_unique_constraints(). This ends up looking like:

Failed to execute PEX file, missing macosx_10_14_x86_64-cp-27-cp27m compatible dependencies for:
more-itertools
pytest
funcsigs
pytest-cov
coverage

Solution

  • Force the selected interpreter used to create the pytest pex to be used when running it by setting PEX_PYTHON and PEX_PYTHON_PATH.
  • Add testing for the case of targets with intersecting interpreter constraints (such as ['CPython>=2.7,<4', 'CPython>=3.6']).

Result

Running python tests targets with overlapping interpreter constraints should work!

@blorente
Copy link
Contributor

left a comment

This is great, thanks!

@wisechengyi
Copy link
Contributor

left a comment

Thanks! would be nice to have some tests if they are not too expensive.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

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

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 15, 2019

Member

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.

This comment has been minimized.

Copy link
@blorente

blorente Apr 16, 2019

Contributor

I thought that flag was on its way out: pantsbuild/pex#673, but if it's okay to use it, then it looks like the thing we want to use.

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 22, 2019

Member

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.

This comment has been minimized.

Copy link
@blorente

blorente Apr 23, 2019

Contributor

Thanks for the clarification!

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 28, 2019

Author Contributor

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.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pytest-as-python-tool branch 2 times, most recently from cadf7d2 to 471bbf3 Apr 27, 2019

@cosmicexplorer cosmicexplorer changed the title fix the PEX_PYTHON of the pytest pex when run to avoid pexrc clashing fix the PEX_PYTHON of the pytest pex when run to avoid using incompatible pytest requirements Apr 28, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pytest-as-python-tool branch from 471bbf3 to 75564dc Apr 28, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

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.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pytest-as-python-tool branch 2 times, most recently from e3ac73e to c2f39ad Apr 28, 2019

@cosmicexplorer cosmicexplorer changed the title fix the PEX_PYTHON of the pytest pex when run to avoid using incompatible pytest requirements pin the PEX_PYTHON{,_PATH} running the pytest pex to avoid using incompatible pytest requirements Apr 28, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

The CI failure is legit, I'm fixing that now.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Ok, so I just increased the --timeout-default argument for the one failing PytestRun integration test, since the previous failure actually did seem to be flakiness (and it had happened twice in a row). It's suspicious that this change would seem to cause that specific flakiness, and we will be doing performance testing of this change internally to avoid introducing a regression.

@jsirois
Copy link
Member

left a comment

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:

# 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:

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.

@stuhood stuhood added this to the 1.16.x milestone Apr 29, 2019

cosmicexplorer added some commits Apr 27, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pytest-as-python-tool branch from 9d294f8 to 5a461db Apr 30, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pytest-as-python-tool branch from 5a461db to 9a68c3c Apr 30, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Ok, the two pex issues you linked seem to match this failure perfectly and the larger context is clear.

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.

Well, in this one I could at least move the functionality into a separate PexExecutor class so other tasks can pick it up when they need it.

NB: That change does not deal with the possibility of pexrc.

I believe the pexrc config is not affecting the failure here, or at least, setting PEX_IGNORE_RCFILES and using the python interpreter in the argv does not work in the way that setting PEX_PYTHON_PATH and PEX_PYTHON does. On my laptop the changes in 5a461db (setting just PEX_PYTHON_PATH and explicitly selecting the interpter) caused the pytest invocation to fail before tests were run:

============== test session starts ===============
platform darwin -- Python 3.6.4, pytest-3.6.4, py-1.8.0, pluggy-0.7.1 -- /Users/dmcclanahan/tools/pants/build-support/pants_dev_deps.py36.venv/bin/python3
cachedir: .pytest_cache
rootdir: /Users/dmcclanahan/tools/pants, inifile: /Users/dmcclanahan/tools/pants/.pants.d/test/pytest-prep/CPython-3.6.4/667aa724ef06c0a4cda5a996a39591b15fbcde5a/pytest.ini
plugins: cov-2.4.0, timeout-1.2.1
collecting ...
- generated xml file: /Users/dmcclanahan/tools/pants/.pants.d/test/pytest/tests.python.pants_test.backend.python.tasks.pytest_run/junitxml/TEST-tests.python.pants_test.backend.python.tasks.pytest_run.xml -
========== no tests ran in 0.04 seconds ==========
ERROR: not found: /Users/dmcclanahan/tools/pants/build-support/pants_dev_deps.py36.venv/bin/python3
(no name '/Users/dmcclanahan/tools/pants/build-support/pants_dev_deps.py36.venv/bin/python3' in any of [])


                   tests/python/pants_test/backend/python/tasks:pytest_run    .....   NOT RUN

If you have pexrc files that specify both PEX_PYTHON and PEX_PYTHON_PATH

I do. Unfortunately setting PEXRC_IGNORE_FILES=1 in 5a461db failed to run tests, while explicitly setting both PEX_PYTHON_PATH and PEX_PYTHON successfully ran tests.

I think regardless of the eventual fix, generalizing it into a PexExecutor to pin the interpreter seems like a good idea.

@jsirois

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Well, in this one I could at least move the functionality into a separate PexExecutor class so other tasks can pick it up when they need it.

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.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@jsirois I've broken out abstracting hermetic pex execution into #7640, and while I haven't determined why using PEX_PYTHON_PATH=... /path/to/python /path/to/pex.pex args... (attempted in 9a68c3c) is failing, the rationale for instead having to set both PEX_PYTHON and PEX_PYTHON_PATH is described in the docstring for _constrain_pytest_interpreter_search_path(). The added test demonstrates a case which is currently broken that is now fixed, and this diff fixes the same breakage we were seeing internally. Is it reasonable to try to merge this PR now, or am I missing anything?

@stuhood
Copy link
Member

left a comment

Would clarify and document the argument to create_pex.

}
self.context.log.debug('unique_constraints:\n{}'.format(unique_constraints))

if pin_selected_interpreter:

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 30, 2019

Member

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.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 30, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 1, 2019

Author Contributor

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.

@jsirois

This comment has been minimized.

This is not a PR blocker, but investigating the commit you pointed to as your PEX_IGNORE_RCFILES=1 attempt, why did you set PATH? For an invocation like PEX_PYTHON_PATH=<python binary (1)> <python binary (2)> <pex> ... no PATH is needed to find a python interpreter since by this construction the sys.executable (2) matches PEX_PYTHON_PATH (1) short-circuiting any PATH lookup. In addition, since the executable in this construction is python and not pex, the pex shebang which may have something like #!/usr/bin/env python3.6 will no be run avoiding the other possible PATH lookup.

This comment has been minimized.

Copy link
Contributor Author

replied Apr 30, 2019

It didn't work differently at all by adding the PATH variable. I had added that out of the vague concern that something could be happening like in #7615 where python or pex was looking for an executable of some specific name, so we had to ensure the temporary rustup-init file was named rustup-init. You've confirmed that there isn't a PATH lookup we're missing here, so this part should have been removed -- I wanted to see if travis would treat this any differently.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

I'm going to revert the changes to create_pex() which are causing the one CI failure and break it out into a followup PR.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pytest-as-python-tool branch from 36d15cd to ac20482 May 1, 2019

@cosmicexplorer cosmicexplorer merged commit 823ae84 into pantsbuild:master May 1, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.