Fix --build_event_publish_all_actions with --remote_download_minimal #10972
Conversation
There was a problem hiding this comment.
Thanks for the PR, and sorry for the delay in reviewing here! The Google-internal version of this is a bit different: we insert output metadata through a totally different scheme. I'm consulting internally because on the surface, this seems like a more natural way to do it, so perhaps we could unify the codepaths. It may be a few more days, though, with all the disruptions going on right now. Apologies again!
| RemoteFileArtifactValue rm = injectedMetadata.get(execPathString); | ||
| if (rm != null) { | ||
| return rm; | ||
| } |
There was a problem hiding this comment.
nit: can't you just return rm unconditionally?
The ActionExecutedEvent references the primaryOutput of the executed action (see ActionExecutedEvent::referencedLocalFiles). ByteStreamBuildEventArtifactUploader::upload will try to ensure that this referenced file is present on the remote cache. It calls file.isDirectory(), which will eventually go to RemoteActionFileSystem::getRemoteInputMetadata. However, RemoteActionFileSystem::inputArtifactData only contains metadata about the input files, not action outputs. This change proposes a fix, where we make RemoteActionFileSystem implement MetadataInjector, and inject information about output files from ActionMetadataHandler::injectRemoteFile. The implementation is likely not correct for all cases, but demonstrates the issue and a possible approach to fix it.
f031332 to
0358947
Compare
|
I need for apologies @janakdr, I know we're all busy. :) Thanks for the update, and the initial review! |
|
As I mentioned above, the Google-internal version of this filesystem injects metadata earlier, while the Spawn is running. The equivalent in open source would be somewhere in RemoteCache.java, I think, inside the RemoteCache#download* methods. After a discussion, we think that is the right place for it here as well. The idea is that the filesystem should be the source of truth for the build: metadata should hit it before anywhere else. Moreover, internally at least, there are cases where the same Bazel "action" runs two commands. In that case, the second command consumes files produced by the first one, and so those files must be available on the filesystem, but those intermediate files are not considered action outputs, and so aren't ever injected into the MetadataHandler. Finally, it would be great to have a test for the situation you're fixing. Thanks! |
|
What's the state of this PR? |
|
Unfortunately I have not had time to rebase and add tests for this. I'll close this for now, and re-open when I get some time to spend on this again. |
The ActionExecutedEvent references the primaryOutput of the executed
action (see ActionExecutedEvent::referencedLocalFiles).
ByteStreamBuildEventArtifactUploader::upload will try to ensure that
this referenced file is present on the remote cache. It calls
file.isDirectory(), which will eventually go to
RemoteActionFileSystem::getRemoteInputMetadata. However,
RemoteActionFileSystem::inputArtifactData only contains metadata about
the input files, not action outputs.
This change proposes a fix, where we make RemoteActionFileSystem
implement MetadataInjector, and inject information about output files
from ActionMetadataHandler::injectRemoteFile.
The implementation is likely not correct for all cases, but demonstrates
the issue and a possible approach to fix it.
Fixes #10971