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

Use remote build execution to run several unit tests #8051

Merged
merged 22 commits into from Jul 22, 2019

Conversation

@Eric-Arellano
Copy link
Contributor

commented Jul 15, 2019

Per #7649, we have been working to run Pants unit tests via Google Remote Build Execution (RBE) for higher parallelism (and thus faster CI results).

This hooks up to the secure token generator created at https://github.com/pantsbuild/rbe-token-server, per the design doc at https://docs.google.com/document/d/1gL3D1f-AzL_LzRxWLskCpVQ2ZlB_26GTETgXkXsrpDY/edit#.

It introduces a new flag to ci.py --remote-execution-enabled that will call the token generator in CI and will use the gcloud CLI when ran locally.

We must blacklist 5% of the targets, which will be addressed in followup PRs.

As a result, unit test CI goes from 40 minutes to 15 minutes.

Will close #7649.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:rbe-token-server branch from 176af50 to 1098d88 Jul 15, 2019

Show resolved Hide resolved build-support/bin/ci.py Outdated
Show resolved Hide resolved .travis.yml Outdated

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:rbe-token-server branch 3 times, most recently from 0e190f2 to dfb2102 Jul 16, 2019

@Eric-Arellano Eric-Arellano changed the title WIP: Use remote build execution to run several unit tests Use remote build execution to run several unit tests Jul 16, 2019

@Eric-Arellano Eric-Arellano marked this pull request as ready for review Jul 16, 2019

@hrfuller
Copy link
Contributor

left a comment

Looks good!

Curious about the usage of a pex instead of just importing a module into the ci script.

Show resolved Hide resolved build-support/bin/ci.py
Show resolved Hide resolved build-support/bin/ci.py Outdated
Show resolved Hide resolved build-support/bin/BUILD
Show resolved Hide resolved build-support/bin/ci.py Outdated

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:rbe-token-server branch from dfb2102 to 2ea6e33 Jul 16, 2019

@Eric-Arellano
Copy link
Contributor Author

left a comment

Curious about the usage of a pex instead of just importing a module into the ci script.

I'm not sure what you mean by this @hrfuller?

Show resolved Hide resolved build-support/bin/BUILD
Show resolved Hide resolved build-support/bin/ci.py
Show resolved Hide resolved build-support/bin/ci.py Outdated
Show resolved Hide resolved build-support/bin/ci.py Outdated

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:rbe-token-server branch from 3f51806 to b5dffb0 Jul 16, 2019

@hrfuller

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

It seems like it would be easier to import a function from the get_rbe_token module into ci.py instead of building it into a python_binary and then calling it as a subprocess, thats all.

@stuhood
Copy link
Member

left a comment

As mentioned above: the blacklist for remoting probably needs to be a whitelist until the fundamental issues are figured out.

Depending on how long the fundamental issues might take to resolve, you could either invert from blacklist to whitelist here, or block this PR on fixing sdist building in the remote environment.

Show resolved Hide resolved build-support/bin/BUILD
Show resolved Hide resolved build-support/bin/ci.py
Show resolved Hide resolved build-support/bin/ci.py Outdated
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

It seems like it would be easier to import a function from the get_rbe_token module into ci.py instead of building it into a python_binary and then calling it as a subprocess, thats all.

Ah, I see what you mean @hrfuller. This is very intentional. We want ci.py to have no dependencies other than the std lib and common.py, so that we do not need to run via ./pants run and can directly invoke it as a Python script. This is non-negotiable because of a chicken-and-the-egg issue, that we use in CI this script to bootstrap ./pants.pex. Were we to call the script via ./pants run, then it would end up bootstrapping before we intended to.

--

As mentioned above: the blacklist for remoting probably needs to be a whitelist until the fundamental issues are figured out.

I will try to fix the sdist issue (#8057) today, and if I can't figure it out by tonight will convert this to a whitelist to ensure that we can land this before Thursday.

@illicitonion
Copy link
Contributor

left a comment

Looks great, thanks!

Show resolved Hide resolved src/docs/howto_develop.md Outdated

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:rbe-token-server branch from 15690af to c216894 Jul 19, 2019

Eric-Arellano added some commits Jul 3, 2019

Redesign API for enabling RBE in `ci.py`
It makes things simpler for the end caller to simply say they want remoting, and to let `ci.py` figure out how to do this. The rest of `ci.py` operates at a high level of abstraction, and this should too.

Eric-Arellano and others added some commits Jul 16, 2019

Simplify adding remote flags to ci.py
Co-Authored-By: Henry Fuller <hrofuller@gmail.com>
Update docs
# Delete this line to force a full CI run for documentation-only changes.
[ci skip]  # Documentation-only change.

# Delete this line to force a full CI run for documentation-only changes.
[ci skip]  # Documentation-only change.

# Delete this line to force a full CI run for documentation-only changes.
[ci skip]  # Documentation-only change.
Restore host PATH entry for interpreters
Fixes issue with speculation not being able to discover interpreters locally.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:rbe-token-server branch from c216894 to e03fdd5 Jul 20, 2019

Eric-Arellano added some commits Jul 20, 2019

Set remote LC_ALL and LANG vals
This reduces variation caused by the host platform. Values gotten by inspecting the Docker container.

@Eric-Arellano Eric-Arellano force-pushed the Eric-Arellano:rbe-token-server branch from 27f5b12 to 705fb17 Jul 21, 2019

@Eric-Arellano Eric-Arellano requested a review from stuhood Jul 21, 2019

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Now ready to be merged! Got the blacklist down to 14 targets (6%). The vast majority are due to lingering issues with buildroot, which I have a fix queued up for but is blocked by #8063.

I recommend we merge this first, then #8063, then the buildroot PR.

@stuhood
Copy link
Member

left a comment

Thanks!

I left a series of comments, but they're all nits. Merging, and can address in followups.

with open(known_v2_failures_file, "r") as f:
blacklisted_targets = {line.strip() for line in f.readlines()}
with travis_section("PythonTestsV1", "Running Python unit tests with V2 test runner"):
def run_python_tests_v2(*, remote_execution_enabled: bool) -> None:

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

I think that shrinking this code is pretty important. I've opened #8087 because dogfooding our own facilities during rollouts like this is useful.

@@ -23,15 +23,19 @@ remote_execution_extra_platform_properties: [
"container-image=docker://gcr.io/pants-remoting-beta/rbe-remote-execution@sha256:20046a8b909cbbd94f3ff33808ceeeaa489a05eea5d00ad57c378f81f21462fc",
]

# This should correspond to the number of workers running in Google RBE.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

Would be good to link to the relevant Google console page for inspecting this.


[python-setup]
interpreter_search_paths: [
# We include the host PATH and PEXRC values so that speculation still works.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

This whole block probably deserves a TODO pointing to #7735

@stuhood stuhood merged commit 54d0d4e into pantsbuild:master Jul 22, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:rbe-token-server branch Jul 24, 2019

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

@Eric-Arellano Eric-Arellano referenced this pull request Jul 26, 2019

Open

Remote execution of integration tests #8113

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