Skip to content

Introduce --local_resources flag#20398

Closed
AlessandroPatti wants to merge 7 commits intobazelbuild:masterfrom
AlessandroPatti:apatti/19679/local-resources
Closed

Introduce --local_resources flag#20398
AlessandroPatti wants to merge 7 commits intobazelbuild:masterfrom
AlessandroPatti:apatti/19679/local-resources

Conversation

@AlessandroPatti
Copy link
Copy Markdown
Contributor

@AlessandroPatti AlessandroPatti commented Nov 30, 2023

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
Copy Markdown
Contributor Author

AlessandroPatti commented Nov 30, 2023

cc @zhengwei143

fmeum
fmeum reviewed Nov 30, 2023
View reviewed changes
Comment thread 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
Copy Markdown
Collaborator

@fmeum fmeum Nov 30, 2023

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
Copy Markdown
Contributor Author

@AlessandroPatti AlessandroPatti Dec 2, 2023

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
Comment thread src/main/java/com/google/devtools/build/lib/util/ResourceConverter.java Outdated
*/
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
Copy Markdown
Contributor

@zhengwei143 zhengwei143 Dec 4, 2023

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
Copy Markdown
Contributor Author

@AlessandroPatti AlessandroPatti Dec 10, 2023

Choose a reason for hiding this comment

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

Done

Comment thread 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
Copy Markdown
Contributor

@zhengwei143 zhengwei143 Dec 4, 2023

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
Copy Markdown
Contributor Author

@AlessandroPatti AlessandroPatti Dec 10, 2023

Choose a reason for hiding this comment

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

Done

Comment thread 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
Copy Markdown
Contributor

@zhengwei143 zhengwei143 Dec 4, 2023

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
Copy Markdown
Contributor Author

@AlessandroPatti AlessandroPatti Dec 10, 2023

Choose a reason for hiding this comment

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

Done

Comment thread src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
Comment thread src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java
@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
Copy Markdown
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
Copy Markdown
Contributor Author

AlessandroPatti commented Dec 23, 2023

@zhengwei143 @keertk any blocker for this to land?

@zhengwei143
Copy link
Copy Markdown
Contributor

zhengwei143 commented Dec 27, 2023

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
Copy Markdown
Contributor

zhengwei143 commented Jan 9, 2024

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

@brentleyjones
Copy link
Copy Markdown
Contributor

brentleyjones commented Feb 13, 2024

@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
Copy Markdown
Member

iancha1992 commented Feb 22, 2024

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
Closed
copybara-service Bot pushed a commit that referenced this pull request Oct 23, 2025
@AlessandroPatti @copybara-github
Remove deprecated local resource flags
b2cd7af 
Follow up for #20398, kill deprecated flags.

Closes #21684.

PiperOrigin-RevId: 823202836
Change-Id: Ife47d09de29139f3f95dd3817de551b41ac0920b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhengwei143 zhengwei143 zhengwei143 approved these changes

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

+1 more reviewer

@fmeum fmeum fmeum left review comments

Reviewers whose approvals may not affect merge requirements

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