Execution Platform Scoped Spawn Strategies

[Silic0nS0ldier]

While Bazel has the necessary primitives to configure a multi-platform build, it currently lacks a way to map registered execution platforms to spawn strategies. This complicates remote execution adoption by forcing indirect adjustments to be made such as;

• Tagging targets/actions with no-remote-exec to prevent the remote strategy being used for select targets.
• Keeping local and remote spawned action graphs separate, and setting global configuration according to which graph a target belongs to in bazel build.
• Keeping the platform consistent across remote and local.
• Taking advantage of unique properties of actions that are intended to be spawned remotely, like their mnemonics.

To date there have been 2 proposals to address this problem.

• 2019-08-01 - 2019-06-25-platforms-strategies.md by @katre
• 2023-03-31 - Execution Platforms vs. Strategies by @katre and @aragos

I've have a new one to add to the pile, though with a much reduced scope. The gist is that the existing spawn strategy flags (e.g. --spawn_strategy, --strategy=<mnemonic>) will be extended to accept an execution platform as a filter.

e.g.

# When the exec platform is "//:darwin_arm64" use one of "worker,sandboxed,local" by default
build --spawn_strategy=//:darwin_arm64=worker,sandboxed,local

This is intended to fill a capability gap while work on #11432 continues.

Proposal;

• 2023-12-07 - https://github.com/bazelbuild/proposals/blob/1423671b9176d057eda1a42e0e2a469c32ab2476/designs/2023-06-04-exec-platform-scoped-spawn-strategies.md
• 2024-02-10 - https://github.com/bazelbuild/proposals/blob/14f45b8ede1ba162ef7b2e7fdd26eda24409bd47/designs/2023-06-04-exec-platform-scoped-spawn-strategies.md

cc @katre

[katre]

Thanks for suggesting this! I have a lot of comments, but the top-level one is that I think this is a good step forward, and will be useful while we try to figure out a larger plan.

I'm going to split out comments into separate discussion threads, I hope this is easier, let me know if it isn't.

[katre]

This does not seek to solve;

This proposal aims to select the correct strategy for a given execution platform. It is not trying to solve the problem of picking the execution platform based on the execution strategy (as in Execution Platforms vs. Strategies. Can we make that more explicit?

[Silic0nS0ldier]

I've clarified this in bazelbuild/proposals#363. If its too buried in the details let me know, might be worth stating this in the abstract.

[katre]

RuleContext#getExecutionPlatform(), null if getToolchainContext() returns null

In theory, if RuleContext#getToolchainContext() returns null, it means that the rule isn't generating any actions. That said, we don't have any validation, so it's entirely possible that legacy rules still violate this. (I've filed #20505 to try to address this).

So, given that, you should assume the worst: that some rule will generate a Spawn with no execution platform set that should actually try to run (somewhere).

[katre]

I've vetted this (see my comment), and it turns out to be fairly safe: only file write actions ever execute without an execution platform set (and based on my reading those are implicitly the host platform: while there is an isRemote parameter in FileWriteActionContext.writeOutputToFile, it's ignored in Bazel's only implementation).

However, there's no clear way to add a validation to make sure this remains the case, so we may need some degree of defensive coding.

[Silic0nS0ldier]

In bazelbuild/proposals#363 I have updated the risk notes to account for the investigation results, and included potential mitigations (accept the risk and fix bugs if they come up, or refactor so null isn't a potential value for the exec platform).

[katre]

Here's the big one: I like the idea but dislike the flag syntax you've chosen.

I don't know how deeply you've looked into Bazel's flag parsing: it's very specific and arcane (and we'd love to rip it out and replace it wholesale, but Hyrum's law applies: any replacement would have to be bug-for-bug compatible semantically and syntactically, and so isn't worth it).

The likihood of misconfiguration can be reduced with;

--[no]require_platform_scoped_strategies to make inclusion of platforms in spawn strategy flags mandatory.
--[no]exhaustive_platform_strategies to make specifiction of spawn strategies for every registered execution platform required.

I don't think these are possible, for a few reasons:

1. You can't access other flags while parsing a flag, so there's no easy way to tell, while dealing with --spawn_strategy, that --require_platform_scoped_strategies is enabled.
   a. This means you'd need to add a validation step after flags are parsing, and before any analysis starts, to ensure that the strategies are set correctly. It might be possible to do this, but there's not a single central place for it.
   b. See BuildCommand, TestCommand, and AqueryCommand as three places that need this handling. All of them use BuildRequest, so maybe that would work, but I'm not sure we can guarantee it.
2. Furthermore, I'm not sure there's a single answer to "what are the available execution platforms?"
   a. The canonical answer is to fetch RegisteredExecutionPlatformsValue, but the Key class in that depends on the current configuration to check the value of --extra_execution_platforms.
   b. You can't easily call into Skyframe from BuildRequest, because it's before Skyframe has initialized.
   c. While I'm not aware of any transitions that change --extra_execution_platforms, it's currently possible (thanks I hate it). I am aware of transitions while change the related --extra_toolchains.

# For actions with the "FOO" mnemonic and exec platform "//:darwin_arm64" use "local"
build --strategy=FOO=//:darwin_arm64=local

The data structure for --strategy is the very complicated:

  @Option(
      name = "strategy",
      allowMultiple = true,
      converter = Converters.StringToStringListConverter.class,
      defaultValue = "null",
      ...)
  public List<Map.Entry<String, List<String>>> strategy;

The List<Map.Entry part is how Bazel's options parser handles key-value allowMultiple options: in theory this could eventually be collected to an actual Map but really it's directly iterated in BazelStrategyModule to be added to a SpawnStrategyRegistry.

Making this more complicated with a "key1=key2=value" flag seems overly complicated: not so much semantically for users (although I think it is), but definitely as an implementation detail.

[fmeum]

platform declarations do not always live in the workspace (main repo) nor is `platform(...) necessarily directly called. They may be generated by a macro, repository rule, extension, or be a static part of a ruleset.

While I do know of a number of examples and rules_go itself is guilty of it, I've always wondered whether this isn't fundamentally wrong: A platform ties in together all the different dimensions that matter in a build and no single ruleset can be aware of all of them. I'm starting to think that any kind of hard coded platform in a ruleset or external repo that isn't meant to be used solely as a parent platform is conceptually problematic.

Maybe one part of the solution to this problem space is to pin down best practices on how platforms should "flow through" builds?

[katre]

Ideally, rules repos won't define execution platforms, but configuration repos for remote execution might. (Rules repos defining target platforms is also worrying to me, but it's a different problem and hits issues of user experience that I don't have good answers for).

That said, your point is valid: it's worth not expanding the surface of the platform rule. At one point we discussed a separate execution_platform rule, and that would be a valid place to declare compatible strategies, but we're not there now.

I like the --execution_platform_strategy flag (note change in plurality) as a concept, but we need to clearly define the interaction when I have:

1. An action with description "Frobbing the quzzle component", mnemonic "FROB", and execution platform "//platform:macos_arm_remote_cluster"
2. --strategy_regexp=Frobbing.*=local
3. --strategy=FROB=sandbox
4. --execution_platform_strategies=//platform:macos_arm_remote_cluster=remote

I think in this case the execution platform strategy should win, but I'm not 100% on that.

[Silic0nS0ldier]

I think I miscommunicated the intent behind the --execution_platform_strateg[ies|y] flag. It was late for me when I sent the message, so I'm not surprised.

Rather than being another way to specify the spawn strategy to use, the idea was to declare the spawn strategies the execution platform supports. Such that we can state an exec platform supports for example worker,sandboxed,local to exclude remote as a possible spawn strategy. The placeholder name probably didn't help.

[katre]

Ahh, I see. The similarity to the other strategy flags confused me, but that makes much more sense.

Can you update the proposal with these ideas, and then we'll do a second round?

[Silic0nS0ldier]

PR is up for revised proposal. bazelbuild/proposals#363

[katre]

Thanks very much for the updates.

My only remaining question is that I'm not entirely sure how --require_platform_scoped_strategies would be implemented: have you begun to look into coding any of this?

Aside from that (and possibly seeking any idea for flag names: --execution_platform_supported_strategies works but is a bit long) I'm happy to approve this. Thanks for bringing this proposal to the project.

Let me know what your timeline looks like for implementation. I'm happy to review PRs and help get them imported, although because of the holidays today is my last working day until January 2, 2024.

[katre]

Your prototype looks sound, I don't know if you want me to comment on it yet. I like the idea of the PlatformScopedSpawnStrategy, I think that part makes a lot of sense.

Looking forward to the proposal updates, I think this is basically ready to go ahead. Thanks so much!

[Silic0nS0ldier]

Proposal update is up. bazelbuild/proposals#367

Your prototype looks sound, I don't know if you want me to comment on it yet.
The linked prototype is for the original proposal, and mainly served as a PoC. It illustrates my current thinking on how to approach the implementation, but commenting/reviewing wouldn't be the best use of our time.

When a proper implementation is ready, I'll reach out for comments then.

[Silic0nS0ldier]

I've got an incomplete prototype up for the revised proposal. If I'm misusing, abusing, or otherwise putting things in the wrong place let me know and I can course correct.
Silic0nS0ldier#6

[katre]

Thanks, this looks fine to me. I have a small suggestion for the flag name, but otherwise I'm happy to approve this proposal.

[Silic0nS0ldier]

Flag name tweaked in bazelbuild/proposals#367.

I'll give it a final (🤞) look tomorrow and put it up for the approval review.

[katre]

Hi, I'm trying to follow up on this. I'm still happy with the proposal, but I never saw a PR to mark it as approved, and I haven't seen any incoming implementation PRs. Is this still something you're interested in working on?

(In a related area, see my proposal for Selectively Enabling Execution Platforms).

[Silic0nS0ldier]

First sliver of (hopefully merge-able) work. #22925

Next up, trying to make getExecutionPlatform() never return null. From my skim null can arise in a few scenarios currently.

1. Builtin actions like MiddlemanAction, SymlinkAction and SolibSymlinkAction.
2. RuleContext where exec platform is resolved from toolchain context, which may be null.
3. Exec groups (I think, cannot track down where right now).

(1) isn't too much of a worry. A "bazel internal" platform stub could be used satisfy non-null requirements. (2) may be a non-issue since the class in question states instances only exist during analysis. (3) seems like it would represent a bug (if it were to ever happen).

There are several code comments stating that null for PlatformInfo means the host platform should be assumed. Given the latter context, this may actually be referring to builtin actions which are unaffected by spawn strategy controls. My current thinking is that this unique scenario may benefit from a more explicit representation, to distinguish between bugs (null exec platform) and builtin actions.

[katre]

Thanks for the update.

Despite being represented as an action, MiddlemanAction isn't actually executable, and so a null execution platform is reasonable. On the other hand, SymlinkAction and SolibSymlinkAction are actually executable and should have real execution platforms.

RuleContext and ExecGroup actually use the same underlying logic (all of the RuleContext toolchain-related code delegates to the default ExecGroup), which means that the execution platform cannot actually be null if the rule can create actions (some rules, like PlatformRule explicitly disable toolchain resolution and should never create actions). We don't have good checks for this: it'd be nice to add a check that these rules don't actually create actions, but the implicit expectation is that any rule which can create actions has an execution platform. Cleaning all of this up definitely has value.

Do you intend any further changes to this proposal? Feel free to send me PRs but I'd like to get it marked as approved, even if it will change during implementation, just so that we know the current state. Then you can start sending out implementation PRs.

[Silic0nS0ldier]

PR to put proposal into approved is up. bazelbuild/proposals#387

[Silic0nS0ldier]

Been a lot of progress around this since July, if not on this specifically.

• Java Language Support was forked as Java+Bazel Language Support. As of v0.3.1;
  • Import resolution is fully operational in the Bazel repo. Previously external dependencies (e.g. Google base libraries, JavaX, JUnit) and generated code (protos) would not resolve.
  • No longer crashes with Compiler is already in-use after a few minutes of usage.
  • Reference lookups such as "Find All References" now consistently work, no longer crashing with an out of bounds error.
  • Updated workspace detection to support bzlmod changes and handle extensioned (.bazel) versions.
• The dev container for Bazel has been refined through usage via Bazel migration projects (Silic0nS0ldier/java-language-server and Silic0nS0ldier/vscode-git-monolithic-extension).
• Practise, practise, practise.
  • (merged) Remove 4 no-op CLI flags #23233
  • (merged) Remove 6 no-op CLI flags #23375
  • (discarded) Filter files early in bytestream uploader to avoid redundant work and double handling #23252
  • (discarded) Skip path metadata lookup in bytestream uploader when artefact won't be uploaded #23374

Progress on this...

• Give builtin actions a platform #23231
  This will help to cut down on places where null could creep in. Formally at least, annotations don't count for much without something to validate (e.g. NullAway, JSpecify, Infer - Nullsafe).
• Exec platform scoped spawn strategies prototype Silic0nS0ldier/bazel#6
  I'm looking over this, and attempting to recreate the prototype. So far...
  • With the help of find-all-references, I tracked down where SpawnStrategyRegistry is constructed (in src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java).
    CLI options can be access in this scope via env (CommandEnvironment).
    e.g. env.getOptions().getOptions(ClientOptions.class)
  • At that same site I can access the product name (possibly as many as 3 ways, all via env).
    With this I can implement a Precondition check to ensure in Bazel, anything asking for a spawn strategy must have an execution platform. This is to favour correctness over a defect (null platform) allowing the wrong strategy to be selected.

I'm aiming to have this implemented to an experimental level ahead of the Bazel v8 release branch being cut (this month).

[Silic0nS0ldier]

Took longer than planned (needed a solid block of focus time to hammer this out) but I've finally got it done!

#24265

Half done... The misconfiguration safeguard --require_platform_scoped_strategies needs some more work. Had some issues threading through the CLI options into src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredExecutionPlatformsFunction.java.