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.

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

UX: make --remote_download_output=minimal download the necessary files when invoking bazel run #11920

Closed
Tracked by #12665
sitaktif opened this issue Aug 11, 2020 · 17 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request

Comments

@sitaktif
Copy link
Contributor

Description of the problem / feature request:

When using remote build exection, calling bazel run sometarget --remote_download_output=minimal doesn't make sense: Bazel will not dowload all the files required to run sometarget.

I propose changing the behaviour of --remote_download_output=minimal to download files needed to run an action that is called with bazel run.

Feature requests: what underlying problem are you trying to solve with this feature?

I'm trying to improve the user experience of Bazel.

Not downloading the files needed to run a target passed to bazel run is never something that a user wants to do.

Any other information, logs, or outputs that you want to share?

I think the behavior would still be consistent with the meaning of --remote_download_output=minimal. The documentation says:

If set to 'minimal' doesn't download any remote build outputs to the local machine, except the ones required by local actions.

I believe it is fair to consider sometarget a local action when calling bazel run sometarget.

@sitaktif
Copy link
Contributor Author

In #12015 (comment), @allada said:

I suspect the problem is that you need the runfiles and minimal does not download runfiles by default.

Try adding:
--build_runfile_links to the end of the cmdline.

@sitaktif
Copy link
Contributor Author

Though the documentation says that the --build_runfile_links flag is true by default, and I can't seem to spot any other flag that would disable it.

@allada
Copy link
Contributor

allada commented Aug 28, 2020

I think the problem here is that local action in bazel means an action that runs during the execution phase. From my understanding the actual binary executed from a bazel run is not considered a local action. Minimal is mostly for CI in cases where everything is remote or for pipelines that one phase might heat up the cache for another pipeline action that might download the items from the CAS.

Your use case toplevel should do exactly what you want.

We had a case where we execute ~50k jobs + ~2k tests remotely and nothing locally. The problem is that we needed the undeclared_outputs of the tests so we could process some of the stats that our test harness creates, but minimal won't download these, so we use toplevel + --nobuild_runfile_links to accomplish our goal, but in your case I think toplevel gives exactly what you want.

@ash2k
Copy link
Contributor

ash2k commented Aug 28, 2020

Minimal is mostly for CI in cases where everything is remote or for pipelines that one phase might heat up the cache for another pipeline action that might download the items from the CAS.

I'm using bazel run after bazel test to build and push docker images (rules_docker) from CI. I think it'd be good to unbreak the run command with minimal because for test we'd avoid downloading stuff that happens with toplevel. I haven't measured the difference, but it's project specific too. Might be significant. Does that make sense?

@allada
Copy link
Contributor

allada commented Aug 30, 2020

I see, but since you are running bazel test first, why not use:

bazel test --remote_download_output=minimal
...
bazel run --remote_download_output=toplevel

You can do this with automatically in a .bazelrc file. The down side is you'll probably need to do another analysis phase between test and run, but this is going to be unavoidable even if the bazel team fixed it.

@sitaktif
Copy link
Contributor Author

I explicitly flagged this issue as a UX one because I don't believe it makes sense to let people run commands that will inherently not work.

If bazel run with --remote_download_output=minimal doesn't make sense, bazel would be better either erroring out or promoting the flag to toplevel.

@ash2k
Copy link
Contributor

ash2k commented Sep 1, 2020

Thanks @allada, adding the following to my .bazelrc for CI worked (for anyone who finds this issue here):

build --remote_download_minimal
run --remote_download_outputs=toplevel

I agree with what the issue description says - the current behavior is confusing for me, the end user. If, for whatever reason, it will not be fixed, it'd be great to at least detect this situation and explain how to fix it rather than just let the build fail with "file not found" errors.

Thank you for helping to understand what is going on and helping to fix it.

@tommyknows
Copy link

i just ran into this too, and had to set run --remote_download_outputs=all, as we are pushing different artifacts (like docker images) through a single runnable target with multirun. If one specifies remote_download_toplevel, multirun is available, but the docker images that it should push are not (so the runfiles are missing).
This can be mitigated by explicitly building these runfiles first, but that feels weird from a UX perspective.

I would have expected Bazel to download all runfiles for a runnable target.

To me, "minimal" means "only what is required", and not "nothing". and for a bazel run action, the runnable target + all runfiles are required for it to work.

(I do understand the explanation with the local action, but the UX is suboptimal here, especially for beginner users.)

@allada
Copy link
Contributor

allada commented Feb 5, 2021

@tommyknows, I have seen this happen before when we had a custom rule that was not properly declaring outputs. You may want to look into ensuring that if you are using custom rules to ensure you are using DefaultInfo properly and possibly providers. We had to do some logic like:

data_runfiles = ctx.runfiles(files = [...])
if ctx.attr.data:
  for item in ctx.attr.data:
    data_runfiles = data_runfiles.merge(ctx.runfiles(files = item[DefaultInfo].files.to_list()))
    data_runfiles = data_runfiles.merge(item[DefaultInfo].data_runfiles)
    ...
return [DefaultInfo(files = ..., data_runfiles = data_runfiles)]

It took us quite some time to get this figured out, but at the end of the day doing some fancy DefaultInfo-foo solved the problem. Was it the right way? Not sure, but it did allow us to not have these problems any more.

@rmohr
Copy link

rmohr commented Mar 15, 2021

With --remote_dowload_output=toplevel, I still see issues with run commands on 3.7.2.

If I have a genrule like this:

genrule(
    name = "build-dump",
    srcs = [
        "//cmd/dump",
    ],
    outs = ["dump-copier"],
    cmd = "echo '#!/bin/sh\n\ncp $(SRCS) $$1' > \"$@\"",
    executable = 1,
)

I would have expected that srcs is pulled from the cache. Instead the genrule fails on the dump binary not being copied. Is that expected too?

@jfirebaugh
Copy link

I'm encountering the same behavior as described by @tommyknows and @rmohr: bazel run --remote_download_toplevel <target> does not download all the runfile dependencies of the given target.

@brentleyjones
Copy link
Contributor

@coeuvre how hard would this be to get fixed?

@coeuvre
Copy link
Member

coeuvre commented Oct 6, 2022

IIUC, there are two problems here:

  1. For toplevel mode, runfiles of toplevel targets are not downloaded.
  2. For minimal mode, bazel run ... doesn't work because files needed to run the target are not downloaded.

I agree both issues should be fixed. I am looking into the solutions.

brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Oct 14, 2022
…tputs=all`. This leads to warnings if people set BwtB settings themselves. They will just have to work around bazelbuild/bazel#11920 on their own. This also allows us to immediately benefit if that is eventually fixed.
brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Oct 14, 2022
Also stop setting `run:rules_xcodeproj_generator --remote_download_outputs=all`. This leads to warnings if people set BwtB settings themselves. They will just have to work around bazelbuild/bazel#11920 on their own. This also allows us to immediately benefit if that is eventually fixed.
brentleyjones added a commit to MobileNativeFoundation/rules_xcodeproj that referenced this issue Oct 14, 2022
Also stop setting `run:rules_xcodeproj_generator
--remote_download_outputs=all`. This leads to warnings if people set
BwtB settings themselves. They will just have to work around
bazelbuild/bazel#11920 on their own. This also
allows us to immediately benefit if that is eventually fixed.
@purkhusid
Copy link

@coeuvre Any chance that this gets into 6.0?

@coeuvre
Copy link
Member

coeuvre commented Nov 9, 2022

I will try to cherry-pick this into 6.0.

@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 22, 2022
@sgowroji
Copy link
Member

@bazel-io fork 6.0.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 22, 2022
coeuvre added a commit to coeuvre/bazel that referenced this issue Nov 22, 2022
Always use `ToplevelArtifactsDownloader` when building without the bytes.

It checks the combination of current command (e.g. `build`, `run`, etc.) and download mode (e.g. `toplevel`, `minimal`) to decide whether download outputs for the toplevel targets or not.

Also in the `RunCommand`, we wait for the background downloads before checking the local filesystem.

Fixes bazelbuild#11920.

Closes bazelbuild#16545.

PiperOrigin-RevId: 487181884
Change-Id: I6b1a78a0d032d2cac8093eecf9c4d2e76f90380f
ShreeM01 pushed a commit that referenced this issue Nov 22, 2022
Always use `ToplevelArtifactsDownloader` when building without the bytes.

It checks the combination of current command (e.g. `build`, `run`, etc.) and download mode (e.g. `toplevel`, `minimal`) to decide whether download outputs for the toplevel targets or not.

Also in the `RunCommand`, we wait for the background downloads before checking the local filesystem.

Fixes #11920.

Closes #16545.

PiperOrigin-RevId: 487181884
Change-Id: I6b1a78a0d032d2cac8093eecf9c4d2e76f90380f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.