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

Starlark API to allow rules and genrules to declare ResourceSets #8107

Closed

ghost wants to merge 1 commit into bazelbuild:master from unknown repository

ghost commented Apr 21, 2019

This change allows rule writers to define CPU and memory requirements for
rules. It's intended to help avoid overcommitting a build host when
invoking multithreaded or memory-intensive commands.

The approach taken was discussed both in #6477 and
https://groups.google.com/d/msg/bazel-dev/HaZuVmx6dfk/h6WnlabHCQAJ.

In a future change, TaggedRequirement could replace (and be renamed
to) ParseableRequirement.

Resolves #6477.

googlebot added the cla: yes label

aiuto added team-Starlark untriaged labels

aiuto requested a review from c-parsons

April 22, 2019 18:32

c-parsons requested review from buchgr and lberki and removed request for c-parsons

April 23, 2019 16:26

c-parsons added team-Local-Exec and removed team-Starlark labels

Contributor

c-parsons commented Apr 23, 2019

This doesn't quite belong under Starlark team IMO. The "API change" itself doesn't constitute much of a Starlark semantic change -- it simply expands the set of meaningful action execution requirements (all strings are accepted, I believe, but only some strings have meaning. this is increasing the list of strings which have meaning).

(FWIW, this change in Starlark seems fine, but I don't know about the ramifications of the rest of the code)

CC @buchgr and @lberki

Member

jin commented Apr 23, 2019

team-Local-Exec is managed by @jmmv according to https://www.bazel.build/maintainers-guide.html#team-labels

jin assigned jmmv

jmmv reviewed

View reviewed changes

Contributor

jmmv left a comment

Thanks a lot for doing this! Just today I was talking to someone and saying that this feature existed but wasn't exposed to Starlark, so this is very timely.

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java Outdated Show resolved Hide resolved

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

+                     */
+                    public ValidationException(String tagValue, String errorMsg) {
+                      super(errorMsg);
+                      this.tagValue = tagValue;

Contributor

jmmv Apr 24, 2019

Shouldn't we guarantee in some way that the error message contains the tag value?

Author

ghost Apr 24, 2019

Good question. I can go in either direction, but I'd like to point out that this is copied/pasted from ParseableRequirement.ValidationException. Similarly, the error handler in SkylarkActionFactory.java is directly influenced by the one from TestTargetProperties.java.

What's the dominant convention? (Throwers of InvalidPackageNameException don't include bad values in their error message, while throwers of EvalException sometimes do.)

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java Outdated Show resolved Hide resolved

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java Show resolved Hide resolved

src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java Outdated Show resolved Hide resolved

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

+                /** Integer validator to use with {@link ParseableRequirement} and {@link TaggedRequirement}. */
+                private static String IntegerValidator(String s) {
+                  Preconditions.checkNotNull(s);

Contributor

jmmv Apr 24, 2019

When would we expect this to be null? I think never, so it's fair to assume it's not null and let the code below crash if it is.

Author

ghost Apr 24, 2019

As a traveler in a foreign codebase, I'm totally okay with removing it, but that check predates me, fwiw. (See da21ba7.)

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java Show resolved Hide resolved

jmmv added P2 type: feature request and removed untriaged labels

Contributor

jmmv commented Apr 24, 2019

CC @sergiocampama @thomasvl


          Starlark API to allow rules to declare ResourceSets

73c807f

This change allows rule writers to define CPU and memory requirements for
rules.  It's intended to help avoid overcommitting a build host when
invoking multithreaded or memory-intensive commands.

The approach taken was discussed both in #6477 and
https://groups.google.com/d/msg/bazel-dev/HaZuVmx6dfk/h6WnlabHCQAJ.

In a future change, `TaggedRequirement` could replace (and be renamed
to) `ParseableRequirement`.

Resolves #6477.

Author

ghost commented Sep 4, 2019 •

edited by ghost

Loading

@jmmv Can we please revisit this? You provided some feedback which I responded to with some clarifying questions of my own. It'd be nice if this (or something better) were accepted into 1.0.

Contributor

laurentlb commented Oct 2, 2019

@jmmv What's the status of this?
Is there anything blocking?

Contributor

tetromino commented Mar 18, 2020

@jmmv - ping for review

Author

ghost commented Mar 18, 2020

It looks like Action Groups supersedes this? (Proposal: https://docs.google.com/document/d/1m9xLRLg09lncQTuBUMhhG_lqDQGJRYMZPejvwdBXvqo/edit#heading=h.5mcn15i0e1ch ; search for exec_properties and requires-mem.)

Author

ghost commented Mar 18, 2020

Sorry for the bogus cc; my attention is split atm. (Julie commented on the parent issue.)

Since Action Groups w/ exec properties is underway, I'm going to discard this.

ghost closed this

This pull request was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes P2 team-Local-Exec type: feature request