Skip to content

Communicate persistent worker protocol to remote execution service #28489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
armandomontanez wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −7
Open

Communicate persistent worker protocol to remote execution service #28489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,13 +528,17 @@ public RemoteAction buildRemoteAction(

// Get the remote platform properties.
Platform platform;
ImmutableMap.Builder<String, String> additionalPropertiesBuilder = ImmutableMap.builder();
if (toolSignature != null) {
platform =
PlatformUtils.getPlatformProto(
spawn, remoteOptions, ImmutableMap.of("persistentWorkerKey", toolSignature.key));
} else {
platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
additionalPropertiesBuilder.put("persistentWorkerKey", toolSignature.key);
}
if (spawn.getExecutionInfo().containsKey(ExecutionRequirements.REQUIRES_WORKER_PROTOCOL)) {
additionalPropertiesBuilder.put(
ExecutionRequirements.REQUIRES_WORKER_PROTOCOL,
Copy link
Collaborator

@fmeum fmeum Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the worker protocol is specific to Bazel, we could consider prefixing this with bazel-.

Copy link
Author

@armandomontanez armandomontanez Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not hugely in love with the fact that this looks different from persistentWorkerKey. It probably makes sense to just solve this with a short docs page that lists out established platform properties.

Copy link
Contributor

@jayconrod jayconrod Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessarily specific to Bazel? The protos are defined by Bazel of course, but other build tools and adapters may deal with these keys.

Copy link
Collaborator

@fmeum fmeum Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, other build systems could also use persistent workers. In practice I don't know of any such other usage (perhaps buck2?). If we think that this is worth using more widely, it would make sense to include in the REAPI spec.

Copy link
Contributor

@jayconrod jayconrod Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great to codify persistent workers in RE API. I don't think much would actually need to change in the protocol, but we'd need to document the conventions that Bazel follows (so persistentWorkerKey, requires-worker-protocol or whatever we call it, the --persistent_worker flag, separate of arguments between workers and actions).

What's the process for an RE change like that? Would it go through https://github.com/bazelbuild/proposals?

Copy link
Collaborator

@fmeum fmeum Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/bazelbuild/remote-apis would be the best place to discuss this in the form of a PR and/or in the monthly meeting.

Copy link
Contributor

@tjgq tjgq Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that, even though Bazel might the only client implementation that supports them, there are already multiple server implementations of persistent workers (AFAIK, Engflow and BuildBuddy both implement it; there might be more).

Unfortunately, the spec itself (which is maintained separately from Bazel at http://github.com/bazelbuild/remote-apis) is currently silent on persistent workers; they effectively operate as an unstandardized extension to the protocol (though my understanding is that existing implementations are convergent).

I agree that it would be desirable to eventually make persistent workers part of the spec, but so far nobody has tried to move it forward. If you'd like to get the discussion started, I think the most productive way to do so is to attend our monthly working group meeting. If that doesn't work for you, opening an issue on http://github.com/bazelbuild/remote-apis is fine too.

On the other hand, I don't think that this particular PR needs to be gated on standardizing the existing spec. I would like, however, to see some indication from existing implementations that they're willing to implement it before we do so on the Bazel side.

Copy link
Author

@armandomontanez armandomontanez Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to moving in parallel. I'll drop in to the sync next week. It seems worthwhile doing the work of standardizing this since not handling this extension tends to result in hard failures.

spawn.getExecutionInfo().get(ExecutionRequirements.REQUIRES_WORKER_PROTOCOL));
}
platform =
PlatformUtils.getPlatformProto(spawn, remoteOptions, additionalPropertiesBuilder.build());

SpawnScrubber spawnScrubber = scrubber != null ? scrubber.forSpawn(spawn) : null;
Command command =
Expand Down
11 changes: 9 additions & 2 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2636,6 +2636,7 @@ public void buildRemoteActionForRemotePersistentWorkers(
Spawn spawn =
new SpawnBuilder("@flagfile")
.withExecutionInfo(ExecutionRequirements.SUPPORTS_WORKERS, "1")
.withExecutionInfo(ExecutionRequirements.REQUIRES_WORKER_PROTOCOL, "json")
.withInputs(input, toolInput, runfilesArtifact)
.withTools(toolInput, runfilesArtifact)
.setPathMapper(
Expand Down Expand Up @@ -2726,10 +2727,16 @@ public void buildRemoteActionForRemotePersistentWorkers(
(byte[]) merkleTree.blobs().get(merkleTree.digest()),
ExtensionRegistry.getEmptyRegistry()))
.isEqualTo(rootDirectory);
assertThat(remoteAction1.getAction().getPlatform().getPropertiesList()).hasSize(1);
assertThat(remoteAction1.getAction().getPlatform().getPropertiesList()).hasSize(2);
assertThat(remoteAction1.getAction().getPlatform().getProperties(0).getName())
.isEqualTo("persistentWorkerKey");

// Ensure the worker protocol is communicated.
assertThat(remoteAction1.getAction().getPlatform().getProperties(1).getName())
.isEqualTo("requires-worker-protocol");
assertThat(remoteAction1.getAction().getPlatform().getProperties(1).getValue())
.isEqualTo("json");

// Check that if a non-tool input changes, the persistent worker key does not change.
fakeFileCache.createScratchInput(input, "value2");
var remoteAction2 = service.buildRemoteAction(spawn, context);
Expand All @@ -2739,7 +2746,7 @@ public void buildRemoteActionForRemotePersistentWorkers(
// Check that if a tool input changes, the persistent worker key changes.
fakeFileCache.createScratchInput(toolInput, "worker value2");
var remoteAction3 = service.buildRemoteAction(spawn, context);
assertThat(remoteAction3.getAction().getPlatform().getPropertiesList()).hasSize(1);
assertThat(remoteAction3.getAction().getPlatform().getPropertiesList()).hasSize(2);
assertThat(remoteAction3.getAction().getPlatform().getProperties(0).getName())
.isEqualTo("persistentWorkerKey");
assertThat(remoteAction3.getAction().getPlatform().getProperties(0).getValue())
Expand Down