Extract Python dependencies in an intrinsic #18854

thejcannon · 2023-04-28T17:58:14Z

This PR does two things:

Introduces a new crate to the engine: dep_inference. A module inside is dedicated to Python, and leverages tree-sitter and tree-sitter-python to parse Parse dependencies. tree-sitter was chosen because it supports Py2/3, supports other languages, and also is syntax-error-resistant.
Leverages the new crate in an intrinsic. The new behavior is forced opt-in/out and will eventually be the "only" way to do the inference.

TImings

Helper script:

#!/bin/bash
# Replace some random numbers
find src/python/pants -type f -name "*.py" -not -name "__init__.py" | xargs sed -i s/'Copyright [0123456789][0123456789][0123456789][0123456789]'/"Copyright $RANDOM"/
# Wait for the kernel really quick
sleep 1
# Wait for the inotify notifications to stop
while true; do
  mtime=$(stat -c %Y .pants.d/pants.log)
  now=$(date +%s)
  diff=$((now - mtime))
  if (( diff >= 5 )); then
    break
  fi
  sleep $((5 - diff))
done

Timings follows. ./dirty_files.sh runs test worst case scenario ( touch every copyright header). I'm on a 64 core machine, so I run as if we only had 8 cores.

Findings:

In the worst case (the extraction process is not in the process cache) we blow it out of the water in terms of time saved
In the best case (the process cache is hot) we're comparable. Put another way, the time it takes to execute the rule code and lookup the process in the process cache is roughly the amount of time it takes just to parse it again

Worst case (completely cold cache)

$ hyperfine --prepare ./dirty_files.sh --runs 4 --warmup 1 'pants --rule-threads-core=4 --process-execution-local-parallelism=8 --no-python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::' 'pants --rule-threads-core=4 --process-execution-local-parallelism=8 --python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::'
Benchmark 1: pants --rule-threads-core=4 --process-execution-local-parallelism=8 --no-python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::
  Time (mean ± σ):     36.335 s ±  1.286 s    [User: 0.754 s, System: 0.151 s]
  Range (min … max):   34.698 s … 37.645 s    4 runs
 
Benchmark 2: pants --rule-threads-core=4 --process-execution-local-parallelism=8 --python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::
  Time (mean ± σ):      2.899 s ±  0.096 s    [User: 0.758 s, System: 0.131 s]
  Range (min … max):    2.764 s …  2.990 s    4 runs
 
Summary
  'pants --rule-threads-core=4 --process-execution-local-parallelism=8 --python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::' ran
   12.54 ± 0.61 times faster than 'pants --rule-threads-core=4 --process-execution-local-parallelism=8 --no-python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::'

Best Case (hot cache, but no daemon)

$ hyperfine --runs 4 --warmup 1 'pants --no-pantsd --rule-threads-core=4 --process-execution-local-parallelism=8 --no-python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::' 'pants --no-pantsd --rule-threads-core=4 --process-execution-local-parallelism=8 --python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::'
Benchmark 1: pants --no-pantsd --rule-threads-core=4 --process-execution-local-parallelism=8 --no-python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::
  Time (mean ± σ):     20.589 s ±  0.319 s    [User: 20.303 s, System: 2.002 s]
  Range (min … max):   20.167 s … 20.934 s    4 runs
 
Benchmark 2: pants --no-pantsd --rule-threads-core=4 --process-execution-local-parallelism=8 --python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::
  Time (mean ± σ):     19.273 s ±  0.347 s    [User: 18.881 s, System: 1.669 s]
  Range (min … max):   18.940 s … 19.759 s    4 runs
 
Summary
  'pants --no-pantsd --rule-threads-core=4 --process-execution-local-parallelism=8 --python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::' ran
    1.07 ± 0.03 times faster than 'pants --no-pantsd --rule-threads-core=4 --process-execution-local-parallelism=8 --no-python-infer-use-rust-parser --filter-target-type=python_source  dependencies ::'

thejcannon

I'll handle the Django plugin and the dep inference helper union in a separate PR

thejcannon · 2023-05-04T01:13:17Z

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

@@ -109,11 +109,11 @@ def test_normal_imports(rule_runner: RuleRunner) -> None:
            ignored1 as alias1,  # pants: no-infer-dep


Changes to this file will be most important, as they are potential changes in behavior.

src/rust/engine/src/intrinsics.rs

src/rust/engine/src/python_parsing.rs

stuhood

Looks great!

As mentioned in Slack, I'm fine with beginning to do more of this in-process. As you mentioned, there is nothing stopping someone from writing a native wheel 3rdparty plugin to do the same thing, but this eases distribution/release.

src/rust/engine/fs/brfs/Cargo.toml

src/python/pants/engine/internals/native_dep_inference.py

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

src/rust/engine/dep_inference/build.rs

src/rust/engine/src/intrinsics.rs

thejcannon · 2023-05-05T00:58:14Z

Whew! Nice comments. Got through 'em

huonw

I can only review the Rust code (rather than the big picture of how it fits into Pants). I've also not looked through all the 'resolved' comments, so I might've been repeating some things.

My general comment is: nice!

Broadly speaking though, it feels like there's quite a lot of unwraping. I've commented on some specific ones, but there's also a lot of name.named_child(...).unwrap() or equivalent. I'd be nervous about this making dep inference fragile to invalid code, or syntax changes, or other unexpected problems, and bringing down the whole pantsd when they fail.

Some alternatives, depending on the situation:

propagate errors with ? etc.
.expect("<short explanation>") or unwrap_or_else(|| panic!("foo {some_value} bar")) to at least give more context
ignore silently or with some logging (e.g. if the code is invalid, just skip over)

src/rust/engine/dep_inference/src/python/mod.rs

huonw · 2023-05-05T05:54:06Z

src/rust/engine/dep_inference/src/python/mod.rs

+
+#[derive(Serialize, Deserialize)]
+pub struct ParsedPythonDependencies {
+  pub imports: HashMap<String, (u64, bool)>,


Using HashMap<&str, ...> here may not work directly because from .. import abc might be referring to a module that is never named literally.

I guess this could be std::borrow::Cow<'file_contents, str> to allow &str references into the raw file contents when available, but still support dynamically constructed Strings too.

(But, also, not important for this PR.)

src/rust/engine/dep_inference/src/python/mod.rs

src/rust/engine/dep_inference/src/python/tests.rs

src/rust/engine/dep_inference/build.rs

src/rust/engine/dep_inference/src/python/visitor.rs

stuhood

Once the build.rs script is fixed to avoid needing to commit the generated code, I'll shipit. Thanks!

stuhood

Thanks!

thejcannon · 2023-05-05T20:18:53Z

Oh wait, we still need to port or fallback for the Django deo inference plugin 😅

huonw · 2023-05-05T22:12:00Z

Given this is behind an opt-in flag, maybe that could be follow up: people who are using Django inference can not opt-in for now?

thejcannon · 2023-05-07T16:43:51Z

I made sure we resort to the old behavior if the Django framework is enabled. So we can port in a follow up

There's a been a few new crates added recently (#18854, #19958), but we didn't update `[workspace].members` and `[workspace].default-members` in `src/rust/engine/Cargo.toml` to match. In addition, the older `protos` and `grpc_util` weren't listed. This syncs up the lists. The lists of `members` and `default-members` now exactly matches, except for `fs/brfs` as commented. They also match the `Cargo.toml`s that exist on disk.

thejcannon added 15 commits April 19, 2023 14:16

WIP

167e9e7

Actually do something useful

f3a450a

big bundle of changes

7a44d02

undo some goofiness

3e1c65b

it compllies!

8d42a2d

commit

3164f94

pretty much there

05a9c6e

hold onto this

7c570c3

some changes it works

8024d8b

hold these

8b0a067

rename

c600d0c

ok it works now

a65ebd5

bye bye

e9cb728

fmt

a9c4d0c

okie dokie

8dd68cc

thejcannon commented May 4, 2023

View reviewed changes

thejcannon changed the title ~~WIP: Extract Py3 deps in an intrinsic~~ Extract Python dependencies in an intrinsic May 4, 2023

thejcannon requested review from stuhood and huonw May 4, 2023 02:08

thejcannon marked this pull request as ready for review May 4, 2023 02:08

thejcannon commented May 4, 2023

View reviewed changes

src/rust/engine/src/intrinsics.rs Show resolved Hide resolved

thejcannon commented May 4, 2023

View reviewed changes

src/rust/engine/src/python_parsing.rs Outdated Show resolved Hide resolved

thejcannon added 5 commits May 4, 2023 05:54

moving some code

982f5ea

Add workunit

d4e2e4c

bump to debug

a342e8d

Merge branch 'main' into intrinsic-parsing

30f9732

ok good to go 😄

ee41f63

thejcannon added the category:new feature label May 4, 2023

clarify comment

4b0e33e

stuhood reviewed May 4, 2023

View reviewed changes

thejcannon force-pushed the intrinsic-parsing branch from 0024d65 to df2b964 Compare May 4, 2023 20:49

thejcannon added 4 commits May 4, 2023 19:41

Some comments

c0cc660

finish comment

332a896

header

eb926f2

almost everything

ca35c23

huonw reviewed May 5, 2023

View reviewed changes

src/rust/engine/dep_inference/build.rs Outdated Show resolved Hide resolved

src/rust/engine/dep_inference/build.rs Show resolved Hide resolved

src/rust/engine/dep_inference/src/python/visitor.rs Outdated Show resolved Hide resolved

thejcannon added 2 commits May 5, 2023 11:20

Huon's as well

2b5a0a6

a few more

f9fd7eb

stuhood mentioned this pull request May 5, 2023

Dependency calculation performance poor even with cache warm cache #18911

Open

thejcannon added 2 commits May 5, 2023 12:14

fix spacing

9381835

fix build again

f18df33

stuhood reviewed May 5, 2023

View reviewed changes

thejcannon added 2 commits May 5, 2023 13:21

behind the scenes now

60a5c9b

ok there we go

92cab01

stuhood approved these changes May 5, 2023

View reviewed changes

thejcannon added 2 commits May 6, 2023 13:07

hopefully green

8d0b927

dead comment

2df89d0

there we go

f8261a4

thejcannon merged commit 3a20af9 into pantsbuild:main May 9, 2023

thejcannon deleted the intrinsic-parsing branch May 9, 2023 15:00

thejcannon mentioned this pull request May 10, 2023

Improve dependency inference performance #18614

Closed

huonw mentioned this pull request May 11, 2023

Python import parser does not support subprocess_environment or some other way to leak env vars. #16565

Open

thejcannon mentioned this pull request Sep 7, 2023

Remove the deprecated python dep parser #19785

Closed

huonw mentioned this pull request Oct 29, 2023

Refine Rust workspace members for recently added crates #20117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Python dependencies in an intrinsic #18854

Extract Python dependencies in an intrinsic #18854

thejcannon commented Apr 28, 2023 •

edited

Loading

thejcannon left a comment

thejcannon May 4, 2023

stuhood left a comment

thejcannon commented May 5, 2023

huonw left a comment •

edited

Loading

huonw May 5, 2023 •

edited

Loading

stuhood left a comment

stuhood left a comment

thejcannon commented May 5, 2023

huonw commented May 5, 2023 •

edited

Loading

thejcannon commented May 7, 2023

		@@ -109,11 +109,11 @@ def test_normal_imports(rule_runner: RuleRunner) -> None:
		ignored1 as alias1, # pants: no-infer-dep

Extract Python dependencies in an intrinsic #18854

Extract Python dependencies in an intrinsic #18854

Conversation

thejcannon commented Apr 28, 2023 • edited Loading

TImings

Worst case (completely cold cache)

Best Case (hot cache, but no daemon)

thejcannon left a comment

Choose a reason for hiding this comment

thejcannon May 4, 2023

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

thejcannon commented May 5, 2023

huonw left a comment • edited Loading

Choose a reason for hiding this comment

huonw May 5, 2023 • edited Loading

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

thejcannon commented May 5, 2023

huonw commented May 5, 2023 • edited Loading

thejcannon commented May 7, 2023

thejcannon commented Apr 28, 2023 •

edited

Loading

huonw left a comment •

edited

Loading

huonw May 5, 2023 •

edited

Loading

huonw commented May 5, 2023 •

edited

Loading