add "workspace" process execution strategy #20772

tdyas · 2024-04-09T22:07:48Z

Add a "workspace" process execution strategy and a process_execution::workspace::ComandRunner to execute processes in the workspace instead of the ordinary "local" execution sandbox.

This implements the Rust parts of Design B from the "In-Workspace Process Execution" design document which will add support to Pants for "workspace environments."

This PR supersedes #20740 since the approach of using the existing Process capture workflow fits better with the existing system design.

src/rust/engine/process_execution/src/workspace.rs

tdyas · 2024-04-09T22:14:12Z

src/rust/engine/src/context.rs

@@ -219,6 +220,22 @@ impl Core {
            exec_strategy_opts.local_keep_sandboxes,
        );

+        // TODO: Wrap this in a BoundedCommandRunner so only run process can execute in the workspace at once.


Thoughts on using BoundedCommandRunner to enforce this?

Are the sandboxes uniquely named such that they wouldn't collide?

For cases where we are invoking an existing build tool, we'd expect them to do the right thing with regard to locking so that they could safely run concurrently. But I wonder whether it would make it challenging to write correct end users scripts to have concurrency.

It sounds like mutability of declared outputs is hidden, because those go into the sandbox? So only undeclared outputs that didn't go into temporary directories would be a problem. And that is easy to fix... but definitely need to be documented.

I think it's fine to ship without limiting to 1... in future could add a concurrency limit as a workspace option perhaps?

tdyas · 2024-04-16T00:07:59Z

Did some refactoring and added tests. This should be ready for review now.

benjyw · 2024-05-02T10:13:46Z

Sorry @tdyas, this fell below the fold. I think @stuhood is best positioned to review, if he has time?

stuhood

Looks good to me! Thanks @tdyas : sorry for the long delayed review.

src/python/pants/engine/environment.py

src/python/pants/engine/process_test.py

src/rust/engine/process_execution/src/lib.rs

stuhood · 2024-05-18T16:36:20Z

src/rust/engine/src/context.rs

@@ -219,6 +220,22 @@ impl Core {
            exec_strategy_opts.local_keep_sandboxes,
        );

+        // TODO: Wrap this in a BoundedCommandRunner so only run process can execute in the workspace at once.


Are the sandboxes uniquely named such that they wouldn't collide?

For cases where we are invoking an existing build tool, we'd expect them to do the right thing with regard to locking so that they could safely run concurrently. But I wonder whether it would make it challenging to write correct end users scripts to have concurrency.

It sounds like mutability of declared outputs is hidden, because those go into the sandbox? So only undeclared outputs that didn't go into temporary directories would be a problem. And that is easy to fix... but definitely need to be documented.

I think it's fine to ship without limiting to 1... in future could add a concurrency limit as a workspace option perhaps?

stuhood · 2024-05-18T16:37:02Z

src/rust/engine/src/externs/process.rs

@@ -29,17 +29,22 @@ impl PyProcessExecutionEnvironment {
        platform: String,
        remote_execution: bool,
        remote_execution_extra_platform_properties: Vec<(String, String)>,
+        execute_in_workspace: bool,
        environment_name: Option<String>,
        docker_image: Option<String>,
    ) -> PyResult<Self> {


Oy. Need support for ADTs at this boundary. Oh well!

Oy. Need support for ADTs at this boundary. Oh well!

What's the preferred way to do that? A Python class per enum variant with them all in a Union?

Also, I will explore such a refactor after this PR has landed.

tdyas · 2024-05-20T02:24:39Z

Are the sandboxes uniquely named such that they wouldn't collide?

Yes, the code makes use of the same temporary directory support has the ordinary local executor and so makes uniquely-named temporary directories for the sandbox directories.

tdyas · 2024-05-20T02:27:54Z

I think it's fine to ship without limiting to 1... in future could add a concurrency limit as a workspace option perhaps?

Agreed. Seems too early to fix without affirmative proof it is a problem. And it can be a config option in the future. I'll update the comment accordingly.

tdyas · 2024-05-20T02:28:45Z

It sounds like mutability of declared outputs is hidden, because those go into the sandbox? So only undeclared outputs that didn't go into temporary directories would be a problem. And that is easy to fix... but definitely need to be documented.

@stuhood: What do you mean by "mutability of declared outputs is hidden"?

src/rust/engine/process_execution/src/fork_exec.rs

src/rust/engine/src/context.rs

Move the exclusive spawn logic out of `process_execution::local::CommandRunner` into a helper module in advance of using that logic in the forthcoming "workspace" command runner to be introduced by #20772. (This PR was extracted from #20772.)

tdyas · 2024-05-22T02:53:13Z

Rebased on top of main including the refactor in c8cdca0.

@benjyw: This version should be smaller diff size with the refactor.

tdyas · 2024-05-22T11:34:44Z

src/python/pants/engine/environment.py

+# Reserved sentinel value representing execution within the workspace and not local sandbox.
+LOCAL_WORKSPACE_ENV_NAME = "__local_workspace__"


This sentinel value was added so tests in this PR could make use of the workspace process execution strategy even without the next PR in the series (which adds the workspace_environment type). That next PR in the series will remove this particular use and rename it.

Maybe it should be renamed so the support in this PR is not in the public API (in case the next PR is delayed in landing)?

I prefixed the symbol with underscore to make it clear that the symbol is not public API, and added a comment to that effect.

benjyw

LG, thanks for pairing on this!

tdyas requested review from stuhood and benjyw April 9, 2024 22:07

tdyas changed the title ~~add "workspace" process execution straegy~~ add "workspace" process execution strategy Apr 9, 2024

tdyas mentioned this pull request Apr 9, 2024

add intrinsic rule for in-workspace process execution #20740

Closed

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from c6fd8d3 to beb5516 Compare April 9, 2024 22:09

tdyas added the category:new feature label Apr 9, 2024

tdyas commented Apr 9, 2024

View reviewed changes

src/rust/engine/process_execution/src/workspace.rs Outdated Show resolved Hide resolved

tdyas commented Apr 9, 2024

View reviewed changes

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from 51c55a4 to b911f9d Compare April 13, 2024 20:55

tdyas marked this pull request as ready for review April 16, 2024 00:05

tdyas requested a review from tgolsson April 16, 2024 00:05

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from d093168 to 54aceee Compare April 16, 2024 00:09

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch 2 times, most recently from 797ee54 to c9ca3fa Compare May 9, 2024 23:07

tdyas mentioned this pull request May 10, 2024

add workspace environment support #20900

Open

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from c9ca3fa to b35e034 Compare May 10, 2024 15:37

stuhood approved these changes May 18, 2024

View reviewed changes

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from 86a6371 to 5a58a79 Compare May 20, 2024 17:15

tdyas commented May 20, 2024

View reviewed changes

src/rust/engine/process_execution/src/fork_exec.rs Outdated Show resolved Hide resolved

tdyas commented May 20, 2024

View reviewed changes

src/rust/engine/src/context.rs Outdated Show resolved Hide resolved

tdyas mentioned this pull request May 21, 2024

refactor fork/exec logic for the command runners #20944

Merged

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from 85db027 to 6c72fc9 Compare May 22, 2024 02:49

tdyas commented May 22, 2024

View reviewed changes

tdyas added 15 commits May 23, 2024 12:05

add ProcessExecutionStrategy::LocalInWorkspace

9cb5be4

add workspace::CommandRunner

9f663fa

setup workspace::CommandRunner

cba8b1e

allow configuring workspace environment from Python + test

5baddc5

set working directory correctly + test

126b8bb

test for output captures + use CapturedWorkdir machinery

cabf1e4

ensure file in build root is not captured

8c36adf

clippy fix

29b6455

fmt

cc6e940

refactor fork/exec logic for the command runners

a9975cb

add workspace tests

45ef8ad

plugin release notes

2262f2c

fix comments

4692dd4

update comment

035d18f

fix merge conflict

7c04231

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from 6c72fc9 to 212167f Compare May 23, 2024 16:13

prefix with underscore to mark as not public API

58fc224

tdyas force-pushed the workspace_process_intrinsic_as_strategy branch from 212167f to 58fc224 Compare May 23, 2024 16:25

benjyw approved these changes May 23, 2024

View reviewed changes

tdyas merged commit a79beec into pantsbuild:main May 24, 2024
25 checks passed

tdyas deleted the workspace_process_intrinsic_as_strategy branch May 24, 2024 03:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add "workspace" process execution strategy #20772

add "workspace" process execution strategy #20772

tdyas commented Apr 9, 2024 •

edited

tdyas Apr 9, 2024

stuhood May 18, 2024 •

edited

tdyas commented Apr 16, 2024

benjyw commented May 2, 2024

stuhood left a comment

stuhood May 18, 2024 •

edited

stuhood May 18, 2024

tdyas May 19, 2024

tdyas May 20, 2024

tdyas commented May 20, 2024

tdyas commented May 20, 2024

tdyas commented May 20, 2024

tdyas commented May 22, 2024

tdyas May 22, 2024 •

edited

tdyas May 22, 2024

tdyas May 23, 2024

benjyw left a comment

		# Reserved sentinel value representing execution within the workspace and not local sandbox.
		LOCAL_WORKSPACE_ENV_NAME = "__local_workspace__"

add "workspace" process execution strategy #20772

add "workspace" process execution strategy #20772

Conversation

tdyas commented Apr 9, 2024 • edited

tdyas Apr 9, 2024

Choose a reason for hiding this comment

stuhood May 18, 2024 • edited

Choose a reason for hiding this comment

tdyas commented Apr 16, 2024

benjyw commented May 2, 2024

stuhood left a comment

Choose a reason for hiding this comment

stuhood May 18, 2024 • edited

Choose a reason for hiding this comment

stuhood May 18, 2024

Choose a reason for hiding this comment

tdyas May 19, 2024

Choose a reason for hiding this comment

tdyas May 20, 2024

Choose a reason for hiding this comment

tdyas commented May 20, 2024

tdyas commented May 20, 2024

tdyas commented May 20, 2024

tdyas commented May 22, 2024

tdyas May 22, 2024 • edited

Choose a reason for hiding this comment

tdyas May 22, 2024

Choose a reason for hiding this comment

tdyas May 23, 2024

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

tdyas commented Apr 9, 2024 •

edited

stuhood May 18, 2024 •

edited

stuhood May 18, 2024 •

edited

tdyas May 22, 2024 •

edited