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

Properly handle source roots and resources with V2 Pytest runner #8063

Merged
merged 10 commits into from Jul 24, 2019

Conversation

@Eric-Arellano
Copy link
Contributor

commented Jul 17, 2019

Problem

While #7696 added support for source roots, its implementation of stripping the actual prefix from the file broke use of resources, specifically when referring to their path in source code, e.g. calling open('tests/python/pants_test/backend/jvm/tasks/ivy_utils.py'). This is because we physically stripped the name, so the hardcoded file path would need to be updated to 'pants_test/backend/jvm/tasks/ivy_utils.py'. Per #8060 (comment), this is not acceptable because it changes a public API.

We must have a way to relativize Python files so that their import statements work, but to also keep the full path.

Solution

By populating the PYTHONPATH env var for the Pex that runs Pytest with the unique source root paths, Python absolute imports work without actually having to strip any prefix. This appears similar to what Pycharm and IntelliJ do.

Alternative attempted: use PEX's --source-directory

PEX is able to understand source roots via its --source-directory and --resource-directory args. The idea was to create two pexes: a requirements PEX and a sources PEX, then to merge the two via PEX_PATH. They would stay separate to increase the likelihood of cache hits.

This solution failed, however, due to Pytest failing to discover any tests. See #8063 (comment) and the comment below it.

Alternative attempted: only strip PythonTargets

The first idea was to only strip any subclass of PythonTarget and keep the full path for other target types like files (2b68ebd). This failed quickly, though, because the VERSION file is not able to correctly imported in this case.

Result

Two tests can be removed from the V2 blacklist without any changes to them required.

@stuhood
Copy link
Member

left a comment

This is great! Thanks.

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

As discussed offline: rather than setting the PYTHONPATH, this should be using the relevant pex CLI args to include each of these directories as pythonpath entries.

@Eric-Arellano Eric-Arellano changed the title Add support for resources to V2 Pytest runner Properly handle source roots and resources with V2 Pytest runner Jul 18, 2019

@Eric-Arellano Eric-Arellano changed the title Properly handle source roots and resources with V2 Pytest runner WIP: Properly handle source roots and resources with V2 Pytest runner Jul 18, 2019

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:source-root-resources branch from 8136976 to 2f23d82 Jul 20, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

I tried to address this via the PEX approach:

  1. Create the requirements PEX with entrypoint pytest:main, as before.
  2. Create a sources PEX with all sources and resources relevant.
  3. Merge the two via PEX_PATH. Note that this only merges at runtime - it does not actually create one single PEX.

This does not work, however, because pytest:main expects to be passed the files it should run tests against, otherwise it will default to its standard test discovery. This results in the following:

./pants --no-v1 --v2 test tests/python/pants_test/util:strutil

02:55:47 [INFO] Starting tests: tests/python/pants_test/util:strutil
02:55:51 [INFO] Tests failed: tests/python/pants_test/util:strutil
tests/python/pants_test/util:strutil stdout:
============================= test session starts ==============================
platform darwin -- Python 3.6.8, pytest-3.6.4, py-1.8.0, pluggy-0.7.1
rootdir: /Users/eric/DocsLocal/code/projects/pants/.pants.d/process-executionxZnVFf, inifile:
plugins: cov-2.4.0, timeout-1.2.1
collected 0 items

========================= no tests ran in 0.00 seconds =========================


tests/python/pants_test/util:strutil                                            .....   FAILURE
Tests failed

See https://docs.pytest.org/en/latest/goodpractices.html#conventions-for-python-test-discovery for how Pytest does test discovery.

We could I think get things working if we provided the config value testpaths with all of the unique source roots, but there's no CLI arg for this and it would require creating a temporary pytest config file. Maybe that's okay?

It looks like people usually solve this problem by indeed modifying PYTHONPATH: https://stackoverflow.com/a/50156706 and https://stackoverflow.com/a/51522899.

--

I had earlier wanted to use the merged PEX approach so that ./pants binary is easier. However, I realized as highlighted above that we cannot do this - you cannot merge two built PEXes, beyond at runtime. So, with ./pants binary we will have to pass both the sources and requirements in the same build call. This means that the above flow would not actually help with ./pants binary, even if we could get it working for Pytest.

(Instead, to get ./pants binary working, I think we'll extend create_requirements_pex.py / aka resolve_requirements.py to be create_pex.py and allow it to take source files.)

--

I'm off to see Lion King (😀) but tomorrow can test if testpaths / the config file will get things working. If not, I recommend we go back to approach #2 of PYTHONPATH: 77413b8

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

I'm off to see Lion King (😀) but tomorrow can test if testpaths / the config file will get things working.

Couldn't get this working. Still errors that no test was found.

I also realized in the process that I think we are passing arguments to Pytest? We pass a merged digest of the requests PEX, sources PEX, and now pytest.ini. If I understand correctly, it will view the sources.pex and pytest.ini files as inputs? This doesn't work properly - Pytest needs source files and does not understand .pex files.

I'm going to go back to the simpler PYTHONPATH solution. It works, less code, and as described above, the sources PEX wouldn't even be helpful in porting ./pants binary, which will need to combine requirements and sources at the beginning at build time because you cannot merge two pexes.

Eric-Arellano added a commit that referenced this pull request Jul 20, 2019

Refactor V2 PEX creation rules (#8080)
#8063 was originally going to result in creating a `SourcesPex` (in addition to `RequirementsPex`). In preparation for that, the below refactors were made. Even though the approach to #8063 has changed, these are good changes to make in general and may be useful to porting `./pants binary`.

* Extract out `download_pex_bin` rule.
* Rename `resolve_requirements.py` -> `create_requirements_pex.py` for more explicit name (and avoid name clash with V1 file).
* Fix shadowing variable name.
* Use long CLI args.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:source-root-resources branch from 79c1ca9 to ff5e52a Jul 20, 2019

@Eric-Arellano Eric-Arellano changed the title WIP: Properly handle source roots and resources with V2 Pytest runner Properly handle source roots and resources with V2 Pytest runner Jul 20, 2019

@Eric-Arellano Eric-Arellano requested a review from illicitonion Jul 20, 2019

@Eric-Arellano Eric-Arellano requested a review from jsirois Jul 20, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

d66b77f is an unfortunate side effect of this change. As a result, those tests won't pass with V1 anymore and only V2. Regardless of which of the 3 approaches we took, I think this change would be necessary to get the V2 tests working.

Fortunately, it only appears to change the displayed output for some engine code - it does not actually require people to change their source code, unlike the motivating problem for this PR that our current handling of source roots would require changing source code. So, this seems the lesser of two evils.

@stuhood
Copy link
Member

left a comment

The module name changes are potentially problematic. It's not clear to me why we shouldn't be detecting the correct name of the module, so updating the tests to work around that is a bit odd.

If @jsirois is available, it would be great to get his opinion on how to resolve this before landing.

@@ -209,7 +209,7 @@ def test_thirdparty_dep(self):
plugins: SOME_TEXT
collected 1 item
pants/dummies/test_with_thirdparty_dep.py . [100%]
testprojects/tests/python/pants/dummies/test_with_thirdparty_dep.py . [100%]

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

This is a step forward. Thanks.

@@ -119,16 +119,16 @@ def test_include_trace_error_raises_error_with_trace(self):

self.assert_equal_with_printing(dedent('''
1 Exception encountered:
Computing Select(<pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, A)
Computing Task(nested_raise(), <pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, A, true)
Computing Select(<tests.python.pants_test.engine.test_engine.B object at 0xEEEEEEEEE>, A)

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

This is a potentially problematic step backward. It means that we're not correctly detecting the name of the module relative to the sourceroot (although only for tests, perhaps? that's a bit odd). I suspect that it would mean that imports would break in some situations.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 24, 2019

Author Contributor

I suspect that it would mean that imports would break in some situations.

We support both types of imports now: tests.python.pants_test.engine.scheduler_test_base and pants_test.engine.scheduler_test_base, whereas V1 only supports the latter.

It means that we're not correctly detecting the name of the module relative to the sourceroot (although only for tests, perhaps? that's a bit odd).

Sort of. It means that Python uses the full module name tests.python.pants_test.engine.scheduler_test_base, and we still allow using the source-root stripped version pants_test.engine.scheduler_test_base. Both are valid ways to refer to the Python modules; the former is more just more explicit, so Python uses it when printing things like repr.

Regardless, don't see a way to do this without stripping the prefix from files. I'm still unclear how V1 did this all, even after reading the V1 Pytest code dozens of times the past year :/

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 24, 2019

Member

The answer is probably related to the pytest module that (I had forgotten that) we install: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/tasks/pytest/plugin.py ...

This comment has been minimized.

Copy link
@jsirois

jsirois Aug 5, 2019

Member

Regardless, don't see a way to do this without stripping the prefix from files. I'm still unclear how V1 did this all, even after reading the V1 Pytest code dozens of times the past year :/

See here:

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 18, 2019

Author Contributor

I figured out the line that I was missing for the life of me! https://github.com/pantsbuild/pants/pull/8180/files#diff-5f2c007abfa9fbf0323826392a6374c7R101

And, as a result, I think I understand a bit better now how this plugin comes into play.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

If jsirois is available, it would be great to get his opinion on how to resolve this before landing.

Talked to Asher that John is out until August 1.

@stuhood
Copy link
Member

left a comment

I think that given that you didn't need to change any pkg_resources calls here (and assuming that there are some in these tests), this seems kosher. Sorry for the fud.

Thanks!

@stuhood stuhood merged commit 4854194 into pantsbuild:master Jul 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:source-root-resources branch Jul 24, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jul 26, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jul 27, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jul 27, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jul 27, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Jul 27, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Aug 11, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.