Add path to cache key for Rust dep inference, for relative imports #19630

huonw · 2023-08-21T04:57:40Z

This adds the file path for the file being dependency-inferred to the cache key for the Rust dep inference. Without this, the same file contents appearing multiple times in different places will give the wrong results for relative imports, because the dep inference process mashes together the file path and the relative import.

The circumstances here seem most likely to occur in the real world if a file is moved, with the inference results from before the move reused for the file after the move too.

I've tested this manually on the reproducer in #19618 (comment), in addition to comparing the before (fails) and after (passes) behaviour of the new test.

To try to make this code more resilient to this sort of mistake, I've removed the top level PreparedInferenceRequest.path key, in favour of using the new input_file_path on the request protobuf struct, at the cost of an additional String -> PathBuf conversion when doing the inference.

Fixes #19618

huonw · 2023-08-21T08:07:37Z

src/rust/engine/src/intrinsics.rs

      )),
      |_workunit| async move {
        let result: ParsedPythonDependencies = get_or_create_inferred_dependencies(
          core,
          &store,
          prepared_inference_request,
-          |content, request| python::get_dependencies(content, request.path),
+          |content, request| {
+            python::get_dependencies(content, request.inner.input_file_path.into())


Per the PR description, this is the additional String -> PathBuf conversion. We can avoid this in a few ways:

use and manipulate String in the get_dependencies functions directly, instead of PathBuf, which seems reasonable given the functions do a &Path -> &str conversion internally anyway (although this would lose some convenience helpers for manipulating the paths)

have a protobuf type that maps to PathBuf directly

Or... just not worry about it, because this conversion is fast.

thejcannon

🙏 You rock! Thanks!

src/python/pants/backend/python/dependency_inference/rules_test.py

thejcannon · 2023-08-21T14:10:44Z

src/rust/engine/src/intrinsics.rs

+  /// The request that's guaranteed to have been constructed via ::prepare().
+  ///
+  /// NB. this `inner` value is used as the cache key, so anything that can influence the dep
+  /// inference should (also) be inside it, not just a key on this struct


WorkerPants · 2023-08-22T00:44:30Z

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.17.x

I was unable to cherry-pick this PR to 2.17.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

(Ensure your git working directory is clean)

Run the following script to reproduce the merge-conflicts:

git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.17.x \
  && git checkout -b cherry-pick-19630-to-2.17.x FETCH_HEAD \
  && git cherry-pick ec77fa9346dd612006dbdba24e5d823b7733da46

Fix the merge conflicts and commit the changes
Run build-support/cherry_pick/make_pr.sh "19630" "2.17.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

…herry-pick of #19630) (#19640) This adds the file path for the file being dependency-inferred to the cache key for the Rust dep inference. Without this, the same file contents appearing multiple times in different places will give the wrong results for relative imports, because the dep inference process mashes together the file path and the relative import. The circumstances here seem most likely to occur in the real world if a file is moved, with the inference results from before the move reused for the file _after_ the move too. I've tested this manually on the reproducer in #19618 (comment), in addition to comparing the before (fails) and after (passes) behaviour of the new test. To try to make this code more resilient to this sort of mistake, I've removed the top level `PreparedInferenceRequest.path` key, in favour of using the new `input_file_path` on the request protobuf struct, at the cost of an additional `String` -> `PathBuf` conversion when doing the inference. Fixes #19618 This is a cherry pick of #19630, but is essentially a slightly weird rewrite that partially cherry-picks #19001 too, by copying over the whole `DependencyInferenceRequest` protobuf type as is (even with some fields that are unused) because that's the easiest way to generate appropriate bytes for turning into the digest for use in the cache-key. I think it's okay to have this "weird" behaviour in the 2.17 branch, with the real/normal code in main/2.18?

…19630) This adds the file path for the file being dependency-inferred to the cache key for the Rust dep inference. Without this, the same file contents appearing multiple times in different places will give the wrong results for relative imports, because the dep inference process mashes together the file path and the relative import. The circumstances here seem most likely to occur in the real world if a file is moved, with the inference results from before the move reused for the file _after_ the move too. I've tested this manually on the reproducer in #19618 (comment), in addition to comparing the before (fails) and after (passes) behaviour of the new test. To try to make this code more resilient to this sort of mistake, I've removed the top level `PreparedInferenceRequest.path` key, in favour of using the new `input_file_path` on the request protobuf struct, at the cost of an additional `String` -> `PathBuf` conversion when doing the inference. Fixes #19618

WorkerPants · 2023-08-29T22:07:03Z

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

✔️ 2.18.x

Successfully opened #19690.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

…herry-pick of #19630) (#19690) This adds the file path for the file being dependency-inferred to the cache key for the Rust dep inference. Without this, the same file contents appearing multiple times in different places will give the wrong results for relative imports, because the dep inference process mashes together the file path and the relative import. The circumstances here seem most likely to occur in the real world if a file is moved, with the inference results from before the move reused for the file _after_ the move too. I've tested this manually on the reproducer in #19618 (comment), in addition to comparing the before (fails) and after (passes) behaviour of the new test. To try to make this code more resilient to this sort of mistake, I've removed the top level `PreparedInferenceRequest.path` key, in favour of using the new `input_file_path` on the request protobuf struct, at the cost of an additional `String` -> `PathBuf` conversion when doing the inference. Fixes #19618 Co-authored-by: Huon Wilson <huon@exoflare.io>

Add path to cache key for Rust dep inference, for relative imports

cb205de

huonw added needs-cherrypick category:internal CI, fixes for not-yet-released features, etc. labels Aug 21, 2023

huonw added this to the 2.17.x milestone Aug 21, 2023

huonw added 2 commits August 21, 2023 18:06

rm top level path

f9b17b8

test

5498b2d

huonw force-pushed the bugfix/19618-rust-infer-path branch from bfe02bd to 5498b2d Compare August 21, 2023 08:06

huonw requested review from stuhood, thejcannon and tobni August 21, 2023 08:06

huonw commented Aug 21, 2023

View reviewed changes

thejcannon approved these changes Aug 21, 2023

View reviewed changes

Review: clearer test, clearer comment

055576a

huonw merged commit ec77fa9 into pantsbuild:main Aug 22, 2023

huonw deleted the bugfix/19618-rust-infer-path branch August 22, 2023 00:43

WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Aug 22, 2023

huonw removed the needs-cherrypick label Aug 22, 2023

huonw mentioned this pull request Aug 22, 2023

Add path to cache key for Rust dep inference, for relative imports (cherry-pick of #19630) #19640

Merged

huonw added the needs-cherrypick label Aug 29, 2023

huonw modified the milestones: 2.17.x, 2.18.x Aug 29, 2023

WorkerPants removed the needs-cherrypick label Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add path to cache key for Rust dep inference, for relative imports #19630

Add path to cache key for Rust dep inference, for relative imports #19630

huonw commented Aug 21, 2023 •

edited

Loading

huonw Aug 21, 2023

thejcannon left a comment

thejcannon Aug 21, 2023

WorkerPants commented Aug 22, 2023

WorkerPants commented Aug 29, 2023

Add path to cache key for Rust dep inference, for relative imports #19630

Add path to cache key for Rust dep inference, for relative imports #19630

Conversation

huonw commented Aug 21, 2023 • edited Loading

huonw Aug 21, 2023

Choose a reason for hiding this comment

thejcannon left a comment

Choose a reason for hiding this comment

thejcannon Aug 21, 2023

Choose a reason for hiding this comment

WorkerPants commented Aug 22, 2023

❌ 2.17.x

WorkerPants commented Aug 29, 2023

✔️ 2.18.x

huonw commented Aug 21, 2023 •

edited

Loading