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

Introduce --local_resources flag #20398

Closed
wants to merge 7 commits into from
Closed

Introduce --local_resources flag #20398

wants to merge 7 commits into from

Conversation

AlessandroPatti
Copy link
Contributor

Follow up for #19839 (comment), introduce new flag that will replace all the others.

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Nov 30, 2023
@AlessandroPatti
Copy link
Contributor Author

cc @zhengwei143

fmeum
fmeum reviewed Nov 30, 2023
View reviewed changes
src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java Outdated
@@ -294,6 +294,8 @@ public boolean shouldMaterializeParamFiles() {
@Option(
name = "local_cpu_resources",
defaultValue = "HOST_CPUS",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to keep using these variables in --local_resources? That would be worth documenting in the flag's description.

Also, what is going to happen to the default values for cpu and memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to keep using these variables in --local_resources? That would be worth documenting in the flag's description.

Aah, good point. I've refactored a bit the converter to support both integers and doubles and added support for HOST_CPUS and HOST_RAM

Also, what is going to happen to the default values for cpu and memory?

Once --local_{cpu,ram}_resources are deprecated and removed, they should probably be moved here. I've added a comment as a reminder.

@iancha1992 iancha1992 added the team-Performance Issues for Performance teams label Nov 30, 2023
@meisterT meisterT requested a review from zhengwei143 December 1, 2023 12:28
@AlessandroPatti AlessandroPatti requested a review from a team as a code owner December 2, 2023 10:54
@AlessandroPatti AlessandroPatti force-pushed the apatti/19679/local-resources branch 2 times, most recently from a9d099b to 74225e4 Compare December 2, 2023 11:55
@AlessandroPatti AlessandroPatti force-pushed the apatti/19679/local-resources branch from 74225e4 to 620a249 Compare December 2, 2023 12:14
zhengwei143
zhengwei143 requested changes Dec 4, 2023
View reviewed changes
src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java Outdated
@@ -39,7 +41,83 @@
* <p>The supplier of the auto value, and, optionally, a max or min allowed value (inclusive), are
* passed to the constructor.
*/
public class ResourceConverter extends Converters.IntegerConverter {
public abstract class ResourceConverter<T extends Number & Comparable<T>> extends Converter.Contextless<T> {
public static final Supplier<Integer> HOST_CPUS =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Rename to HOST_CPUS_SUPPLIER. Similarly to HOST_RAM_SUPPLIER below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java Outdated
private static final ResourceConverter.DoubleConverter resource =
new ResourceConverter.DoubleConverter(
ImmutableMap.of(
"HOST_CPU", () -> (double) HOST_CPUS.get(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Refactor "HOST_CPU" and "HOST_RAM" into a static String variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/main/java/com/google/devtools/build/lib/util/RamResourceConverter.java Outdated
() -> (int) Math.ceil(LocalHostCapacity.getLocalHostCapacity().getMemoryMb())),
/* minValue= */ 0,
/* maxValue= */ Integer.MAX_VALUE);
super(ImmutableMap.of("HOST_RAM", HOST_RAM), 0, Integer.MAX_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Add /* minValue= */ comments for argument 0 since it isn't immediately obvious. Same as for all other similar changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java Show resolved Hide resolved
@AlessandroPatti AlessandroPatti requested a review from a team as a code owner December 10, 2023 13:01
@AlessandroPatti AlessandroPatti requested review from gregestren and zhengwei143 and removed request for a team December 10, 2023 13:01
zhengwei143
zhengwei143 approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@zhengwei143 zhengwei143 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AlessandroPatti
Copy link
Contributor Author

@zhengwei143 @keertk any blocker for this to land?

@zhengwei143
Copy link
Contributor

Hi, drive-by comment, I've been on vacation and will be till the start of next year - so will push this through when I'm back then.

@brentleyjones brentleyjones mentioned this pull request Jan 4, 2024
Open
@copybara-service copybara-service bot closed this in ea75928 Jan 9, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jan 9, 2024
@zhengwei143
Copy link
Contributor

@AlessandroPatti Sorry this took so long due to the holidays, thanks for your contribution!

@brentleyjones
Copy link
Contributor

@bazel-io fork 7.1.0

@bazel-io bazel-io mentioned this pull request Feb 13, 2024
Closed
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Feb 13, 2024
@AlessandroPatti @bazel-io
Introduce --local_resources flag
d06dbc4 
Follow up for bazelbuild#19839 (comment), introduce new flag that will replace all the others.

Closes bazelbuild#20398.

PiperOrigin-RevId: 596905096
Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52
@bazel-io bazel-io mentioned this pull request Feb 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 14, 2024
@bazel-io @AlessandroPatti
[7.1.0] Introduce --local_resources flag (#21331)
deb2dc2 
Follow up for
#19839 (comment),
introduce new flag that will replace all the others.

Closes #20398.

Commit
ea75928

PiperOrigin-RevId: 596905096
Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52

Co-authored-by: Alessandro Patti <[email protected]>
AlessandroPatti added a commit to AlessandroPatti/bazel that referenced this pull request Feb 19, 2024
@AlessandroPatti
Introduce --local_resources flag
aeb2ea5 
Follow up for bazelbuild#19839 (comment), introduce new flag that will replace all the others.

Closes bazelbuild#20398.

PiperOrigin-RevId: 596905096
Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52
apattidb pushed a commit to databricks/bazel that referenced this pull request Feb 20, 2024
@AlessandroPatti
Introduce --local_resources flag
aad2df6 
Follow up for bazelbuild#19839 (comment), introduce new flag that will replace all the others.

Closes bazelbuild#20398.

PiperOrigin-RevId: 596905096
Change-Id: Ic2c20dae40efa797953cde6934085f899acb9b52
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

@AlessandroPatti AlessandroPatti mentioned this pull request Mar 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum left review comments

@zhengwei143 zhengwei143 zhengwei143 approved these changes

@gregestren gregestren Awaiting requested review from gregestren gregestren is a code owner automatically assigned from bazelbuild/configurability

Assignees
No one assigned
Labels
team-Performance Issues for Performance teams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlessandroPatti @zhengwei143 @brentleyjones @iancha1992 @fmeum