Upgraded to the latest versions of the remote execution APIs

[sbarnat]

Upgraded to the latest versions of the https://github.com/googleapis/googleapis and https://github.com/bazelbuild/remote-apis
RE v2.1+

[sbarnat]

AI Disclosure: Changes were made by an AI agent and review by multiple humans. Unfortunately, none of them are Rust experts, so some special attention is warranted there.

[benjyw]

Before reviewing, why are there generated .pb.go files here?

[sbarnat]

Before reviewing, why are there generated .pb.go files here?

These were copied "as-is" from the remote-apis repo, eg. https://github.com/bazelbuild/remote-apis/blob/main/build/bazel/remote/asset/v1/remote_asset.pb.go

Anything else that should not be not copied from these repos?

[tdyas]

Before reviewing, why are there generated .pb.go files here?

These were copied "as-is" from the remote-apis repo, eg. https://github.com/bazelbuild/remote-apis/blob/main/build/bazel/remote/asset/v1/remote_asset.pb.go

Anything else that should not be not copied from these repos?

The .pb.go files are irrelevant to Pants. Only the proto files matter since the prost crate will generate Rust bindings at build time.

[benjyw]

I mean this respectfully and constructively, but I'm not confident about the quality of this PR, given that thousands of lines of unnecessary generated code were included in it, either due to AI or to cargo-culting. It would be helpful if this underwent a deeper pre-review audit by the author and others on their team, and if you could provide a more detailed explanation in a comment on the PR of how it was generated, and how the logic was subsequently humanly validated.

[cburroughs]

Thanks for the contribution. We've just branched for 2.31.x, so merging this pull request now will come out in 2.32.x, please move the release notes updates to docs/notes/2.32.x.md if that's appropriate.

[sbarnat]

I mean this respectfully and constructively, but I'm not confident about the quality of this PR, given that thousands of lines of unnecessary generated code were included in it, either due to AI or to cargo-culting. It would be helpful if this underwent a deeper pre-review audit by the author and others on their team, and if you could provide a more detailed explanation in a comment on the PR of how it was generated, and how the logic was subsequently humanly validated.

I removed the pb.go files from the googleapis and bazelbuild_remote-apis dump. I also removed other redundant files from these repo copies.
Whilst I can understand reluctance in accepting AI generated code but the changes are actually quite simple (if not trivial).
Here is a brief summary, also included in the 2.32 release notes:
RE API v2.2 changes:

• platform field moved from Command to Action. This means it will now be part of cache key
• Command.platform is now deprecated
  RE API v2.1 chagnes:
• Deprecated separate Command.output_files and Command.output_directories
• Unified into single Command.output_paths field
• Runtime logic now determines if each path is a file or directory

Like mentioned before, the code was reviewed by a few key engineers at NVIDIA. There is no overly verbose or redundant code that we can see. There was no cargo-culting. The only part, which may be unnecessary is trying to fall back to deprecated output_files/output_directories for backward compatibility in main.rs.

The code was verified using our ~15k target scaling demo, which is substantial and without the change it hangs. Obviously, internal unit tests and linting were run as part of PR submission.

We also object to the characterization of this change that thousands of unnecessary generated code were included in. Here are the actual stats:
10 files changed
+219 insertions
-150 deletions
Net: +69 lines

Do we have any specific objections to any of the code that was submitted? That would help moving things forward and addressing the actual issues/concerns.

[benjyw]

There are several unrelated commits in this PR now. I think you need to rebase so we can see what the actual diffs are now. The information you provided above on how you created and tested this PR is helpful, particularly that you ran it in your demo repo and it worked. Once you rebase I can evaluate the remaining changes.

[benjyw]

FWIW the .pb.go files amounted to about 10K lines of superfluous code, so the concern that raised seems warranted.

[sureshjoshi]

My read of this is that there were a few misses here, which can help improve the process moving forward:

Whilst I can understand reluctance in accepting AI generated code

I don't think this part is relevant. We have an in-progress AI disclosure policy (#22982), but your team has always disclosed anyways - which is great. It just helps us focus review effort - and you already called out the Rust experience 👍🏽

Many of the maintainers use LLMs and we've also merged a lot of PRs with code aided by LLMS.

The code was verified using our ~15k target scaling demo, which is substantial and without the change it hangs.

It turns out that we don't have a pull_request_template.md in this repo, which I must have hallucinated that we did (or I just conflated our issue templates). The information you provided about the crux of the change, the summary, how you tested/verified it, etc... is great.

It's exactly the sort of thing our PR template should have - which should help with making sure reviewers know what to expect when opening up a review.

Having this info in the PR description likely would have streamlined this process a bit, so I've opened up an issue to add a PR template including this sort of info.

We also object to the characterization of this change that thousands of unnecessary generated code

At time of PR, all the superfluous code was thousands of lines of unnecessary generated code.

If nothing else, just the sticker shock of the diff was there.

Do we have any specific objections to any of the code that was submitted?

As Benjy mentioned, this PR seems to be mangled with unrelated PRs now to main (93 files changed, +4700, -3500 lines). My guess is once the PR hygiene is cleaned up, and with the generated code removed, this would be a lot easier/quicker to review.

[sbarnat]

The PR has now been rebased cleanly and should only contain 3 commits. No other unrelated commits are included.
Do you still want me to include info in pull_request_template.md?

[tdyas]

Frankly this PR conflates updating the protobufs with also figuring out how to handle the deprecations which may have semantic implications for Pants users. For example, the Pants plugin API will still use output_files and output_directories even though this PR maps them both to output_paths for REPAI. This could be a change in semantics because, for example, a path in REAPI output_files should result in an error under REAPI if it is a directory. Whether Pants should actually enforce that semantic is an open question, but I believe we should at least ask how this will impact users (even if we decide to just move straight to the current REAPI semantics).

For ease of review, I separately split out the REAPI protobuf update to #23077 so we have a minimal change with no immediate semantic change. The output_paths change can be built on top of that PR.

[tdyas]

We also object to the characterization of this change that thousands of unnecessary generated code were included in.

Facts:

• You included ~10,942 lines of generated Go code in a Rust crate. The proof is in commit 9476260 where you deleted that very same Go code.
• Again, this PR is a change to a Rust crate.

What is your specific objection in light of these facts?