[red-knot] Add support for editable installs to the module resolver #12307

AlexWaygood · 2024-07-12T19:51:44Z

Summary

This PR adds support for editable installations in site-packages to the module resolver. Still a few details to be checked vis-à-vis Python's runtime semantics.

Work towards #11653.

Test Plan

cargo test -p red_knot_module_resolver

github-actions · 2024-07-12T20:04:48Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

AlexWaygood · 2024-07-12T20:24:46Z

crates/red_knot_module_resolver/src/path.rs

@@ -73,6 +73,7 @@ enum ModuleResolutionPathBufInner {
    FirstParty(SystemPathBuf),
    StandardLibrary(FilePath),
    SitePackages(SystemPathBuf),
+    EditableInstall(SystemPathBuf),


I considered just using the SitePackages variant for editable installs, since these search paths do come about as a consequence of .pth files in the site-packages directory. However, the search paths for editable installs aren't necessarily children of the site-packages search path (and indeed most often won't be), and I think it would be too easy to assume that all instances of the SitePackages variant have the site-packages directory as their root.

I don't have a problem with introducing a new enum variant for pth-added paths, though I'm also not totally convinced by it: I don't know when we'd ever handle them differently, and it requires one more chunk of duplicated code wherever we handle this enum. I don't think the confusion you mention should occur: in general search paths should never be children of other search paths (this is a really bad setup that leads to the same module being importable under two different names), so it doesn't seem to me to be a likely confusion to assume this would be the case.

I'm not sure about the name EditableInstall. pth files are a feature of the Python import system that are used by the editable-install feature of some package managers, but the feature predates that use, and pth files have been (and I'm sure still are) used in many other ways as well (e.g. for the zipped-egg installation format of setuptools). So I think it's potentially a little misleading to imply that every path discovered in a .pth file is an editable install.

I'd suggest PthEntry or something similar instead.

I'm not sure about the name EditableInstall. pth files are a feature of the Python import system that are used by the editable-install feature of some package managers, but the feature predates that use, and pth files have been (and I'm sure still are) used in many other ways as well (e.g. for the zipped-egg installation format of setuptools). So I think it's potentially a little misleading to imply that every path discovered in a .pth file is an editable install.

Hrm, true, but the validation we do elsewhere should ensure that we only add paths that point to directories when we iterate through .pth files to try to find paths that could potentially be editable installs. While it's true that a directoy path in a .pth file could point to something which wasn't intended to be an editable install, the Python runtime will still consider Python modules at that path to be importable, correct (since they'll be added to sys.path by site.py)? So IIUC a directory path in a .pth file is in effect an editable install, even if it... wasn't meant to be.

All of which is to say, from the perspective of the module resolver, this variant should only really be used if we think it's more-likely-than-not that a path represents a directory that has been editably installed, or a path relative to a path representing a directory that has been editably installed. We should have done sufficient validation at this point that it's more than just "an arbitrary line in a .pth file".

I'm okay with either name but it might be at the time that we add some documentation to each variant because I don't think the linked order still applies (it doesn't mention editable installs)

Adding some more docs makes sense but I won't do it just now, since I may still refactor this pretty soon according to the idea Carl suggested on one of my earlier PRs

Certainly any path found in a .pth file will be added to sys.path, and particularly if that path is a directory, it will result in modules there being importable. I'm not sure if there is any other validation you're referring to that we do.

The logical jump you're making that I don't quite follow is "if we add a path to sys.path that makes things importable, that's an 'editable install'." I don't think that follows. It's just importable Python source code. "Editable install" is a very specific user-facing feature that implies a certain use case. For example, there's no reason the path added by the pth file line couldn't point to some Python code on a read-only filesystem. We should still support that just fine, but it's not very editable! In that case the pth file entry is being used for some other use case that I wouldn't describe as an "editable install."

crates/red_knot_module_resolver/src/path.rs

crates/red_knot_module_resolver/src/resolver.rs

MichaReiser · 2024-07-12T20:46:30Z

Dude, stop working 😂 I thought you needed some rest

AlexWaygood · 2024-07-12T20:51:23Z

Dude, stop working 😂 I thought you needed some rest

I have an addiction :(

crates/red_knot_module_resolver/src/resolver.rs

codspeed-hq · 2024-07-13T14:49:16Z

CodSpeed Performance Report

Merging #12307 will improve performances by 4.81%

_{Comparing editables (0d64399) with editables (e29d3d4)}

Summary

⚡ 1 improvements
✅ 32 untouched benchmarks

Benchmarks breakdown

	Benchmark	`editables`	`editables`	Change
⚡	`red_knot_check_file[incremental]`	90 µs	85.9 µs	+4.81%

AlexWaygood · 2024-07-13T14:49:55Z

crates/red_knot_module_resolver/src/resolver.rs

+    #[test]
+    fn deleting_pth_file_on_which_module_resolution_depends_invalidates_cache() {
+        const SITE_PACKAGES: &[FileSpec] = &[("_foo.pth", "/x/src")];
+        const X_DIRECTORY: &[FileSpec] = &[("/x/src/foo.py", "")];
+
+        let TestCase {
+            mut db,
+            site_packages,
+            ..
+        } = TestCaseBuilder::new()
+            .with_site_packages_files(SITE_PACKAGES)
+            .with_other_files(X_DIRECTORY)
+            .build();
+
+        let foo_module_name = ModuleName::new_static("foo").unwrap();
+        let foo_module = resolve_module(&db, foo_module_name.clone()).unwrap();
+        assert_eq!(
+            Some(foo_module.file()),
+            system_path_to_file(&db, SystemPathBuf::from("/x/src/foo.py"))
+        );
+
+        let pth_file = site_packages.join("_foo.pth");
+        db.memory_file_system().remove_file(&pth_file).unwrap();
+        File::touch_path(&mut db, &pth_file);
+        assert_eq!(resolve_module(&db, foo_module_name.clone()), None);
+    }


This test is failing, and I'm not sure why. To read the contents of the pth files, I'm using system_path_to_file in PthFileIterator::next(), and source_text() in PthFile::new(). IIUC, I think that should be sufficient to indicate to Salsa that the module_search_paths() query depends on the existence and contents of the _foo.pth file, and that therefore deleting the _foo.pth file should invalidate the previous query.

I took a quick look at the salsa events and this seems correct.

[crates/red_knot_module_resolver/src/resolver.rs:1555:9] db.take_salsa_events().debug(&db) = [ Event { runtime_id: RuntimeId { counter: 0, }, kind: WillCheckCancellation, }, Event { runtime_id: RuntimeId { counter: 0, }, kind: WillCheckCancellation, }, Event { runtime_id: RuntimeId { counter: 0, }, kind: WillCheckCancellation, }, Event { runtime_id: RuntimeId { counter: 0, }, kind: DidValidateMemoizedValue { database_key: source_text(2), }, }, Event { runtime_id: RuntimeId { counter: 0, }, kind: DidValidateMemoizedValue { database_key: parse_typeshed_versions(2), }, }, Event { runtime_id: RuntimeId { counter: 0, }, kind: WillCheckCancellation, }, Event { runtime_id: RuntimeId { counter: 0, }, kind: WillExecute { database_key: dynamic_module_resolution_paths(0), }, }, Event { runtime_id: RuntimeId { counter: 0, }, kind: DidValidateMemoizedValue { database_key: resolve_module_query(0), }, }, ]

It re-runs the dynamic_module_resolution_paths query because the dynamic module resolution paths changed (you deleted a phf file). It then re-runs the resolve_module_query but evaluates to the same cached result. Not sure why this is the case.

It seems that adding db.report_untracked_read() to the function body, like you suggested elsehwere, makes the test start passing. But then if I start using db.system().read_to_string() in the query to get the contents of the .pth file rather than source_text() (which you said should be okay if I added the report_untracked_read() call), it starts failing again.

MichaReiser

Overall looks good.

An unrelated comment for resolver (and some of them might be because how I structured the code). I find it difficult to read the code because I always have to jump up and down. The ModuleResolutionSettings aren't next to the ValidatedSearchPathSettings, instead, the Pth code becomes in between. We shouldn't refactor the code as part of this PR but maybe you can come up with an ordering that brings the code closer together.

crates/red_knot_module_resolver/src/testing.rs

crates/red_knot_module_resolver/src/resolver.rs

AlexWaygood · 2024-07-15T11:01:49Z

An unrelated comment for resolver (and some of them might be because how I structured the code). I find it difficult to read the code because I always have to jump up and down. The ModuleResolutionSettings aren't next to the ValidatedSearchPathSettings, instead, the Pth code becomes in between. We shouldn't refactor the code as part of this PR but maybe you can come up with an ordering that brings the code closer together.

Yeah, I have the same opinion. I'm planning on doing a followup where I move some more code out into a separate submodule just for finding the search paths -- but although this PR makes the problem worse, I didn't want to do it in this PR, as it would have made the diff harder to read and review.

crates/red_knot_module_resolver/src/resolver.rs

…ation rather than deletion of the `pth` file

Co-authored-by: Micha Reiser <micha@reiser.io>

crates/red_knot_module_resolver/src/resolver.rs

Co-authored-by: Carl Meyer <carl@astral.sh>

crates/red_knot_module_resolver/src/resolver.rs

MichaReiser

This looks good to me. Thanks for addressing all feedback.

I think we should handle deduplication in a follow up PR. It's not specifically related to editable installs. It's just that editable installs make the problem worse.

My only ask before merging is to change PthFile to use a String instead of SourceText and to use IndexMap over OrderedSet.

MichaReiser · 2024-07-16T14:40:18Z

crates/red_knot_module_resolver/src/path.rs

@@ -73,6 +73,7 @@ enum ModuleResolutionPathBufInner {
    FirstParty(SystemPathBuf),
    StandardLibrary(FilePath),
    SitePackages(SystemPathBuf),
+    EditableInstall(SystemPathBuf),


I'm okay with either name but it might be at the time that we add some documentation to each variant because I don't think the linked order still applies (it doesn't mention editable installs)

crates/red_knot_module_resolver/src/resolver.rs

AlexWaygood · 2024-07-16T16:36:54Z

to use IndexMap over OrderedSet.

I already changed the PR so that it uses indexmap::IndexSet internally rather than ordermap::OrderSet. But I think I have a confusingly named type alias in there. I'll change the name of that.

…itables

[red-knot] Add support for editable installs to the module resolver

287fcec

AlexWaygood added the red-knot Multi-file analysis & type inference label Jul 12, 2024

Deduplicate search path roots

645532b

AlexWaygood mentioned this pull request Jul 12, 2024

[red-knot] PEP 561 compliant module resolver #11653

Open

17 tasks

AlexWaygood commented Jul 12, 2024

View reviewed changes

crates/red_knot_module_resolver/src/path.rs Outdated Show resolved Hide resolved

AlexWaygood commented Jul 12, 2024

View reviewed changes

crates/red_knot_module_resolver/src/resolver.rs Outdated Show resolved Hide resolved

AlexWaygood commented Jul 12, 2024

View reviewed changes

crates/red_knot_module_resolver/src/resolver.rs Outdated Show resolved Hide resolved

Make all module resolution search paths absolute

a456ef9

AlexWaygood marked this pull request as ready for review July 13, 2024 13:01

AlexWaygood requested review from carljm and MichaReiser as code owners July 13, 2024 13:01

AlexWaygood added 3 commits July 13, 2024 14:16

Improve docs

9d68c07

Be more lenient about trailing and leading whitespace

99d87b4

Make it a Salsa query and add caching tests

f7219f2

AlexWaygood commented Jul 13, 2024

View reviewed changes

MichaReiser mentioned this pull request Jul 14, 2024

[red-knot] Add basic workspace support #12318

Merged

Lazily discover editable installations

f664391

AlexWaygood force-pushed the editables branch from 66a7ded to f664391 Compare July 14, 2024 15:09

MichaReiser reviewed Jul 15, 2024

View reviewed changes

AlexWaygood added 5 commits July 15, 2024 12:51

Misc review comments

42551d1

Simplify

1709e91

Use report_untracked_read()

64f03c6

Avoid an unnecessary allocation

b1b2ffb

Don't add a new method for creating arbitrary directories in tests

1adf37d

AlexWaygood requested a review from MichaReiser July 15, 2024 13:16

MichaReiser reviewed Jul 15, 2024

View reviewed changes

crates/red_knot_module_resolver/src/resolver.rs Outdated Show resolved Hide resolved

AlexWaygood force-pushed the editables branch from 0c1e24c to a77b535 Compare July 15, 2024 14:27

Change test so that it asserts the cache is invalidated after modific…

388fdca

…ation rather than deletion of the `pth` file

AlexWaygood force-pushed the editables branch from a77b535 to 388fdca Compare July 15, 2024 14:43

AlexWaygood and others added 2 commits July 15, 2024 17:13

Update crates/red_knot_module_resolver/src/resolver.rs

1c267b5

Co-authored-by: Micha Reiser <micha@reiser.io>

Cleanup tests

6d8acb9

AlexWaygood force-pushed the editables branch from c6b2316 to 6d8acb9 Compare July 15, 2024 17:04

carljm reviewed Jul 15, 2024

View reviewed changes

crates/red_knot_module_resolver/src/resolver.rs Outdated Show resolved Hide resolved

crates/red_knot_module_resolver/src/resolver.rs Outdated Show resolved Hide resolved

Fix comment

e29d3d4

Co-authored-by: Carl Meyer <carl@astral.sh>

AlexWaygood commented Jul 16, 2024

View reviewed changes

crates/red_knot_module_resolver/src/resolver.rs Outdated Show resolved Hide resolved

MichaReiser approved these changes Jul 16, 2024

View reviewed changes

AlexWaygood added 3 commits July 16, 2024 19:09

Fix bugs, address review, add TODOs

23d2d8b

Merge branch 'editables' of https://github.com/astral-sh/ruff into ed…

52f457b

…itables

Merge branch 'main' into editables

0d64399

AlexWaygood enabled auto-merge (squash) July 16, 2024 18:14

AlexWaygood merged commit 9a2dafb into main Jul 16, 2024
19 checks passed

AlexWaygood deleted the editables branch July 16, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] Add support for editable installs to the module resolver #12307

[red-knot] Add support for editable installs to the module resolver #12307

AlexWaygood commented Jul 12, 2024

github-actions bot commented Jul 12, 2024 •

edited

Loading

AlexWaygood Jul 12, 2024

carljm Jul 15, 2024

AlexWaygood Jul 16, 2024

MichaReiser Jul 16, 2024

AlexWaygood Jul 16, 2024

carljm Jul 16, 2024 •

edited

Loading

MichaReiser commented Jul 12, 2024 •

edited

Loading

AlexWaygood commented Jul 12, 2024

codspeed-hq bot commented Jul 13, 2024 •

edited

Loading

AlexWaygood Jul 13, 2024

MichaReiser Jul 15, 2024

AlexWaygood Jul 15, 2024

MichaReiser left a comment

AlexWaygood commented Jul 15, 2024

MichaReiser left a comment

MichaReiser Jul 16, 2024

AlexWaygood commented Jul 16, 2024

[red-knot] Add support for editable installs to the module resolver #12307

[red-knot] Add support for editable installs to the module resolver #12307

Conversation

AlexWaygood commented Jul 12, 2024

Summary

Test Plan

github-actions bot commented Jul 12, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

MichaReiser commented Jul 12, 2024 • edited Loading

AlexWaygood commented Jul 12, 2024

codspeed-hq bot commented Jul 13, 2024 • edited Loading

Merging #12307 will improve performances by 4.81%

Summary

Benchmarks breakdown

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

AlexWaygood commented Jul 15, 2024

MichaReiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood commented Jul 16, 2024

github-actions bot commented Jul 12, 2024 •

edited

Loading

`ruff-ecosystem` results

carljm Jul 16, 2024 •

edited

Loading

MichaReiser commented Jul 12, 2024 •

edited

Loading

codspeed-hq bot commented Jul 13, 2024 •

edited

Loading