-
-
Notifications
You must be signed in to change notification settings - Fork 628
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.
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
Populate output_directory for remote execution responses #5709
Comments
This was referenced Apr 16, 2018
What are the thoughts behind eager fetching of outputs?
As it stands, the v1 product exposed by JVMCompile requires that the files
exist locally, but I figured that we'd want to deprecate that guarantee
soon.
An explicit synchronous call to warm/pre-fetch might make sense for the JVM
compile usecase... I could also imagine fetching by default, but only in
the background?
…On Mon, Apr 16, 2018, 5:32 PM Daniel Wagner-Hall ***@***.***> wrote:
Currently, ExecuteProcessRequest
<https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/lib.rs#L25-L48>
doesn't specify the list of output file paths it expects to be generated by
the process execution, the local
<https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/local.rs#L9-L30>
and remote
<https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/remote.rs#L51-L144>
executors don't fetch them and put them in the Store, and
ExecuteProcessResult
<https://github.com/pantsbuild/pants/blob/e20b205fa8d065222c03f47b16187a65ddf60efa/src/rust/engine/process_execution/src/lib.rs#L53-L58>
doesn't expose them. We should fix all of the above.
Once we have the ability to recursively download Directories
<#5708>, we should:
1. Add expected output files to ExecuteProcessRequest
<https://github.com/pantsbuild/pants/blob/628c83d5ab2706b0f64d69568c57a718ec7c5e2a/src/rust/engine/process_execution/src/lib.rs#L27>
.
2. Populate it on the Action
<https://github.com/pantsbuild/pants/blob/628c83d5ab2706b0f64d69568c57a718ec7c5e2a/src/rust/engine/process_execution/src/remote.rs#L183-L185>
.
3. Call the fetch code from #5708
<#5708> when we get a
response
<https://github.com/pantsbuild/pants/blob/628c83d5ab2706b0f64d69568c57a718ec7c5e2a/src/rust/engine/process_execution/src/remote.rs#L219-L223>
to prime the Store with the output.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#5709>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAC2lF_HboGm_E7D8_jhAvDq3ImwPmE2ks5tpLmhgaJpZM4TWw0E>
.
|
My goal is to make remoting be a drop-in replacement for what currently exists, and then to make optimisations in the future. I find it incredibly likely we'll want to make output fetching lazy in the future, but it's a pure optimisation and is not on the critical path for getting something working end-to-end :) |
Hm, ok. I suppose eagerly fetching doesn't mean eagerly materializing. |
illicitonion
changed the title
Eagerly fetch output files from remote execution responses
Populate output_directory for remote execution responses
May 17, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, ExecuteProcessRequest specifies a list of output file paths that it wants to know about if they're generated by the process, and ExecuteProcessResult has a Digest of a Directory representing these outputs.
Local process execution reads these files from disk, and populates the output Directory Digest.
Remote process execution does not.
The ActionResult protocol buffer message has a list of OutputFile messages which are populated by the remote server.
We need to take this list, turn it into a Directory, and populate it on the ExecuteProcessResult.
The steps to do this are, roughly:
The text was updated successfully, but these errors were encountered: