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

Remove unit test runtime dependencies on resources #8066

Merged
merged 3 commits into from Jul 18, 2019

Conversation

@Eric-Arellano
Copy link
Contributor

commented Jul 17, 2019

Problem

Currently, test_base.py, jvm_tool_task_test_base.py, and interpreter_cache_test_mixin.py have implicit runtime dependencies on files defined at the build root, specifically ./pants, ./BUILD.tools, 3rdparty/BUILD, and .pants.d.

Because V2 is completely hermetic, these runtime dependencies fail to resolve as the test runner does not know to copy the files into the snapshot. Instead, we need some way to explicitly mark these dependencies via the BUILD file or to remove the dependency outright.

FYI: why didn't we detect this earlier?

We've been using V2 to run unit tests in CI for the past two months. Why didn't we detect this? The reason is that V2 runs locally in .pantsd, and the logic to get the buildroot traverses ancestors.

This was only detected when running unit tests with remote execution.

Solution

  • Remove the dependency on ./pants, which was not actually needed.
  • Explicitly depend on ./BUILD.tools and 3rdparty/BUILD.
  • Remove interpreter_cache_test_mixin.py, which is inherently non-hermetic and cannot work with the V2 model. Even though this has a slight performance impact, this is offset by the remote caching we will now be able to get + having more sound / hermitic tests.
Show resolved Hide resolved BUILD Outdated

@Eric-Arellano Eric-Arellano changed the title WIP: Allow BUILD files to depend on buildroot targets Remove tests' implicit runtime dependency on `BUILD.tools` and `3rdparty/BUILD` Jul 17, 2019

@Eric-Arellano Eric-Arellano changed the title Remove tests' implicit runtime dependency on `BUILD.tools` and `3rdparty/BUILD` Remove unit test implicit runtime dependency on `BUILD.tools` and `3rdparty/BUILD` Jul 17, 2019

@Eric-Arellano Eric-Arellano changed the title Remove unit test implicit runtime dependency on `BUILD.tools` and `3rdparty/BUILD` Remove unit test runtime dependency on `BUILD.tools` and `3rdparty/BUILD` Jul 17, 2019

@stuhood
Copy link
Member

left a comment

Fine with this as soon as things are green!

Show resolved Hide resolved tests/python/pants_test/test_base.py
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

I don't know how to handle this case:

class InterpreterCacheTestMixin:
"""A mixin to allow tests to use the "real" interpreter cache.
This is so each test doesn't waste huge amounts of time recreating the cache on each run.
Note: Must be mixed in to a subclass of BaseTest.
"""
def setUp(self):
super().setUp()
# It would be nice to get the location of the real interpreter cache from PythonSetup,
# but unfortunately real subsystems aren't available here (for example, we have no access
# to the enclosing pants instance's options), so we have to hard-code it.
python_setup_workdir = os.path.join(self.real_build_root, '.pants.d', 'python-setup')
self.set_options_for_scope('python-setup',
interpreter_cache_dir=os.path.join(python_setup_workdir, 'interpreters'),
chroot_cache_dir=os.path.join(python_setup_workdir, 'chroots'))

This mixin is used for all of the Python tasks tests.

Inherently, .pants.d is non-hermetic and this entire mixin clashes with the model of V2. This seems like something we need to completely rethink.

On this note, does it even make sense to test the V1 Python pipeline using the V2 Python test runner? I can get this mixin working again by inlining self.real_build_root, it just won't work with V2 remoting.

cc @illicitonion @stuhood

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

This seems like something we need to completely rethink.

On this note, does it even make sense to test the V1 Python pipeline using the V2 Python test runner?

Researched this some more. The above code is not only non-hermitic, but it also mutates the real world / users' actual .pants.d folder!

Rather than keeping the code and never running with V2, I recommend we keep using V2 and simply remove the mixin. Because of remote caching (and soon local caching), in most cases we won't even need to re-run the Pytest invocation, so this becomes a non-issue in most cases. The times we do have to run the test, we'll have to accept the cost of slower performance so that we can use V2 and remoting for this all (i.e. all the Python task tests) + have more sound / hermitic tests.

This mixin is solely test util code, so it is safe to remove minus the performance concerns addressed above.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:allow-buildroot-dependencies branch from ae12280 to 734656e Jul 18, 2019

@Eric-Arellano Eric-Arellano changed the title Remove unit test runtime dependency on `BUILD.tools` and `3rdparty/BUILD` Remove unit test runtime dependencies on resources Jul 18, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Pushed the change to remove interpreter_cache_test_mixin. Please feel free to merge this PR tomorrow if it goes green since I'll be afk.

@blorente
Copy link
Contributor

left a comment

I would love some numbers on the perf degradation that comes from removing the mixin, but as long as we keel the warning in the commit message I'm okay with that.

@@ -0,0 +1,12 @@
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

This comment has been minimized.

Copy link
@blorente

blorente Jul 18, 2019

Contributor

Will this file be inadvertently end up being the implicit owner of any unowned source file in the root directory? Asking because in the bazel world ownership of directories is influenced by the presence of BUILD files, and I'm not sure if that is also the case in Pants.

Not sure that is a problem, just something to note in a comment maybe.

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

This seems like something we need to completely rethink.
On this note, does it even make sense to test the V1 Python pipeline using the V2 Python test runner?

Researched this some more. The above code is not only non-hermitic, but it also mutates the real world / users' actual .pants.d folder!

Rather than keeping the code and never running with V2, I recommend we keep using V2 and simply remove the mixin. Because of remote caching (and soon local caching), in most cases we won't even need to re-run the Pytest invocation, so this becomes a non-issue in most cases. The times we do have to run the test, we'll have to accept the cost of slower performance so that we can use V2 and remoting for this all (i.e. all the Python task tests) + have more sound / hermitic tests.

This mixin is solely test util code, so it is safe to remove minus the performance concerns addressed above.

Agreed. Thanks!

@stuhood
Copy link
Member

left a comment

Thanks!

@stuhood stuhood merged commit da57bce into pantsbuild:master Jul 18, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:allow-buildroot-dependencies branch Jul 19, 2019

Eric-Arellano added a commit that referenced this pull request Aug 13, 2019

Recognize multiple sentinel files for determining the build root (#8105)
## Problem
We cannot have implicit runtime dependencies on files in the buildroot - they must all be declared explicitly via `BUILD` entries. #8066 fixed several of these issues, but not all. 

However, we cannot explicitly depend on the file `./pants` because it breaks V1 test execution. When running tests with V1, the folder in `.pants.d` will strip the prefix so `src/python/pants` -> `pants`. You cannot both have a directory named `pants` and a file named `pants`, so this causes most V1 tests to fail.

## Solution
Use multiple sentinel files to establish the buildroot, such as `BUILD_ROOT`. This allows us to safely depend on a sentinel file with very few code changes.
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.