Add a flag to force Bazel to download certain artifacts when using --remote_download_minimal by lfpino · Pull Request #15638 · bazelbuild/bazel

lfpino · 2022-06-08T07:45:20Z

When using --remote_download_minimal Bazel only downloads to the client the minimum number of artifacts necessary for the build to succeed. This can be sometimes problematic when local development (i.e. IDE) requires some artifacts to do local work (e.g. syntax completion). We add a flag --experimental_remote_download_regex that allow specifying a regular expression that will force certain artifacts to be forced to downloaded.

Tested locally that syntax errors disappear in IntelliJ when compiling Bazel itself with --remote_download_minimal and --experimental_remote_download_regex=".*jar$", also included an integration test with several cases.

Signed-off-by: Luis Fernando Pino Duque luis@engflow.com

When using --remote_download_minimal Bazel only downloads to the client the minimum number of artifacts necessary for the build to succeed. This can be sometimes problematic when local development (i.e. IDE) requires some artifacts to do local work (e.g. syntax completion). We add a flag --experimental_force_downloads_regex that allow specifying a regular expression that will force certain artifacts to be forced to downloaded. Tested locally that syntax errors disappear in IntelliJ when compiling Bazel itself with --remote_download_minimal and --experimental_force_downloads_regex=".*jar$", also included an integration test with several cases. Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

coeuvre

Thanks for your PR!

I think this is working towards the point 3 I mentioned in #12665.

The new flag works on output-wise, i.e. filter each output of a spawn. I didn't look into IDE usages yet, but since we are on the topic, I want to learn more about the requirements. Have you consider other options, like, filter at spawn level (that's how --remote_download_toplevel works) or use an aspect with output group?

coeuvre · 2022-06-08T13:43:13Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

+    String regex = remoteOptions.experimentalForceDownloadsRegex;
+    // TODO(bazel-team): Consider adding a warning or more validation if the experimentalForceDownloadsRegex is used
+    // without RemoteOutputsMode.MINIMAL
+    this.shouldForceDownloads = !regex.isEmpty() && remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL;


I think we can also use this flag for --remote_download_toplevel.

coeuvre · 2022-06-08T13:43:21Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

            /* exitCode = */ result.getExitCode(),
            hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload));

+    ImmutableList.Builder<ListenableFuture<FileMetadata>> forcedDownloadsBuilder = ImmutableList.builder();


why not reuse downloadsBuilder?

I was trying to steer away from the current downloads codepath since I'm not an expert in this part of the code. Wanted to make sure this change is isolated to avoid any potential problems but if you want I can try to unify them using downloadsBuilder.

Yes, please unify them.

I tried to unify them in a few ways but I always ended up finding issues and making the code more unreadable. Do you have any suggestions? Since this is experimental I'm inclined to leave it as it is right now and if we can confirm this is a good direction then we can refactor the code to make it more readable (I still find it very confusing the code path that depends on the downloadOutputs boolean)

downloadOutputs basically means whether we are using BwoB. The two mods were merged into one function because eventually we want to only have BwoB (but the behaviour is still the same by default that Bazel download ALL outputs but in the background, so that the boolean is removed and only one code path left).

As you said this is an experimental feature, I am fine to move forward without unifying them at this PR if that introduce other issues.

Yeah, I understood that, but some tests are failing when I'm unifying them and it's not entirely clear to me why that's the case. Seems like a bug somewhere where downloadOutputs is not entirely consistent with using BwoB. I'll go ahead as is.

coeuvre · 2022-06-08T13:45:10Z

src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java

  public int maximumOpenFiles;

+  @Option(
+          name = "experimental_force_downloads_regex",


How about name this flag --experimental_remote_download_regex to match other flags?

lfpino

Thanks for your PR!

I think this is working towards the point 3 I mentioned in #12665.

The new flag works on output-wise, i.e. filter each output of a spawn. I didn't look into IDE usages yet, but since we are on the topic, I want to learn more about the requirements. Have you consider other options, like, filter at spawn level (that's how --remote_download_toplevel works) or use an aspect with output group?

You're welcome! :-) and thanks for the prompt reply!

Yeah, I hadn't seen #12665 but this does contribute towards that goal.

The requirement is quite simple: Being able to use build without the bytes without breaking IDE integration. By using this flag, one can use IntelliJ with --remote_download_minimal and ask Bazel to also download the jar files.

I haven't explored any other options since, as usual, there's too many ways of doing this and I tried something that made sense when I looked at the code. Of course if you think that this is not ok architecturally I'm happy to look at other options but I'll probably need a couple of code pointers. If you think this approach is ok, I think we should merge and improve this with some follow-up PRs. WDYT?

lfpino · 2022-06-09T21:05:44Z

src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java

  public int maximumOpenFiles;

+  @Option(
+          name = "experimental_force_downloads_regex",


lfpino · 2022-06-09T21:06:51Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

            /* exitCode = */ result.getExitCode(),
            hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload));

+    ImmutableList.Builder<ListenableFuture<FileMetadata>> forcedDownloadsBuilder = ImmutableList.builder();


I was trying to steer away from the current downloads codepath since I'm not an expert in this part of the code. Wanted to make sure this change is isolated to avoid any potential problems but if you want I can try to unify them using downloadsBuilder.

lfpino · 2022-06-09T21:09:54Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

+    String regex = remoteOptions.experimentalForceDownloadsRegex;
+    // TODO(bazel-team): Consider adding a warning or more validation if the experimentalForceDownloadsRegex is used
+    // without RemoteOutputsMode.MINIMAL
+    this.shouldForceDownloads = !regex.isEmpty() && remoteOptions.remoteOutputsMode == RemoteOutputsMode.MINIMAL;


coeuvre

It seems that the new changes haven't been pushed.

coeuvre · 2022-06-10T09:03:53Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

            /* exitCode = */ result.getExitCode(),
            hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload));

+    ImmutableList.Builder<ListenableFuture<FileMetadata>> forcedDownloadsBuilder = ImmutableList.builder();


Yes, please unify them.

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino

Addressed comments PTAL

lfpino · 2022-06-14T21:40:26Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

            /* exitCode = */ result.getExitCode(),
            hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload));

+    ImmutableList.Builder<ListenableFuture<FileMetadata>> forcedDownloadsBuilder = ImmutableList.builder();


I tried to unify them in a few ways but I always ended up finding issues and making the code more unreadable. Do you have any suggestions? Since this is experimental I'm inclined to leave it as it is right now and if we can confirm this is a good direction then we can refactor the code to make it more readable (I still find it very confusing the code path that depends on the downloadOutputs boolean)

coeuvre

Sorry for the delay. I think this PR is good now. Just address remaining comments and we are ready to merge.

coeuvre · 2022-06-24T11:49:07Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

            /* exitCode = */ result.getExitCode(),
            hasFilesToDownload(action.getSpawn().getOutputFiles(), filesToDownload));

+    ImmutableList.Builder<ListenableFuture<FileMetadata>> forcedDownloadsBuilder = ImmutableList.builder();


downloadOutputs basically means whether we are using BwoB. The two mods were merged into one function because eventually we want to only have BwoB (but the behaviour is still the same by default that Bazel download ALL outputs but in the background, so that the boolean is removed and only one code path left).

As you said this is an experimental feature, I am fine to move forward without unifying them at this PR if that introduce other issues.

coeuvre · 2022-06-24T11:52:04Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

    }

+    ImmutableList<ListenableFuture<FileMetadata>> forcedDownloads = forcedDownloadsBuilder.build();
+    try (SilentCloseable c = Profiler.instance().profile("Remote.forcedDownload")) {


Consider extracting this into a function to share the code above.

@coeuvre unfortunately the generic Exception catch here is making this code very brittle. Exception handling needs to be improved before we can move this to a generic function, I've added a TODO to avoid mixing things up. That was causing the test failures from my previous attempts.

coeuvre · 2022-06-24T11:52:45Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

    tmpOutErr.clearOut();
    tmpOutErr.clearErr();

+    if (!forcedDownloads.isEmpty()) {


Consider moving this into else block below to make it clear that this is only available in BwoB.

coeuvre · 2022-06-24T11:53:10Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

    this.scheduler = Schedulers.from(executor, /*interruptibleWorker=*/ true);
+
+    String regex = remoteOptions.remoteDownloadRegex;
+    // TODO(bazel-team): Consider adding a warning or more validation if the experimentalForceDownloadsRegex is used


Comment needs update.

…wnloads Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino · 2022-07-05T22:16:35Z

@coeuvre this PR should be ready now, PTAL!

lfpino · 2022-07-06T00:04:18Z

There are some failures, let me check that first.

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino · 2022-07-08T22:53:49Z

@coeuvre you should be able to take a look now

coeuvre

LGTM! I will import after following comment is addressed.

coeuvre · 2022-07-11T08:13:24Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

    return null;
  }

+//  private void waitForDownloads(ImmutableList<ListenableFuture<FileMetadata>> downloads,


Please remove commented code.

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino · 2022-07-12T15:07:37Z

Done. Feel free to merge @coeuvre

Thanks a lot!

brentleyjones · 2022-07-13T13:30:36Z

@bazel-io flag

sgowroji · 2022-07-13T16:04:59Z

@bazel-io fork 5.3.0

…remote_download_minimal When using --remote_download_minimal Bazel only downloads to the client the minimum number of artifacts necessary for the build to succeed. This can be sometimes problematic when local development (i.e. IDE) requires some artifacts to do local work (e.g. syntax completion). We add a flag --experimental_remote_download_regex that allow specifying a regular expression that will force certain artifacts to be forced to downloaded. Tested locally that syntax errors disappear in IntelliJ when compiling Bazel itself with --remote_download_minimal and --experimental_remote_download_regex=".*jar$", also included an integration test with several cases. Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com> Closes bazelbuild#15638. PiperOrigin-RevId: 460672954 Change-Id: I3a4f18d046d2d91603a276f16173ad2a253480dd

…remote_download_minimal (#15870) When using --remote_download_minimal Bazel only downloads to the client the minimum number of artifacts necessary for the build to succeed. This can be sometimes problematic when local development (i.e. IDE) requires some artifacts to do local work (e.g. syntax completion). We add a flag --experimental_remote_download_regex that allow specifying a regular expression that will force certain artifacts to be forced to downloaded. Tested locally that syntax errors disappear in IntelliJ when compiling Bazel itself with --remote_download_minimal and --experimental_remote_download_regex=".*jar$", also included an integration test with several cases. Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com> Closes #15638. PiperOrigin-RevId: 460672954 Change-Id: I3a4f18d046d2d91603a276f16173ad2a253480dd Co-authored-by: Luis Fernando Pino Duque <luis@engflow.com>

…remote_download_minimal When using --remote_download_minimal Bazel only downloads to the client the minimum number of artifacts necessary for the build to succeed. This can be sometimes problematic when local development (i.e. IDE) requires some artifacts to do local work (e.g. syntax completion). We add a flag --experimental_remote_download_regex that allow specifying a regular expression that will force certain artifacts to be forced to downloaded. Tested locally that syntax errors disappear in IntelliJ when compiling Bazel itself with --remote_download_minimal and --experimental_remote_download_regex=".*jar$", also included an integration test with several cases. Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com> Closes bazelbuild#15638. PiperOrigin-RevId: 460672954 Change-Id: I3a4f18d046d2d91603a276f16173ad2a253480dd

lfpino requested a review from a team as a code owner June 8, 2022 07:45

sgowroji added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Jun 8, 2022

meisterT requested a review from coeuvre June 8, 2022 11:14

coeuvre requested changes Jun 8, 2022

View reviewed changes

lfpino commented Jun 9, 2022

View reviewed changes

lfpino requested a review from coeuvre June 9, 2022 21:14

coeuvre reviewed Jun 10, 2022

View reviewed changes

lfpino added 3 commits June 14, 2022 13:38

Address most comments

1fa3b54

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

Unify builders

1e6ea20

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

Revert and push green version

01a26a4

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino commented Jun 14, 2022

View reviewed changes

lfpino requested a review from coeuvre June 14, 2022 21:40

lfpino mentioned this pull request Jun 14, 2022

Add a flag to force Bazel to download certain artifacts when using --remote_download_minimal #15676

Closed

coeuvre reviewed Jun 24, 2022

View reviewed changes

lfpino added 4 commits July 5, 2022 16:41

Merge https://github.com/bazelbuild/bazel into lfpino-bazel-forced-do…

eefb0cd

…wnloads Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

Move tests

ecc33b9

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

Address comments

4fef106

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

Address comments bazelbuild#2

987d387

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino added 2 commits July 7, 2022 21:09

Debug

fdf3b25

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

Address comments bazelbuild#3

df6de24

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino requested a review from coeuvre July 8, 2022 22:53

coeuvre approved these changes Jul 11, 2022

View reviewed changes

Address comments

73fd192

Signed-off-by: Luis Fernando Pino Duque <luis@engflow.com>

lfpino requested a review from coeuvre July 12, 2022 15:07

copybara-service bot closed this in 3eeae23 Jul 13, 2022

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 13, 2022

bazel-io mentioned this pull request Jul 13, 2022

[5.3.0] Add a flag to force Bazel to download certain artifacts when using --remote_download_minimal #15869

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 13, 2022

sgowroji mentioned this pull request Jul 13, 2022

Add a flag to force Bazel to download certain artifacts when using --… #15870

Merged

ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022

meisterT mentioned this pull request Apr 5, 2023

Mark "Remote Output Service..." as approved. bazelbuild/proposals#311

Merged

arunkumar9t2 mentioned this pull request Jun 26, 2023

[WIP] Enable populating java/kotlin/android artifacts for darwin (M1) from linux #18669

Closed

Conversation

lfpino commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coeuvre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfpino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coeuvre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfpino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coeuvre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfpino commented Jul 5, 2022

Uh oh!

lfpino commented Jul 6, 2022

Uh oh!

lfpino commented Jul 8, 2022

Uh oh!

coeuvre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfpino commented Jul 12, 2022

Uh oh!

brentleyjones commented Jul 13, 2022

Uh oh!

lfpino commented Jun 8, 2022 •

edited

Loading