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.

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

RemoteExecutionService: Action.Command to set output_paths #18202

Closed

Conversation

sluongng
Copy link
Contributor

This is a follow-up to #18198

Make Bazel compatible with newer version of Remote Api by setting
output_paths along-side output_directories and output_files. The latter
2 are deprecated in newer REAPI specification.

@sluongng sluongng changed the title sluongng/command output paths RemoteExecutionService: Action.Command to set output_paths Apr 25, 2023
@sluongng sluongng force-pushed the sluongng/command-output-paths branch 2 times, most recently from e965f1e to bc0ddbb Compare April 25, 2023 16:01
@sluongng sluongng marked this pull request as ready for review April 27, 2023 08:58
@sluongng sluongng requested a review from a team as a code owner April 27, 2023 08:58
@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 Apr 27, 2023
This is a follow-up to bazelbuild#18198

Make Bazel compatible with newer version of Remote Api by setting
output_paths along-side output_directories and output_files. The latter
2 are deprecated in newer REAPI specification.
@sluongng sluongng force-pushed the sluongng/command-output-paths branch from bc0ddbb to 7dfb5b9 Compare April 27, 2023 09:04
@sluongng
Copy link
Contributor Author

cc: @fmeum @EdSchouten @tjgq

@tjgq
Copy link
Contributor

tjgq commented Apr 27, 2023

How much of a concern is it that this change increases the Action.Command message size for every action?

An alternative would be to use the high_api_version reported in the ServerCapabilities message [1] to decide which field to use. But I'm not sure if the complication is worth it.

[1] https://cs.opensource.google/bazel/bazel/+/master:third_party/remoteapis/build/bazel/remote/execution/v2/remote_execution.proto;l=1799;drc=d0cba5507fcb5d636b1a9a3b1f58cf63314781c0

@sluongng
Copy link
Contributor Author

How much of a concern is it that this change increases the Action.Command message size for every action?

An alternative would be to use the high_api_version reported in the ServerCapabilities message [1] to decide which field to use. But I'm not sure if the complication is worth it.

I have to scratch my head a bit on this.
Regarding Remote Cache / RBE implementation, most of the weight is on the CAS traffic and not on Action Cache(AC) side.
Increasing the Command message should have minimal impact.

The hard question would be when/how we deprecate the usages of the old fields.
That's when I think checking ServerSide capability would come into play:

  1. In a major LTS release (i.e. 7.0.0), create a transition period when we only set the old fields when detecting the older version of the API.
  2. In another major LTS release, remove the old field completely.

But I suspect there isn't much appetite for doing this deprecation.
Regardless, it's outside the scope of this PR.

@tjgq
Copy link
Contributor

tjgq commented Apr 27, 2023

How much of a concern is it that this change increases the Action.Command message size for every action?

An alternative would be to use the high_api_version reported in the ServerCapabilities message [1] to decide which field to use. But I'm not sure if the complication is worth it.

I have to scratch my head a bit on this. Regarding Remote Cache / RBE implementation, most of the weight is on the CAS traffic and not on Action Cache(AC) side. Increasing the Command message should have minimal impact.

Fair enough, I'm happy to approve as-is. We can improve it later if any of the API implementors express concerns.

The hard question would be when/how we deprecate the usages of the old fields. That's when I think checking ServerSide capability would come into play:

  1. In a major LTS release (i.e. 7.0.0), create a transition period when we only set the old fields when detecting the older version of the API.
  2. In another major LTS release, remove the old field completely.

But I suspect there isn't much appetite for doing this deprecation. Regardless, it's outside the scope of this PR.

I'd expect some future Bazel version to start requiring at least v2.1 of the API when negotiating the server capabilities (currently, it requires at least v2.0). At that point we could stop sending the deprecated fields. I agree that this is future work.

@tjgq tjgq 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 Apr 27, 2023
@sluongng
Copy link
Contributor Author

There is a CI failure on CentOS which I think was due to infra underneath not executing //src/test/java/com/google/devtools/build/lib/remote:remote fast enough leading to a timeout.

I suspect splitting the target into 5 different shards might help, but I will leave that to the maintainers to decide.
The exact target passed on the other linux platforms on CI.

@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 27, 2023
@sluongng sluongng deleted the sluongng/command-output-paths branch April 27, 2023 15:15
sluongng added a commit to sluongng/bazel that referenced this pull request Apr 29, 2023
In bazelbuild#18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.
sluongng added a commit to sluongng/bazel that referenced this pull request Apr 29, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request Apr 29, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request May 1, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request May 1, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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.
copybara-service bot pushed a commit that referenced this pull request May 15, 2023
In #18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

Closes #18269.

PiperOrigin-RevId: 531999688
Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0
sluongng added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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
Copy link
Contributor Author

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 17, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.3.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 17, 2023
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request May 17, 2023
This is a follow-up to bazelbuild#18198

Make Bazel compatible with newer version of Remote Api by setting
output_paths along-side output_directories and output_files. The latter
2 are deprecated in newer REAPI specification.

Closes bazelbuild#18202.

PiperOrigin-RevId: 527560509
Change-Id: I14c80d69aa9a5e9bf29a8c5694412ecd58ea17bf
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request May 17, 2023
In bazelbuild#18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

Closes bazelbuild#18269.

PiperOrigin-RevId: 531999688
Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
This is a follow-up to bazelbuild#18198

Make Bazel compatible with newer version of Remote Api by setting
output_paths along-side output_directories and output_files. The latter
2 are deprecated in newer REAPI specification.

Closes bazelbuild#18202.

PiperOrigin-RevId: 527560509
Change-Id: I14c80d69aa9a5e9bf29a8c5694412ecd58ea17bf
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
In bazelbuild#18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

Closes bazelbuild#18269.

PiperOrigin-RevId: 531999688
Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0
sluongng added a commit to sluongng/bazel that referenced this pull request May 30, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request May 30, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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.
copybara-service bot pushed a commit that referenced this pull request Jun 5, 2023
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.

Closes #18270.

PiperOrigin-RevId: 537817532
Change-Id: Ie299065a7c91abbfc7a4f181410f6a57471e7dc8
copybara-service bot pushed a commit that referenced this pull request Jun 5, 2023
*** Reason for rollback ***

Breaks remote server which only support REPI v2.0.

*** Original change description ***

Conditionally set output_paths based on Remote Executor capabilities

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.

Closes #18270.

PiperOrigin-RevId: 537883626
Change-Id: I13c03cb3f4d64f106dc90767a6e62dfbae4027e2
iancha1992 added a commit that referenced this pull request Jun 6, 2023
In #18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

Closes #18269.

PiperOrigin-RevId: 531999688
Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0

Co-authored-by: Son Luong Ngoc <sluongng@gmail.com>
iancha1992 added a commit that referenced this pull request Jun 6, 2023
This is a follow-up to #18198

Make Bazel compatible with newer version of Remote Api by setting
output_paths along-side output_directories and output_files. The latter
2 are deprecated in newer REAPI specification.

Closes #18202.

PiperOrigin-RevId: 527560509
Change-Id: I14c80d69aa9a5e9bf29a8c5694412ecd58ea17bf

Co-authored-by: Son Luong Ngoc <sluongng@gmail.com>
sluongng added a commit to sluongng/bazel that referenced this pull request Jun 13, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request Jun 20, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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 added a commit to sluongng/bazel that referenced this pull request Jun 21, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants