add workspace environment support #20900

tdyas · 2024-05-10T00:53:43Z

Add support for a "workspace environment" which is similar to a local environment except that execution happens in the repository itself (i.e., the "workspace") and not in an execution sandbox. This implements the user-visible experience of Design B from the "In-Workspace Process Execution" design document.

This PR builds on top of the Rust support for in-workspace execution contained in #20772.

Users will use the new workspace_environment target type to configure workspace execution support.

The pants-workspace-exec-testing repository contains an example of how to use this support to allow Pants to invoke Bazel in the workspace.

tdyas · 2024-05-10T00:55:07Z

src/python/pants/core/util_rules/adhoc_process_support.py

+        # TODO: This is necessary for tests of `adhoc_tool` and `shell_command` with
+        # workspace execution to pass repeatedly in local Pants development.
+        # We need a viable solution instead of this hack.
+        cache_scope=ProcessCacheScope.PER_SESSION,


@benjyw: Any ideas on how to fix the test caching issue in the Pants repository and avoid this hack?

tdyas · 2024-05-10T01:01:48Z

~~Only the most recent commit is new. The other commits are from #20772.~~

tdyas · 2024-05-22T03:05:23Z

src/python/pants/backend/adhoc/target_types.py

@@ -253,6 +254,33 @@ class AdhocToolOutputRootDirField(StringField):
    )


+class AdhocToolCacheScopeField(StringField):


Introduction of cache scope to adhoc_tool and shell_command target types should be moved to a separate PR if we decide to keep it. It was necessary to provide a way for tests to set cache_scope to ProcessCacheScope.PER_SESSION in order to avoid cached results from prior tests runs from being used for a current test run.

This points to a more fundamental problem: Will workspace executions which modify the workspace be inherently a problem if the user is looking for them to always execute?

Maybe dumb question, but why isn't the caching strategy determined by the environment? I.e., local is cacheable and workspace is not? Do we need this extra degree of freedom?

If a user does have a reproducible process invoked in the workspace environment (e.g., Bazel), shouldn't Pants cache the output then?

There is two sort of issues here:

Should processes invoked in workspace environments be cacheable? My thought is yes they should (for example, Bazel invocations). (Invalidations may occur as a result of the metadata-based invalidation to come from PoC for metadata-based invalidation #20914.)

The tests of the workspace environment fail when they invoke a non-idempotent process which writes a file into the workspace and which the test checks for to ensure execution actually happened in the workspace). Running that non-idempotent test multipe times fails unless the cache scope is "per session." Maybe this is a code smell in the test? Is there another way that the tests can set cache scope without exposing that ability in public API?

What key would we cache against though?

I can see not rerunning the process if the invalidation globs haven't changed from the last time we ran the process, but that is not the same as caching. In other words, of we run the process in state A, then as long as we remain in state A we don't rerun it. Then we switch to state B, so obviously we must rerun the process (and we do because the file mtimes have changed). Then we switch back to state A. Normally we'd retrieve the result from cache, but for workspace environments I think we have to rerun the process, no? All we know is that mtimes have changed, we don't fingerprint anything, so there's nothing to compute a cache key out of.

Or am I fundamentally misunderstanding the proposition here?

This PR does not introduce the metadata-based cache key. Thus, you may want to review #20914 which does introduce the cache key: namely a hash of the "full stats" of the files on which the targets are depending via a invalidation_globs field. This is injected into the applicable Process as an environment variable.

src/python/pants/core/util_rules/system_binaries.py

docs/notes/2.22.x.md

benjyw · 2024-05-24T23:09:23Z

src/python/pants/backend/adhoc/target_types.py

@@ -253,6 +254,33 @@ class AdhocToolOutputRootDirField(StringField):
    )


+class AdhocToolCacheScopeField(StringField):


Maybe dumb question, but why isn't the caching strategy determined by the environment? I.e., local is cacheable and workspace is not? Do we need this extra degree of freedom?

src/python/pants/core/util_rules/environments.py

src/python/pants/core/util_rules/system_binaries.py

tdyas · 2024-05-28T03:51:46Z

src/python/pants/core/util_rules/environments.py

+    def default_cache_scope(self) -> ProcessCacheScope:
+        if self.val and self.val.has_field(LocalWorkspaceCompatiblePlatformsField):
+            return ProcessCacheScope.PER_SESSION
+        else:
+            return ProcessCacheScope.SUCCESSFUL


Added this method as a way for an environment to specify what the default cache scope is.

@benjyw: Is this what you were thinking with your suggestion?

If so, this maybe should be pulled out and all applicable use sites updated.

huonw

Nice!

I haven't approved because I haven't reviewed in detail and it seems @benjyw is on top of that. Just have some questions prompted by the docs.

huonw · 2024-06-02T04:18:06Z

docs/docs/using-pants/environments.mdx

+
+The primary motivation for this feature is to better support integration with third-party build orchestration tools (for example, Bazel) which may not operate properly when not invoked in the repository (including in some cases incurring signifcant performance penalties).
+
+There is a significant trade-off though which makes this feature inherently **UNSAFE**: Pants cannot reasonbly guarantee that build processes are reproducible if they run in the workspace environment. Thus, Pants puts that burden on you, the Pants user, to guarantee that any process executed in the workspace environment is reproducible based solely on inputs in the repository. If a process is not reproducible, then unknown side effects may occur.


Some questions about side-effects/process executions and "UNSAFE" call out:

Before this PR, one can run non-reproducible code within a sandbox (like shell_command(command="date", ...) that depends on the time, or something that reads mutable data from the internet) and this might not behave like someone expects. From a Pants perspective, are the side-effects from this approach similar or worse than that? (Obviously the process itself could do things arbitrarily bad too, but that's not the focus.)

Is there risk of problems due to processes being killed or cancelled in unexpected ways? If so, is it worth calling that out?

tdyas added the category:new feature label May 10, 2024

tdyas commented May 10, 2024

View reviewed changes

tdyas force-pushed the shell_workspace_support_2 branch 2 times, most recently from 40dd9d4 to 2808018 Compare May 10, 2024 15:38

tdyas force-pushed the shell_workspace_support_2 branch 3 times, most recently from 66e9416 to 3387cae Compare May 22, 2024 02:59

tdyas commented May 22, 2024

View reviewed changes

src/python/pants/core/util_rules/system_binaries.py Outdated Show resolved Hide resolved

tdyas force-pushed the shell_workspace_support_2 branch 2 times, most recently from d5286e8 to 350d7cf Compare May 24, 2024 03:31

tdyas requested a review from a team May 24, 2024 03:33

tdyas marked this pull request as ready for review May 24, 2024 03:35

tdyas requested a review from benjyw May 24, 2024 03:37

benjyw reviewed May 24, 2024

View reviewed changes

tdyas force-pushed the shell_workspace_support_2 branch 4 times, most recently from 1dfd950 to ef67eea Compare May 28, 2024 03:49

tdyas commented May 28, 2024

View reviewed changes

tdyas requested review from benjyw, stuhood and huonw May 28, 2024 03:51

tdyas added 6 commits June 1, 2024 13:02

plugin release notes

0a9e0c5

fix test

7601380

revert Cargo.lock change

7fd7787

adjust target type help text

6bf1cc7

introduce sandbox_base_path on EnvironmentTarget

b99e95f

introduce default_cache_scope on EnvironmentTarget

5e66cd1

tdyas force-pushed the shell_workspace_support_2 branch from ef67eea to 5e66cd1 Compare June 1, 2024 17:03

add docs for workspace_environment

22762ca

huonw reviewed Jun 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add workspace environment support #20900

add workspace environment support #20900

tdyas commented May 10, 2024 •

edited

tdyas May 10, 2024

tdyas commented May 10, 2024 •

edited

tdyas May 22, 2024 •

edited

benjyw May 24, 2024

tdyas May 24, 2024

tdyas May 24, 2024 •

edited

benjyw May 25, 2024

tdyas May 26, 2024

benjyw May 24, 2024

tdyas May 28, 2024

huonw left a comment

huonw Jun 2, 2024

		@@ -253,6 +254,33 @@ class AdhocToolOutputRootDirField(StringField):
		)


		class AdhocToolCacheScopeField(StringField):


		The primary motivation for this feature is to better support integration with third-party build orchestration tools (for example, Bazel) which may not operate properly when not invoked in the repository (including in some cases incurring signifcant performance penalties).

		There is a significant trade-off though which makes this feature inherently UNSAFE: Pants cannot reasonbly guarantee that build processes are reproducible if they run in the workspace environment. Thus, Pants puts that burden on you, the Pants user, to guarantee that any process executed in the workspace environment is reproducible based solely on inputs in the repository. If a process is not reproducible, then unknown side effects may occur.

add workspace environment support #20900

Are you sure you want to change the base?

add workspace environment support #20900

Conversation

tdyas commented May 10, 2024 • edited

Choose a reason for hiding this comment

tdyas commented May 10, 2024 • edited

tdyas May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdyas commented May 10, 2024 •

edited

tdyas commented May 10, 2024 •

edited

tdyas May 22, 2024 •

edited

tdyas May 24, 2024 •

edited