Allow concurrent runs of pants on the same instance of pantsd #11639

stuhood · 2021-03-05T03:02:07Z

Problem

Concurrent execution of multiple runs of pants against the same instance of pantsd were disabled when forking was remove from pantsd. But the v2 engine isolates state and avoids global mutable variables such that concurrent runs could be unblocked by fixing the remaining global stateful cases:

Replacing usage of UNIX signals for cancellation with socket-level cancellation (Cancel sessions rather than killing pantsd #11223, Interrupt Graph tasks on Ctrl+C #11322, Ignore exit on the nailgun Client output_stream. #11358, Update to nails 0.11.0 #11370, Explicitly flush stdio in the nailgun client #11383, Move to having the Scheduler directly handle SIGINT to cancel Sessions #11399)
Removing mutable initialization of Subsystems (Deprecate Subsystems as global singletons #11388, Remove deprecated v1 Subsystem facilities #11424)
Making stdio thread-local (Thread-local stdio handling to support concurrent pantsd clients #11536, Resolve plugins via PEX rules in a bootstrap scheduler #11568)
Explicitly passing environment variables (Require explicit environment usage #11641)

Solution

Now that all global stateful cases are removed, we can remove the lock that prevents concurrent runs in pantsd, and deprecate the --concurrent flag.

Result

Multiple runs of pants can proceed concurrently with the same instance of pantsd in a single workspace. See https://asciinema.org/a/397032 for an example.

Fixes #7654.

src/python/pants/testutil/rule_runner.py

src/python/pants/backend/python/util_rules/pex_test.py

src/python/pants/testutil/rule_runner.py

[ci skip-rust] [ci skip-build-wheels]

…ronment or a new filtered Environment type. [ci skip-build-wheels] [ci skip-rust]

[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

stuhood · 2021-03-06T01:43:15Z

Only the top commit is relevant: the rest is #11641.

jsirois

Nice.

Eric-Arellano

Epic!

Eric-Arellano · 2021-03-06T05:35:59Z

build-support/bin/release.sh

@@ -94,7 +94,7 @@ function run_pex() {
 function run_packages_script() {
  (
    cd "${ROOT}"
-    ./pants --concurrent run "${ROOT}/build-support/bin/packages.py" -- "$@"
+    ./pants --no-pantsd run "${ROOT}/build-support/bin/packages.py" -- "$@"


My thinking was that in at least some of these cases the --concurrent flag was actually meant-as "please disable pantsd". This one didn't have a comment to explain why it was being used: since this is a bash script, it didn't seem likely that this script was "running under" pants.

Eric-Arellano · 2021-03-06T05:54:32Z

This might be related to #11626, but, I ran ./pants test src/python/pants/base: at the same time in two different tabs. Both correctly rendered the console test summary, but only one showed the std{out,err} of the failed exception_sink_integration_test.py. What would you expect? The EnrichedTestResult rule is uncacheable, but I'm not sure how that plays out with concurrent runs.

To reproduce, probably useful to do a slower test (or add time.sleep()) so it doesn't complete before starting the other run.

stuhood · 2021-03-07T23:02:01Z

This might be related to #11626, but, I ran ./pants test src/python/pants/base: at the same time in two different tabs. Both correctly rendered the console test summary, but only one showed the std{out,err} of the failed exception_sink_integration_test.py. What would you expect? The EnrichedTestResult rule is uncacheable, but I'm not sure how that plays out with concurrent runs.

Mmm, no: this is an oversight: thanks. I had planned to add a test or two before landing this, but I doubt that it would have caught that case.

I have a good idea of how to resolve it though (you're right that it relates to the behavior of uncacheable nodes): will do that as another prework PR.

### Problem In order to allow concurrent runs of Pants in #11639 (part of #7654), all global mutable singletons must be either 1) thread-local, 2) replaced with explicitly passed values. `os.environ` and `sys.argv` are two of the last cases we're concerned with. ### Solution Although we could hypothetically redefine `os.environ` and `os.getenv` as thread-local, they seem to be a better fit to be explicitly passed. So this change: 1. Renames (and moves) `PantsEnvironment` to `CompleteEnvironment` 2. Adds `Environment` and `EnvironmentRequest`, which filter the `CompleteEnvironment` to a smaller subset, and should be used more frequently by `@rules`. 3. Fixes a few cases of implicit `os.environ` usage in the `PythonSetup` subsystem by explicitly selecting interpreter-selection env vars (`HOME`/`PATH`/`PYENV_ROOT`) Finally, we empty the environment for `pantsd` runs to enforce explicit usage of `CompleteEnvironment`/`Environment`. ### Result The last (?) global mutable variable blocking #7654 is removed. [ci skip-build-wheels]

stuhood · 2021-06-17T18:32:03Z

See #7654 (comment)

stuhood commented Mar 5, 2021

View reviewed changes

src/python/pants/testutil/rule_runner.py Outdated Show resolved Hide resolved

Eric-Arellano reviewed Mar 5, 2021

View reviewed changes

src/python/pants/backend/python/util_rules/pex_test.py Outdated Show resolved Hide resolved

src/python/pants/testutil/rule_runner.py Outdated Show resolved Hide resolved

stuhood added 3 commits March 5, 2021 16:36

Rename PantsEnvironment to CompleteEnvironment, and move to engine.

f37db3c

[ci skip-rust] [ci skip-build-wheels]

Clear the environment in daemon runs, and explicitly use CompleteEnvi…

478068c

…ronment or a new filtered Environment type. [ci skip-build-wheels] [ci skip-rust]

Explicitly pass the relevant env for InteractiveProcess.

130b280

[ci skip-build-wheels]

stuhood mentioned this pull request Mar 6, 2021

Require explicit environment usage #11641

Merged

Remove locking to allow concurrent runs.

288263c

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

stuhood force-pushed the stuhood/concurrent-pantsd-runs branch from 10fc4e2 to 288263c Compare March 6, 2021 01:09

stuhood requested review from Eric-Arellano, tdyas, jsirois and benjyw and removed request for tdyas March 6, 2021 01:42

stuhood marked this pull request as ready for review March 6, 2021 01:42

jsirois approved these changes Mar 6, 2021

View reviewed changes

Eric-Arellano approved these changes Mar 6, 2021

View reviewed changes

stuhood marked this pull request as draft March 9, 2021 00:32

Base automatically changed from master to main March 19, 2021 19:20

stuhood closed this Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow concurrent runs of pants on the same instance of pantsd #11639

Allow concurrent runs of pants on the same instance of pantsd #11639

stuhood commented Mar 5, 2021 •

edited

Loading

stuhood commented Mar 6, 2021

jsirois left a comment

Eric-Arellano left a comment

Eric-Arellano Mar 6, 2021

stuhood Mar 7, 2021

Eric-Arellano commented Mar 6, 2021

stuhood commented Mar 7, 2021

stuhood commented Jun 17, 2021

Allow concurrent runs of pants on the same instance of pantsd #11639

Allow concurrent runs of pants on the same instance of pantsd #11639

Conversation

stuhood commented Mar 5, 2021 • edited Loading

Problem

Solution

Result

stuhood commented Mar 6, 2021

jsirois left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Mar 6, 2021

Choose a reason for hiding this comment

stuhood Mar 7, 2021

Choose a reason for hiding this comment

Eric-Arellano commented Mar 6, 2021

stuhood commented Mar 7, 2021

stuhood commented Jun 17, 2021

stuhood commented Mar 5, 2021 •

edited

Loading