Skip to content
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

Jump to bottom

dynamic spawn strategy does not take download time into account #8647

Closed
buchgr opened this issue Jun 17, 2019 · 3 comments
Closed

dynamic spawn strategy does not take download time into account #8647

buchgr opened this issue Jun 17, 2019 · 3 comments
Assignees
@buchgr
Labels
P1 I'll work on this now. (Assignee required) team-Local-Exec Issues and PRs for the Execution (Local) team type: feature request
Milestone
Dynamic execution

Comments

@buchgr
Copy link
Contributor

buchgr commented Jun 17, 2019

The dynamic spawn strategy does not take downloading of output files into account when being used with remote execution but will only take cache hit/miss + execution time into account.

We'll need to change the SpawnRunner interface to differentiate between generation of action outputs (execution + download) and moving the outputs into the correct place.

Related #8646
@buchgr buchgr mentioned this issue Jun 17, 2019
Closed
@jmmv
Copy link
Contributor

jmmv commented Jun 17, 2019

I think what's proposed in #7818 would do the trick: download remote outputs into temporary files so that we don't "block" the output tree (preventing the local action from running) and then, only after all files have been downloaded, lock the output tree and move them in place.

This requires changing the remote strategies, not the dynamic scheduler, but I think it's a worthwhile change in general (e.g. to avoid overwriting local data unless we know we have the full new version available).

@jmmv jmmv added this to the Dynamic execution milestone Jun 17, 2019
@jmmv jmmv added P1 I'll work on this now. (Assignee required) team-Local-Exec Issues and PRs for the Execution (Local) team type: feature request labels Jun 17, 2019
bazel-io pushed a commit that referenced this issue Jun 19, 2019
@buchgr @copybara-github
remote: make the dynamic spawn scheduler work. Fixes #8646
d75b6cf 
This change fixes the correctness issue of dynamic spawn scheduler when being used with remote execution. See #8646 for more details.

There's a performance issue remaining: #8647

Closes #8648.

PiperOrigin-RevId: 253998300
siberex pushed a commit to siberex/bazel that referenced this issue Jul 4, 2019
@buchgr @siberex
remote: make the dynamic spawn scheduler work. Fixes bazelbuild#8646
f85d589 
This change fixes the correctness issue of dynamic spawn scheduler when being used with remote execution. See bazelbuild#8646 for more details.

There's a performance issue remaining: bazelbuild#8647

Closes bazelbuild#8648.

PiperOrigin-RevId: 253998300
irengrig pushed a commit to irengrig/bazel that referenced this issue Jul 15, 2019
@buchgr @irengrig
remote: make the dynamic spawn scheduler work. Fixes bazelbuild#8646
56de8db 
This change fixes the correctness issue of dynamic spawn scheduler when being used with remote execution. See bazelbuild#8646 for more details.

There's a performance issue remaining: bazelbuild#8647

Closes bazelbuild#8648.

PiperOrigin-RevId: 253998300
@jmmv
Copy link
Contributor

jmmv commented Nov 20, 2019

@buchgr, I think this was already done?

@buchgr
Copy link
Contributor Author

buchgr commented Nov 20, 2019

yep

@buchgr buchgr closed this as completed Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@buchgr buchgr

Labels
P1 I'll work on this now. (Assignee required) team-Local-Exec Issues and PRs for the Execution (Local) team type: feature request
Projects
None yet
Milestone
Dynamic execution
Development

No branches or pull requests
2 participants
@jmmv @buchgr