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

Conditionally set output_paths based on Remote Executor capabilities #18270

Closed
wants to merge 1 commit into from
Closed

Conditionally set output_paths based on Remote Executor capabilities #18270

wants to merge 1 commit into from

Conversation

sluongng
Copy link
Contributor

@sluongng sluongng commented Apr 29, 2023 •
edited
Loading

This is a follow up to #18269, toward the discussion in #18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.

@sluongng sluongng changed the title sluongng/cond set output paths Conditionally set output_paths based on Remote Executor capabilities Apr 29, 2023
@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch 3 times, most recently from b7de57a to 006d6f4 Compare May 1, 2023 10:49
@sluongng
Copy link
Contributor Author

sluongng commented May 1, 2023

It might be interesting for subsequent patches to add the ability to set RemoteWorker's ApiVersion via a flag.
That should enable us to write more integration tests for different version behaviors.

However, that's out of the scope of the current PR.

@sluongng sluongng marked this pull request as ready for review May 2, 2023 09:31
@sluongng sluongng requested a review from a team as a code owner May 2, 2023 09:31
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team labels May 2, 2023
@sluongng sluongng mentioned this pull request May 2, 2023
Closed
@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch 4 times, most recently from db79728 to 2c78242 Compare May 15, 2023 10:43
@sluongng
Copy link
Contributor Author

cc: @coeuvre @tjgq @EdSchouten @fmeum

This is probably the biggest and final change in my PR chain about output_paths and output_symlinks.
Do you guys think this is backward compatible enough that it could go into 6.3.0 or do we have to wait for 7.0.0.

@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch 2 times, most recently from 5dac526 to f792e37 Compare May 30, 2023 10:17
@sluongng
Copy link
Contributor Author

ping @coeuvre @tjgq I would appreciate getting some feedback here.

This PR has been out for a month now. The longer it's not merged, the more effort is required to rebase it against conflicts in master.

coeuvre
coeuvre approved these changes Jun 5, 2023
View reviewed changes
Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. LGTM!

@coeuvre coeuvre added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jun 5, 2023
@coeuvre
Copy link
Member

coeuvre commented Jun 5, 2023

@bazel-io fork

@coeuvre
Copy link
Member

coeuvre commented Jun 5, 2023

@bazel-io fork 6.3.0

@bazel-io bazel-io mentioned this pull request Jun 5, 2023
Closed
@copybara-service copybara-service bot closed this in 50c16b3 Jun 5, 2023
@Pavank1992 Pavank1992 removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jun 5, 2023
@coeuvre
Copy link
Member

coeuvre commented Jun 5, 2023

This change breaks remote server that only support REAPI v2.0 (because the change of current). It is bing rolled back.

@coeuvre coeuvre reopened this Jun 5, 2023
@sluongng
Copy link
Contributor Author

sluongng commented Jun 5, 2023

@coeuvre any advice on how I should fix the issue?

I am not sure how current is being used as we do alter the implementation based on Remote Exec server capabilities.
All the tests also pass using RBE service on CI, which I assumed are 2.0 only?

@coeuvre
Copy link
Member

coeuvre commented Jun 5, 2023

The tests are using remote worker instead RBE so we didnt' catch the regression.

current is used to check against remote server's low and high version. Simply increasing it to 2.1 will fail the check for server whose high is 2.0.

Ideally, Bazel should also has the concept of low and high version, and compare it against server's low and high version. If there is an intersection, use highest version in that range - maybe call it resolved version. All the feature test later in the code path should then depend on the resolved version.

coeuvre
coeuvre requested changes Aug 12, 2024
View reviewed changes
Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

Thanks for the update! I left some comments but otherwise LGTM.

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/remote/common/RemoteCacheClient.java Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java Outdated Show resolved Hide resolved
src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java Outdated Show resolved Hide resolved
@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch from 2d7f14f to c311c5e Compare September 10, 2024 15:41
@sluongng sluongng mentioned this pull request Sep 10, 2024
Closed
@sluongng
Copy link
Contributor Author

@coeuvre sorry for the slow turnaround. I have rebased the PR on top of the latest master and resolved all the merge conflicts as well as addressed the review comment.

Let me know what you think.

@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch from c311c5e to da30780 Compare September 11, 2024 10:46
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 11, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
e63c6ed 
Follow up of bazelbuild#23582 and bazelbuild#18270
@sluongng sluongng mentioned this pull request Sep 11, 2024
Closed
@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch from da30780 to e705eb0 Compare September 11, 2024 12:52
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 11, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
bd8db3e 
Follow up of bazelbuild#23582 and bazelbuild#18270
@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch from e705eb0 to 1bb3bf6 Compare September 11, 2024 13:19
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 11, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
2d9e163 
Follow up of bazelbuild#23582 and bazelbuild#18270
@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch from 1bb3bf6 to c577870 Compare September 11, 2024 13:41
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 11, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
3a5c281 
Follow up of bazelbuild#23582 and bazelbuild#18270
@sluongng
Copy link
Contributor Author

@coeuvre This finally passed CI. You might want to give this another read as the rebase came with some adjustments for the latest changes in master branch. 🙇

sluongng added a commit to sluongng/bazel that referenced this pull request Sep 12, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
062b4db 
Follow up of bazelbuild#23582 and bazelbuild#18270
coeuvre
coeuvre approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

LGTM! Can you rebase again? I will do the import then.

@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch from c577870 to fcf158b Compare September 12, 2024 12:56
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 12, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
cef46be 
Follow up of bazelbuild#23582 and bazelbuild#18270
@sluongng sluongng force-pushed the sluongng/cond-set-output-paths branch from fcf158b to 5117612 Compare September 12, 2024 13:39
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 12, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
7b5729f 
Follow up of bazelbuild#23582 and bazelbuild#18270
@sluongng
Copy link
Contributor Author

Rebased. PTAL 🙏

coeuvre
coeuvre approved these changes Sep 12, 2024
View reviewed changes
Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

Thanks! Importing.

@fmeum
Copy link
Collaborator

fmeum commented Sep 16, 2024

@bazel-io fork 7.4.0

@bazel-io bazel-io mentioned this pull request Sep 16, 2024
Closed
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 16, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
b7524f9 
Follow up of bazelbuild#23582 and bazelbuild#18270
sluongng added a commit to sluongng/bazel that referenced this pull request Sep 16, 2024
@sluongng
RemoteCache: replace getCacheCapabilities with getServerCapabilities
883a745 
Follow up of bazelbuild#23582 and bazelbuild#18270
copybara-service bot pushed a commit that referenced this pull request Sep 17, 2024
@sluongng @copybara-github
RemoteCache: replace getCacheCapabilities with getServerCapabilities
858b30c 
Follow up of #23582 and #18270

Closes #23593.

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

@EdSchouten EdSchouten EdSchouten left review comments

@coeuvre coeuvre coeuvre approved these changes

@tjgq tjgq Awaiting requested review from tjgq

Assignees
No one assigned
Labels
awaiting-user-response Awaiting a response from the author team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sluongng @coeuvre @fmeum @EdSchouten @tjgq @sgowroji @Pavank1992