Filter spawn strategies by execution platform by Silic0nS0ldier · Pull Request #24265 · bazelbuild/bazel

Silic0nS0ldier · 2024-11-11T03:43:57Z

This PR implements part of the Execution Platform Scoped Spawn Strategies proposal.

It adds a new flag --allowed_strategies_by_exec_platform which permits filtering spawn strategies for spawns execution platform.

Example

# //.bazelrc
# Default strategies (order sensitive)
build --spawn_strategy=remote,worker,sandboxed,local

# Mnemonic targeted strategy override (order sensitive)
build --strategy=BAR=remote,sandboxed

# Host platform allowed strategies
build --allowed_strategies_by_exec_platform=@platforms//host:host=local,sandboxed,worker

# Remote platform allowed strategies
build --allowed_strategies_by_exec_platform=//:remote_platform=remote

For an action with mnemonic FOO configured for the host platform (@platforms//host:host), it will resolve worker,sandboxed,local as it's spawn strategy candidates.

remote was eliminated as a candidate (not in allow list for platform).
Order from --spawn_strategy was preserved.

For an action with mnemonic BAR configured for the host platform (@platforms//host:host), it will resolve sandboxed as it's spawn strategy candidate.

remote was eliminated as a candidate (not in allow list for platform).
Mnemonic override applied, leaving sandboxed as the final candidate.

For an action with mnemonic BAR configured for the remote platform (//:remote_platform), it will resolve remote as it's spawn strategy candidate.

sandboxed was eliminated as a candidate (not in allow list for platform).
Mnemonic override applied, leaving remote as the final candidate.

If no spawn strategy candidate remains after filtering, the standard error will be logged.

ERROR: /workspaces/___/BUILD.bazel:3:22: _description_ [for tool] failed: _mnemonic_ spawn \
cannot be executed with any of the available strategies: []. Your --spawn_strategy, \
--genrule_strategy, --strategy and/or --allowed_strategies_by_exec_platform flags are probably \
too strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for advice

Silic0nS0ldier · 2024-11-11T03:51:35Z

cc @katre

tjgq · 2024-11-19T17:35:24Z

@Silic0nS0ldier Can you please fix the test failures? (It looks like the execution platform is null in a bunch of places.)

Silic0nS0ldier · 2024-11-19T23:06:06Z

@tjgq The null platform issue is fixed by #23231 which this is dependent on.

Silic0nS0ldier · 2025-02-10T08:58:10Z

src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java

            : ImmutableSortedMap.of();

-    if (spawn.getExecutionPlatform() == null
+    if ((spawn.getExecutionPlatform() == null || spawn.getExecutionPlatform().execProperties().isEmpty())


Without this change, Command protos end up populating platform (a deprecated field) with an empty Platform, increasing the size of the overall message.

In JSON it looks something like { "platform": {} }.

This was caught by //src/test/java/com/google/devtools/build/lib/remote:RemoteTests.

src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

meisterT · 2025-05-07T11:45:22Z

What's the status of this PR?

Silic0nS0ldier · 2025-06-28T07:23:49Z

I didn't have much time to focus on this recently, but I do now.

PR has been rebased and prior feedback implemented. This is now ready for review.

katre

LGTM, But this review should be assigned to @gregestren since I am no longer on the Bazel team.

One comment to clarify the intended semantics, but overall this looks good.

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

Silic0nS0ldier · 2025-07-12T05:56:14Z

@gregestren This is ready for review.

katre

LGTM (with a few requests for clarification in docs and comments).

Needs to wait for a Bazel team member to approve and merge.

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

fmeum · 2025-11-13T18:14:27Z

@Silic0nS0ldier What's the status? Happy to help if you need any!

Silic0nS0ldier · 2025-11-13T18:22:39Z

Waiting for review.

fmeum · 2025-11-13T18:26:04Z

@gregestren I'll do a review pass soon. Let's try to get this into Bazel 9 as it nicely complements the new default test toolchain to finally make "what's executed where" work

fmeum · 2025-11-13T18:26:17Z

@bazel-io fork 9.0.0

gregestren · 2025-11-14T22:41:45Z

Checking in: I'll do a proper review Monday. Thanks for your ongoing patience @Silic0nS0ldier

fmeum · 2025-06-28T08:34:23Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

  }

+  private static class StrategyPlatformFilter {
+    private final StrategyMapper strategyMapper;


This looks unused?

Yep, likely a leftover from an earlier prototype. I'll get it removed.

fmeum · 2025-11-15T07:01:05Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

      }

+      ImmutableListMultimap.Builder<Label, SpawnStrategy> platformToStrategies = ImmutableListMultimap.builder();
+      for (Map.Entry<Label, List<String>> entry : execPlatformFilters.entrySet()) {


The filters are a hash map and thus didn't have a defined iteration order, but here we produce an immutable collection that does. Could you pick one or the other?

Iteration order should not matter for;

The key (a platform Label), since usage remains as a key and key order is not relied on.

The values (a list of spawn strategies, converted from List<String> to ImmutableList<SpawnStrategy>), since usage is only for filtering.

Realistically the values should be using ImmutableSet given order does not matter and usage is exclusively presence checks.
To that end, there is ImmutableSetMultimap.
EDIT: Provided SpawnStrategy is safe to hash.

I can't see SpawnStrategy used as a map key nor set value, so changing to ImmutableSetMultimap may not be a trivial change. Better left to a PR after this IMO.

It looks like this iterated at least to generate the info log above. The main problem with these "sequence-washed" collections is that future usage may start to rely on ordering based on the guarantees afforded by ImmutableCollection types. Using a LinkedHashMap would be a very simple fix.

FWIW I generally think we should always used LinkedHashMap unless there's some known performance / resource risk to doing so.

Taking another look, this is not a problem (anti-pattern? code smell?) introduced by this PR. The exact same map-to-map operation is done for;

mnemonicToIdentifiers

mnemonicToLocalDynamicIdentifiers

mnemonicToRemoteDynamicIdentifiers

which are all iterating over HashMap<String, List<String>> lists.

The only place (currently) where map ordering can result in different behaviour is in logging (logSpawnStrategies and possibly in new SpawnStrategyRegistry).

I'm not against making changes to ensure stable ordering (especially given it is logged, inconsistent order can be a source of confusion) however I do think it is out of scope for this PR. The spawn registry is an important component so I'd personally like to keep the changeset as minimal as possible (less to step through if something breaks and a git bisect is necessary).

If everyone is happy, I'll raise a separate PR that addresses the map ordering concerns raised here (e.g. switching to LinkedHashMap).

Sounds good to me, I couldn't tell from the diff that this was so invasive (same for the copy-on-write data structure).

I've gone in a bit of a different direction with this feedback in #27975. Rather than stabilizing the implementation detail, I've added lexical sorting to all keys and sets that are order insensitive.

This way if a behavior change is implemented, it will be (at least a little) more obvious.

fmeum · 2025-11-16T21:49:40Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

+     *         Order from {@code candidateStrategies} is preserved.
+     */
+    public <T extends SpawnStrategy> List<T> getStrategies(
+        Spawn spawn, List<T> candidateStrategies) {


Suggested change

Spawn spawn, List<T> candidateStrategies) {

Spawn spawn, Collection<T> candidateStrategies) {

so that an `ImmutableCollection can be passed in directly without the copy-on-write adapter.

Switching to Collection here is an easy enough change.

Removing the copy-on-write adapter is a lot more "viral" of a change, spreading all the way to SpawnStrategyRegistry.getStrategies(Spawn, EventHandler). A refactor such as that is deserving of it's own PR.

Addressing in #27975

fmeum · 2025-11-16T21:51:57Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

      }

+      ImmutableListMultimap.Builder<Label, SpawnStrategy> platformToStrategies = ImmutableListMultimap.builder();
+      for (Map.Entry<Label, List<String>> entry : execPlatformFilters.entrySet()) {


It looks like this iterated at least to generate the info log above. The main problem with these "sequence-washed" collections is that future usage may start to rely on ordering based on the guarantees afforded by ImmutableCollection types. Using a LinkedHashMap would be a very simple fix.

fmeum · 2025-11-16T21:51:59Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

      }

+      ImmutableListMultimap.Builder<Label, SpawnStrategy> platformToStrategies = ImmutableListMultimap.builder();
+      for (Map.Entry<Label, List<String>> entry : execPlatformFilters.entrySet()) {


It looks like this iterated at least to generate the info log above. The main problem with these "sequence-washed" collections is that future usage may start to rely on ordering based on the guarantees afforded by ImmutableCollection types. Using a LinkedHashMap would be a very simple fix.

src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java

gregestren · 2025-11-17T21:58:43Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

   * <p>If the reason for selecting the context is worth mentioning to the user, logs a message
   * using the given {@link Reporter}.
+   *
+   * NOTE: This method is public for Blaze, `getStrategies(Spawn, EventHandler)` must not be used in Bazel.


What does the second part of this comment mean? Doesn't Bazel have calls to getStrategies(Spawn, EventHandler)?

🤦 I got the wrong method signature.
getStrategies(ActionExecutionMetadata, String, EventHandler) is the one only used in Blaze (#22925 (comment)).

Good catch!

gregestren · 2025-11-17T22:04:46Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

      }

+      ImmutableListMultimap.Builder<Label, SpawnStrategy> platformToStrategies = ImmutableListMultimap.builder();
+      for (Map.Entry<Label, List<String>> entry : execPlatformFilters.entrySet()) {


FWIW I generally think we should always used LinkedHashMap unless there's some known performance / resource risk to doing so.

gregestren · 2025-12-08T21:26:25Z

Some internal tests are failing and blocking merging. I'll look into them and report back: keeping Bazel 9 cutoff timelines in mind.

gregestren · 2025-12-09T19:00:22Z

Some internal tests are failing and blocking merging. I'll look into them and report back: keeping Bazel 9 cutoff timelines in mind.

Internal PR adjusted and now in the merge queue. Had to:

register the filter values with Google's version of BazelStrategyModule
add exec platforms to internal tests
adjust execution_strategies_test for slightly different strategy names

Also applied a few code lints and BUILD dependency cleanups.

This'll get merged as soon as we get internal approvals.

gregestren · 2025-12-10T22:36:03Z

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java

+     *         platform or all if no restrictions are in place.
+     *         Order from {@code candidateStrategies} is preserved.
+     */
+    public <T extends SpawnStrategy> List<T> getStrategies(


Forwarding a comment on the internal PR:

"Do we really need this overload? Can we just fix call sites so that they deal in immutable values?"

I take no personal position on this but WDYT?

I responded that I'll forward the comment but merge in the meantime to get into Bazel 9.0. Merge is currently processing...

No personal position beyond such a refactor being too much for this PR. Similar to https://github.com/bazelbuild/bazel/pull/24265/files/d88b5e5a1fcc4e94ca38cd5dc0758df66228550b#r2545460338.

Addressing in #27975 (mostly, ImmutableList is a subclass of List so changes are largely within SpawnStrategyRegistry.java).

This PR implements part of the [Execution Platform Scoped Spawn Strategies](https://github.com/bazelbuild/proposals/blob/2b717b19fe805c405576c4feb9ffc6b772068898/designs/2023-06-04-exec-platform-scoped-spawn-strategies.md) proposal. It adds a new flag `--allowed_strategies_by_exec_platform` which permits filtering spawn strategies for spawns execution platform. ## Example ```ini # //.bazelrc # Default strategies (order sensitive) build --spawn_strategy=remote,worker,sandboxed,local # Mnemonic targeted strategy override (order sensitive) build --strategy=BAR=remote,sandboxed # Host platform allowed strategies build --allowed_strategies_by_exec_platform=@platforms//host:host=local,sandboxed,worker # Remote platform allowed strategies build --allowed_strategies_by_exec_platform=//:remote_platform=remote ``` For an action with mnemonic `FOO` configured for the host platform (`@platforms//host:host`), it will resolve `worker,sandboxed,local` as it's spawn strategy candidates. * `remote` was eliminated as a candidate (not in allow list for platform). * Order from `--spawn_strategy` was preserved. For an action with mnemonic `BAR` configured for the host platform (`@platforms//host:host`), it will resolve `sandboxed` as it's spawn strategy candidate. * `remote` was eliminated as a candidate (not in allow list for platform). * Mnemonic override applied, leaving `sandboxed` as the final candidate. For an action with mnemonic `BAR` configured for the remote platform (`//:remote_platform`), it will resolve `remote` as it's spawn strategy candidate. * `sandboxed` was eliminated as a candidate (not in allow list for platform). * Mnemonic override applied, leaving `remote` as the final candidate. If no spawn strategy candidate remains after filtering, the standard error will be logged. ``` ERROR: /workspaces/___/BUILD.bazel:3:22: _description_ [for tool] failed: _mnemonic_ spawn \ cannot be executed with any of the available strategies: []. Your --spawn_strategy, \ --genrule_strategy, --strategy and/or --allowed_strategies_by_exec_platform flags are probably \ too strict. Visit bazelbuild#7480 for advice ``` Closes bazelbuild#24265. PiperOrigin-RevId: 842893103 Change-Id: If2bdb19f08e5dd3c5941f4cfd6c39d7295ff0c25

Silic0nS0ldier marked this pull request as ready for review November 11, 2024 03:46

Silic0nS0ldier requested a review from a team as a code owner November 11, 2024 03:46

github-actions bot added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Nov 11, 2024

meisterT requested review from katre and tjgq November 11, 2024 09:45

Silic0nS0ldier mentioned this pull request Jan 21, 2025

Give builtin actions a platform #23231

Closed

Silic0nS0ldier requested a review from a team as a code owner February 10, 2025 08:48

Silic0nS0ldier commented Feb 10, 2025

View reviewed changes

katre reviewed Feb 10, 2025

View reviewed changes

src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java Outdated Show resolved Hide resolved

katre reviewed Feb 10, 2025

View reviewed changes

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java Outdated Show resolved Hide resolved

katre suggested changes Feb 10, 2025

View reviewed changes

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java Outdated Show resolved Hide resolved

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java Outdated Show resolved Hide resolved

keith mentioned this pull request Mar 5, 2025

Dynamic execution ignores exec constraints #25221

Open

Silic0nS0ldier marked this pull request as draft June 18, 2025 11:53

Silic0nS0ldier force-pushed the filter-strategies-for-exec-platform branch from 01357a0 to 2dd0316 Compare June 18, 2025 12:46

Silic0nS0ldier force-pushed the filter-strategies-for-exec-platform branch from 26efd49 to 18a0fe5 Compare June 27, 2025 13:55

Silic0nS0ldier marked this pull request as ready for review June 28, 2025 07:23

Silic0nS0ldier requested a review from katre June 28, 2025 07:23

github-actions bot added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Jun 28, 2025

katre suggested changes Jun 30, 2025

View reviewed changes

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java Show resolved Hide resolved

Silic0nS0ldier force-pushed the filter-strategies-for-exec-platform branch from b834e0d to 9667ea5 Compare July 12, 2025 05:13

katre approved these changes Jul 12, 2025

View reviewed changes

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java Outdated Show resolved Hide resolved

src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java Show resolved Hide resolved

Silic0nS0ldier force-pushed the filter-strategies-for-exec-platform branch 2 times, most recently from 592eefc to 27786ba Compare July 20, 2025 01:15

bazel-io mentioned this pull request Nov 13, 2025

[9.0.0] Filter spawn strategies by execution platform #27667

Closed

fmeum reviewed Nov 15, 2025

View reviewed changes

Silic0nS0ldier force-pushed the filter-strategies-for-exec-platform branch from 51b35fd to ce39c90 Compare November 16, 2025 20:41

Silic0nS0ldier requested a review from fmeum November 16, 2025 20:42

fmeum reviewed Nov 16, 2025

View reviewed changes

gregestren reviewed Nov 17, 2025

View reviewed changes

Silic0nS0ldier force-pushed the filter-strategies-for-exec-platform branch from ce39c90 to 14019a4 Compare November 20, 2025 11:17

Silic0nS0ldier requested review from fmeum and gregestren November 20, 2025 11:18

Silic0nS0ldier added 2 commits November 28, 2025 15:01

Filter spawn strategies by execution platform

854c8ed

Remove leftover strategyMapper reference

d88b5e5

Silic0nS0ldier force-pushed the filter-strategies-for-exec-platform branch from 14019a4 to d88b5e5 Compare November 28, 2025 15:01

fmeum approved these changes Dec 8, 2025

View reviewed changes

gregestren approved these changes Dec 8, 2025

View reviewed changes

gregestren added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 8, 2025

gregestren reviewed Dec 10, 2025

View reviewed changes

copybara-service bot closed this in ff3d6d0 Dec 10, 2025

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Dec 10, 2025

Silic0nS0ldier mentioned this pull request Dec 13, 2025

Cleaning up SpawnStrategyRegistry #27975

Draft

	Spawn spawn, List<T> candidateStrategies) {
	Spawn spawn, Collection<T> candidateStrategies) {

Conversation

Silic0nS0ldier commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Uh oh!

Silic0nS0ldier commented Nov 11, 2024

Uh oh!

tjgq commented Nov 19, 2024

Uh oh!

Silic0nS0ldier commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meisterT commented May 7, 2025

Uh oh!

Silic0nS0ldier commented Jun 28, 2025

Uh oh!

katre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Silic0nS0ldier commented Jul 12, 2025

Uh oh!

katre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmeum commented Nov 13, 2025

Uh oh!

Silic0nS0ldier commented Nov 13, 2025

Uh oh!

fmeum commented Nov 13, 2025

Uh oh!

fmeum commented Nov 13, 2025

Uh oh!

gregestren commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Silic0nS0ldier Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Silic0nS0ldier commented Nov 11, 2024 •

edited

Loading

Silic0nS0ldier commented Nov 19, 2024 •

edited

Loading

Silic0nS0ldier Nov 16, 2025 •

edited

Loading