Fix issues with path mapping in `map_each` functions #22227

fmeum · 2024-05-03T12:22:33Z

Fixes the following issues with path mapping applied to map_each functions:

Action keys could be stale if an action implementation switches from hard-coded artifact strings to Files mapped at execution time. This commit introduces a special PathMapper instance for fingerprinting, see the long comment on PathMappers.FOR_FINGERPRINTING. It includes a number of tests to verify that neither too much nor too little is mapped during action key computation.
The Starlark objects representing mapped artifact roots didn't have the correct semantics for equality and ordered comparisons. They are now unequal to unmapped roots.
When not using path mapping, the StarlarkSemantics of a thread used to evaluate map_each callbacks is not modified.

These changes require splitting PathMappers out of analysis_cluster and also make StrippingPathMapper a subclass of PathMapper instead of using an anonymous one.

aranguyen

@fmeum thank you for the pr and I'm so sorry for the late review. I added a few questions and just have a general concern with having a distinct PathMapper just for fingerprinting and a different PathMapper? for mapping the command line inputs & for usage on the executors. I don't have a good suggestion to solve the issue at the moment but would like to discuss this further in one of our next meetings

Do we have reported failures that are related to this? It would be great to have these documented.

I LGTM this for now to get this imported and tested internally in parallel.

aranguyen · 2024-07-18T01:52:57Z

src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java

+   *   <li>Unconditionally using {@link StrippingPathMapper} can result in stale action keys when an
+   *       action is opted out of path mapping at execution time due to input path collisions after
+   *       stripping. See path_mapping_test for an example.
+   *   <li>Using {@link PathMapper#NOOP} does not distinguish between map_each results built from


is this because FileApi.getExecPathStringForStarlark still needed for things that are not opted in with map_each? Just wondering if we can merge them to get rid of the difference in behavior

We can't get rid of the difference in this situation since File#path only returns the correct result when used in a map_each callback. If someone e.g. keeps paths as strings around in providers and then migrates to artifacts to become compatible with path mapping, they may run into this staleness (without this fix).

aranguyen · 2024-07-18T02:05:59Z

src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java

+   *       the input depsets of all actions that opt into path mapping and also increases CPU usage.
+   *   <li>Unconditionally using {@link StrippingPathMapper} can result in stale action keys when an
+   *       action is opted out of path mapping at execution time due to input path collisions after
+   *       stripping. See path_mapping_test for an example.


Do we have a real use case for this one? How did you discover this issue?

This issue is more of an edge case and I only discovered it when I tried to think through whether our fingerprinting scheme is sound in all cases.

sgowroji · 2024-07-18T08:15:50Z

Hello @fmeum, Could you please resolve the above code conflicts. Thanks!

fmeum · 2024-07-18T19:36:24Z

@sgowroji I resolved the conflicts

fmeum · 2024-07-30T07:14:17Z

@bazel-io flag

iancha1992 · 2024-07-31T18:44:07Z

@bazel-io fork 7.4.0

nikhilkalige · 2024-07-31T20:18:48Z

@fmeum Is this a necessary fix to try various C++ path mapping changes in upcoming 7.3 release?

fmeum · 2024-07-31T20:37:41Z

@nikhilkalige No, the basics work without it. See #22658 for the exact missing features.

Fixes the following issues with path mapping applied to `map_each` functions: * Action keys could be stale if an action implementation switches from hard-coded artifact strings to `File`s mapped at execution time. This commit introduces a special `PathMapper` instance for fingerprinting, see the long comment on `PathMappers.FOR_FINGERPRINTING`. It includes a number of tests to verify that neither too much nor too little is mapped during action key computation. * The Starlark objects representing mapped artifact roots didn't have the correct semantics for equality and ordered comparisons. They are now unequal to unmapped roots. * When not using path mapping, the `StarlarkSemantics` of a thread used to evaluate `map_each` callbacks is not modified. These changes require splitting `PathMappers` out of `analysis_cluster` and also make `StrippingPathMapper` a subclass of `PathMapper` instead of using an anonymous one. Closes bazelbuild#22227. PiperOrigin-RevId: 657449600 Change-Id: If949d23acab163cb192ea3af1baeb7a3caca7339

Fixes the following issues with path mapping applied to `map_each` functions: * Action keys could be stale if an action implementation switches from hard-coded artifact strings to `File`s mapped at execution time. This commit introduces a special `PathMapper` instance for fingerprinting, see the long comment on `PathMappers.FOR_FINGERPRINTING`. It includes a number of tests to verify that neither too much nor too little is mapped during action key computation. * The Starlark objects representing mapped artifact roots didn't have the correct semantics for equality and ordered comparisons. They are now unequal to unmapped roots. * When not using path mapping, the `StarlarkSemantics` of a thread used to evaluate `map_each` callbacks is not modified. These changes require splitting `PathMappers` out of `analysis_cluster` and also make `StrippingPathMapper` a subclass of `PathMapper` instead of using an anonymous one. Closes #22227. PiperOrigin-RevId: 657449600 Change-Id: If949d23acab163cb192ea3af1baeb7a3caca7339 Closes #22227

iancha1992 · 2024-10-11T21:09:05Z

The changes in this PR have been included in Bazel 7.4.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=7.4.0rc1. Thanks!

fmeum force-pushed the fix-semantics branch from 2a96696 to 92269ab Compare May 3, 2024 12:27

fmeum changed the title ~~Fix semantics~~ Fix issues with path mapping in map_each functions May 3, 2024

fmeum force-pushed the fix-semantics branch from 92269ab to 631f484 Compare May 3, 2024 12:32

fmeum marked this pull request as ready for review May 3, 2024 12:34

fmeum requested review from a team and lberki as code owners May 3, 2024 12:34

fmeum requested review from katre and aranguyen and removed request for a team, lberki and katre May 3, 2024 12:34

github-actions bot added team-Performance Issues for Performance teams team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels May 3, 2024

fmeum removed team-Performance Issues for Performance teams team-Rules-Java Issues for Java rules labels May 3, 2024

aranguyen approved these changes Jul 18, 2024

View reviewed changes

aranguyen 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 Jul 18, 2024

Add tests

a3e7d78

fmeum force-pushed the fix-semantics branch from 631f484 to 5940a5a Compare July 18, 2024 19:29

Fix breakages

95f08cd

fmeum force-pushed the fix-semantics branch from 5940a5a to 95f08cd Compare July 18, 2024 19:35

copybara-service bot closed this in ca6e80e Jul 30, 2024

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jul 30, 2024

fmeum deleted the fix-semantics branch July 30, 2024 07:14

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 30, 2024

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jul 31, 2024

bazel-io mentioned this pull request Jul 31, 2024

[7.4.0] Fix issues with path mapping in map_each functions #23177

Closed

fmeum mentioned this pull request Aug 22, 2024

[7.4.0] Fix issues with path mapping in map_each functions #23394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with path mapping in `map_each` functions #22227

Fix issues with path mapping in `map_each` functions #22227

fmeum commented May 3, 2024 •

edited

Loading

aranguyen left a comment

aranguyen Jul 18, 2024

fmeum Jul 18, 2024

aranguyen Jul 18, 2024

fmeum Jul 18, 2024

sgowroji commented Jul 18, 2024

fmeum commented Jul 18, 2024

fmeum commented Jul 30, 2024

iancha1992 commented Jul 31, 2024

nikhilkalige commented Jul 31, 2024

fmeum commented Jul 31, 2024

iancha1992 commented Oct 11, 2024

Fix issues with path mapping in map_each functions #22227

Fix issues with path mapping in map_each functions #22227

Conversation

fmeum commented May 3, 2024 • edited Loading

aranguyen left a comment

Choose a reason for hiding this comment

aranguyen Jul 18, 2024

Choose a reason for hiding this comment

fmeum Jul 18, 2024

Choose a reason for hiding this comment

aranguyen Jul 18, 2024

Choose a reason for hiding this comment

fmeum Jul 18, 2024

Choose a reason for hiding this comment

sgowroji commented Jul 18, 2024

fmeum commented Jul 18, 2024

fmeum commented Jul 30, 2024

iancha1992 commented Jul 31, 2024

nikhilkalige commented Jul 31, 2024

fmeum commented Jul 31, 2024

iancha1992 commented Oct 11, 2024

Fix issues with path mapping in `map_each` functions #22227

Fix issues with path mapping in `map_each` functions #22227

fmeum commented May 3, 2024 •

edited

Loading