Implement extra resources for test actions #11963

exoson · 2020-08-18T11:36:33Z

Adds support for specifying extra resources available for the bazel
invocation and requirement for test actions. Using
--local_extra_resources=<resource name>=<amount of resource> one can
specify the resources available for tests. Specifying a tag
"resources:<resource name>:<amount of resources>" for a test action will
tell bazel how much of the said resource this test will require. Bazel
will then limit concurrently running test actions using the same
resource if there isn't enough of said resource available.

This can be used to arbitrate access to additional resources such as GPUs or embedded developer boards.

googlebot · 2020-08-18T11:36:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Adds support for specifying extra resources available for the bazel invocation and requirement for test actions. Using --local_extra_resources=<resource name>=<amount of resource> one can specify the resources available for tests. Specifying a tag "resources:<resource name>:<amount of resources>" for a test action will tell bazel how much of the said resource this test will require. Bazel will then limit concurrently running test actions using the same resource if there isn't enough of said resource available.

googlebot · 2020-08-18T11:37:42Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

philwo · 2020-10-21T12:08:40Z

I think this is pretty cool. :)

EricCousineau-TRI · 2021-02-26T15:05:17Z

Does this relate #6477 at all?
EDIT: Ah, prolly not. That issue's for genrule things.

exoson · 2021-02-26T18:21:53Z

Does this relate #6477 at all?
EDIT: Ah, prolly not. That issue's for genrule things.

It seems to me that these are quite similar in the things they achieve. Or at least some of the proposed things.

gregestren · 2021-05-10T20:36:51Z

This PR has been sitting around for a while and deserves proper attention, apologies.

I don't have enough expertise to feel I know how to review this well. But I should be able to route it to the right place. I'm going to chat with people to get this moving. Stay tuned.

meisterT · 2021-05-14T06:43:19Z

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java

+      effectTags = {OptionEffectTag.UNKNOWN},
+      allowMultiple = true,
+      help =
+          "Explicitly set the number of extra recourses available to Bazel. "


nit: typo in resources

meisterT · 2021-05-14T06:44:10Z

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java

+            + "types of extra resources. Bazel will limit concurrently running test actions "
+            + "based on the available extra resources and the extra resources required "
+            + "by the test actions. "
+            + "Note: This is a no-op if --local_resources is set.",


I wonder whether Bazel should fail when both are specified.

Doesn't matter, --local_resources has been removed since the revision this is based on.

larsrc-google · 2021-05-14T07:56:05Z

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

+  /** How many extra resources an action requires for execution. */
+  public static final ParseableRequirement RESOURCES =
+      ParseableRequirement.create(
+          "resources:<str>:<int>",


Why an int rather than a float? CPU resources are already floats.

This is just to match the cpu:<int> code above. Looks like there is some inconsistency around what can be a float and what can be an int:

Execution requirements: src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java parses int.

Test tags: src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java parses Float.

Command line flags: src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java parses int or a Float in the expression multiplier:

/** Description of the accepted inputs to the converter. */ public static final String FLAG_SYNTAX = "an integer, or a keyword (\"auto\", \"HOST_CPUS\", \"HOST_RAM\"), optionally followed by " + "an operation ([-|*]<float>) eg. \"auto\", \"HOST_CPUS*.5\"";

In its current form, this change is matching the existing code: integers in execution requirements, floats in test tags, and integer on the command line. But I'm open to suggestions if we want to use slightly different conventions than what is used for CPU resources.

larsrc-google · 2021-05-14T07:56:42Z

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

+
+            // De-and-reserialize & compare to only allow canonical integer formats.
+            if (!Integer.toString(value).equals(resourceCount)) {
+              return "must be in canonical format (e.g. '4' instead of '+04')";


Why must it be in canonical format?

We were just trying to match behavior of the cpu:N tag validation above. Perhaps there's a reason why that validation was originally added for the cpu:N tag...?

larsrc-google · 2021-05-14T07:57:21Z

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

+              return "can't be zero or negative";
+            }
+
+            return null;


You might want to return value here?

This is a validator lambda that should return null if validation passes.

larsrc-google · 2021-05-14T08:00:32Z

src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java

@@ -322,6 +343,13 @@ private synchronized CountDownLatch acquire(ResourceSet resources) {
  private synchronized boolean release(ResourceSet resources) {
    usedCpu -= resources.getCpuUsage();
    usedRam -= resources.getMemoryMb();
+
+    for (Map.Entry<String, Float> resource : resources.getExtraResourceUsage().entrySet()) {


This would look nicer as a forEach().

larsrc-google · 2021-05-14T08:01:06Z

src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java

@@ -333,6 +361,19 @@ private synchronized boolean release(ResourceSet resources) {
    if (usedRam < epsilon) {
      usedRam = 0;
    }
+
+    Set<String> toRemove = new HashSet<>();
+    for (Map.Entry<String, Float> resource : usedExtraResources.entrySet()) {


You can also use forEach here.

larsrc-google · 2021-05-14T08:17:06Z

src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java

@@ -150,23 +151,21 @@ public ResourceSet getLocalResourceUsage(Label label, boolean usingLocalTestJobs
    ResourceSet testResourcesFromSize = TestTargetProperties.getResourceSetFromSize(size);

    // Tests can override their CPU reservation with a "cpus:<n>" tag.


These comments are a sign that the relevant portions would prefer to live in separate functions.

larsrc-google · 2021-05-14T08:18:36Z

src/main/java/com/google/devtools/common/options/Converters.java

+
+    @Override
+    public Map.Entry<String, Float> convert(String input) throws OptionsParsingException {
+      int pos = input.indexOf("=");


Just use a static Splitter instead of hand-coding. Alternatively, make this a subclass of AssignmentConverter. Or even better, make AssignmentConverter a generic method with a converter lambda.

larsrc-google · 2021-05-14T08:41:55Z

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java

+            + "types of extra resources. Bazel will limit concurrently running test actions "
+            + "based on the available extra resources and the extra resources required "
+            + "by the test actions. "
+            + "Note: This is a no-op if --local_resources is set.",


Doesn't matter, --local_resources has been removed since the revision this is based on.

larsrc-google · 2021-05-14T08:42:33Z

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java

+      effectTags = {OptionEffectTag.UNKNOWN},
+      allowMultiple = true,
+      help =
+          "Explicitly set the number of extra recourses available to Bazel. "


You may want to mention that CPU, RAM, and tests cannot be specified here.

larsrc-google · 2021-05-14T08:44:56Z

src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java

@@ -419,7 +419,7 @@ private Spawn newCustomSpawn(String mnemonic, ImmutableMap<String, String> execu
        executionInfo,
        EmptyRunfilesSupplier.INSTANCE,
        action,
-        ResourceSet.create(1, 0, 0));
+        ResourceSet.create(1, 0, null, 0));


A number of these tests could use the two-parameter overload.

Looks like many actually can't, because they also specify a nonzero localTestCount. In fact, looks like this was the only place where I could use ResourceSet.createWithRamCpu instead.

katre · 2021-09-08T15:26:25Z

Is this still under review? What's the status?

scele

@exoson is no longer working with us, I'm picking this up.

A new PR is here: #13996 - I think this one could be closed.
I've tried to address all comments from @larsrc-google in the new PR.

scele · 2021-09-10T11:02:07Z

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

+              return "can't be zero or negative";
+            }
+
+            return null;


This is a validator lambda that should return null if validation passes.

scele · 2021-09-10T11:02:30Z

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

+
+            // De-and-reserialize & compare to only allow canonical integer formats.
+            if (!Integer.toString(value).equals(resourceCount)) {
+              return "must be in canonical format (e.g. '4' instead of '+04')";


We were just trying to match behavior of the cpu:N tag validation above. Perhaps there's a reason why that validation was originally added for the cpu:N tag...?

scele · 2021-09-10T11:05:02Z

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java

+  /** How many extra resources an action requires for execution. */
+  public static final ParseableRequirement RESOURCES =
+      ParseableRequirement.create(
+          "resources:<str>:<int>",


This is just to match the cpu:<int> code above. Looks like there is some inconsistency around what can be a float and what can be an int:

Execution requirements: src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java parses int.

Test tags: src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java parses Float.

Command line flags: src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java parses int or a Float in the expression multiplier:

/** Description of the accepted inputs to the converter. */ public static final String FLAG_SYNTAX = "an integer, or a keyword (\"auto\", \"HOST_CPUS\", \"HOST_RAM\"), optionally followed by " + "an operation ([-|*]<float>) eg. \"auto\", \"HOST_CPUS*.5\"";

In its current form, this change is matching the existing code: integers in execution requirements, floats in test tags, and integer on the command line. But I'm open to suggestions if we want to use slightly different conventions than what is used for CPU resources.

scele · 2021-09-10T11:27:08Z

src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java

@@ -135,7 +159,7 @@ public ResourceSet convert(String input) throws OptionsParsingException {
        if (memoryMb <= 0.0 || cpuUsage <= 0.0) {
          throw new OptionsParsingException("All resource values must be positive");
        }
-        return create(memoryMb, cpuUsage, Integer.MAX_VALUE);
+        return create(memoryMb, cpuUsage, null, Integer.MAX_VALUE);


Do you mean supporting --local_extra_resources=foo=2,bar=4? If so, can't we already get the same behavior just with --local_extra_resources=foo=2 --local_extra_resources=bar=4?

scele · 2021-09-10T11:29:03Z

src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java

+      effectTags = {OptionEffectTag.UNKNOWN},
+      allowMultiple = true,
+      help =
+          "Explicitly set the number of extra recourses available to Bazel. "


scele · 2021-09-15T09:15:26Z

src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java

@@ -333,6 +361,19 @@ private synchronized boolean release(ResourceSet resources) {
    if (usedRam < epsilon) {
      usedRam = 0;
    }
+
+    Set<String> toRemove = new HashSet<>();
+    for (Map.Entry<String, Float> resource : usedExtraResources.entrySet()) {


scele · 2021-09-15T09:16:24Z

src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java

    this.memoryMb = memoryMb;
    this.cpuUsage = cpuUsage;
+    if (extraResourceUsage == null) {
+        this.extraResourceUsage = new HashMap<>();


In the latest version I converted the extraResourceUsage from Map to an ImmutableMap, and using ImmutableMap.of() here.

scele · 2021-09-15T09:17:07Z

src/main/java/com/google/devtools/build/lib/actions/ResourceSet.java

  /** Returns the local test count used. */
  public int getLocalTestCount() {
    return localTestCount;
  }

  @Override
  public String toString() {
+    StringBuilder sb = new StringBuilder();


scele · 2021-09-15T09:35:09Z

src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetProperties.java

@@ -150,23 +151,21 @@ public ResourceSet getLocalResourceUsage(Label label, boolean usingLocalTestJobs
    ResourceSet testResourcesFromSize = TestTargetProperties.getResourceSetFromSize(size);

    // Tests can override their CPU reservation with a "cpus:<n>" tag.


scele · 2021-09-15T09:38:26Z

src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java

@@ -419,7 +419,7 @@ private Spawn newCustomSpawn(String mnemonic, ImmutableMap<String, String> execu
        executionInfo,
        EmptyRunfilesSupplier.INSTANCE,
        action,
-        ResourceSet.create(1, 0, 0));
+        ResourceSet.create(1, 0, null, 0));


Looks like many actually can't, because they also specify a nonzero localTestCount. In fact, looks like this was the only place where I could use ResourceSet.createWithRamCpu instead.

sgowroji · 2022-04-22T07:08:11Z

We are closing this PR with respect to the comment mentioned over here #11963 (review) and also the new reference PR (#13996) . Please feel free to reach to reopen or for any further update. Thanks!

googlebot added the cla: no label Aug 18, 2020

exoson force-pushed the general_resource branch from 8f384dc to da56473 Compare August 18, 2020 11:37

googlebot added cla: yes and removed cla: no labels Aug 18, 2020

philwo requested a review from gregestren October 21, 2020 12:08

philwo assigned gregestren Oct 21, 2020

philwo added team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request labels Oct 21, 2020

meisterT reviewed May 14, 2021

View reviewed changes

meisterT requested review from larsrc-google and wilwell May 14, 2021 06:44

larsrc-google reviewed May 14, 2021

View reviewed changes

katre added team-Local-Exec Issues and PRs for the Execution (Local) team and removed team-Configurability platforms, toolchains, cquery, select(), config transitions labels Sep 8, 2021

gregestren removed their assignment Sep 8, 2021

scele mentioned this pull request Sep 15, 2021

Implement extra resources for test actions #13996

Closed

scele reviewed Sep 15, 2021

View reviewed changes

philwo force-pushed the master branch from 26cb401 to 168b89b Compare December 2, 2021 18:07

meisterT assigned larsrc-google Jan 26, 2022

sgowroji closed this Apr 22, 2022

		@@ -150,23 +151,21 @@ public ResourceSet getLocalResourceUsage(Label label, boolean usingLocalTestJobs
		ResourceSet testResourcesFromSize = TestTargetProperties.getResourceSetFromSize(size);

		// Tests can override their CPU reservation with a "cpus:<n>" tag.

Implement extra resources for test actions #11963

Implement extra resources for test actions #11963

Uh oh!

Conversation

exoson commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Aug 18, 2020

Uh oh!

googlebot commented Aug 18, 2020

Uh oh!

philwo commented Oct 21, 2020

Uh oh!

EricCousineau-TRI commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exoson commented Feb 26, 2021

Uh oh!

gregestren commented May 10, 2021

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!

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!

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!

katre commented Sep 8, 2021

Uh oh!

scele 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

exoson commented Aug 18, 2020 •

edited

Loading

EricCousineau-TRI commented Feb 26, 2021 •

edited

Loading