Add a subcommand to generate dependency graphs

[charliermarsh]

Summary

This PR adds an experimental Ruff subcommand to generate dependency graphs based on module resolution.

A few highlights:

• You can generate either dependency or dependent graphs via the --direction command-line argument.
• Like Pants, we also provide an option to identify imports from string literals (--detect-string-imports).
• Users can also provide additional dependency data via the include-dependencies key under [tool.ruff.import-map]. This map uses file paths as keys, and lists of strings as values. Those strings can be file paths or globs.

The dependency resolution uses the red-knot module resolver which is intended to be fully spec compliant, so it's also a chance to expose the module resolver in a real-world setting.

The CLI is, e.g., ruff graph build ../autobot, which will output a JSON map from file to files it depends on for the autobot project.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[AlexWaygood]

Couple of comments from skimming!

[charliermarsh]

This needs tests prior to merging but I would like feedback.

[MichaReiser]

Overall looks good.

I think we're creating too many Db instances which will hurt performance and I would recommend to use ModuleName and SystemPath over QualifiedName and Path to avoid conversions at the leaf.

[MichaReiser]

What's the motivation for creating a new db for each source root?

The caching is local to each database, and creating a new database for each package means that we trade less congestion with reduced cache efficiency. This might be fine if the resolved modules in each package root are distinct but I suspect that the files contain cross-package imports and many stdlib imports that now need to resolved multiple times.

The recommended way to parallelism in Salsa is to implement a snapshot method for your Database that creates a shallow clone:

    #[must_use]
    pub fn snapshot(&self) -> Self {
        Self {
            workspace: self.workspace,
            storage: self.storage.clone(),
            files: self.files.snapshot(),
            system: Arc::clone(&self.system),
        }
    }

ruff/crates/red_knot_workspace/src/db.rs

Lines 82 to 90 in ecab04e

	#[must_use]
	pub fn snapshot(&self) -> Self {
	Self {
	workspace: self.workspace,
	storage: self.storage.clone(),
	files: self.files.snapshot(),
	system: Arc::clone(&self.system),
	}
	}

And you can then create a unique database handle for each task:

        let files = WorkspaceFiles::new(db, self);
        let result = Arc::new(std::sync::Mutex::new(Vec::new()));
        let inner_result = Arc::clone(&result);

        let db = db.snapshot();
        let workspace_span = workspace_span.clone();

        rayon::scope(move |scope| {
            for file in &files {
                let result = inner_result.clone();
                let db = db.snapshot();
                let workspace_span = workspace_span.clone();

                scope.spawn(move |_| {
                    let check_file_span = tracing::debug_span!(parent: &workspace_span, "check_file", file=%file.path(&db));
                    let _entered = check_file_span.entered();

                    let file_diagnostics = check_file(&db, file);
                    result.lock().unwrap().extend(file_diagnostics);
                });
            }
        });

        Arc::into_inner(result).unwrap().into_inner().unwrap()

ruff/crates/red_knot_workspace/src/workspace.rs

Lines 206 to 220 in 599103c

	rayon::scope(move |scope| {
	for file in &files {
	let result = inner_result.clone();
	let db = db.snapshot();
	let workspace_span = workspace_span.clone();
	scope.spawn(move |_| {
	let check_file_span = tracing::debug_span!(parent: &workspace_span, "check_file", file=%file.path(&db));
	let _entered = check_file_span.entered();
	let file_diagnostics = check_file(&db, file);
	result.lock().unwrap().extend(file_diagnostics);
	});
	}
	});

Edit: I see below that you already did this. I'm still curious for why we need multiple Dbs. I think what we do here is too granular. At least if I remember how package roots works correctly. I think it doesn't just generate a db for the top-level packages but also for all sub packages. That means that we have two database for a.b.c: one for package a and another for b if c is a module and a and a.b are packages. The result is that we double-resolve modules for a.b, depending on whether they come from the a or a.b. database.

I also worry that relative imports don't work due to that because I think we might baile out at the src_root.

Can't we generate a single database or a database for each common ancestor of the packages (which ideally is the src directory)?

If not, can we add some package paths to SearchPathSettings::extra_paths?

[AlexWaygood]

If not, can we add some package paths to SearchPathSettings::extra_paths?

This isn't really "correct" since extra_paths take priority over even the stdlib search path in module resolution. But maybe it'll produce the right answer in most cases.

In the real world, if you have multiple independent first-party packages in a workspace (which may or may not depend on each other), you're likely to have them all editably installed into a venv, and red-knot would statically discover all the first-party packages by reading the pth files in the site-packages directory and add them as search paths that way. But maybe that approach doesn't work here.

[charliermarsh]

The source roots here are actually just the root folders. So, like, in Django, it's just a small set of top-level folders:

["/Users/crmarsh/workspace/django", "/Users/crmarsh/workspace/django/tests", "/Users/crmarsh/workspace/django/tests/admin_scripts/custom_templates", "/Users/crmarsh/workspace/django/tests/admin_scripts/custom_templates/project_template", "/Users/crmarsh/workspace/django/tests/i18n/sampleproject", "/Users/crmarsh/workspace/django/tests/migrations/faulty_migrations/namespace", "/Users/crmarsh/workspace/django/tests/migrations/test_fake_initial_case_insensitive"]

[charliermarsh]

We could use a single database with extra paths -- that's actually the way I did it initially, though Alex said it's not the intended use-case. The downside, I think, is that it would allow those source roots to import files across one another. Maybe that's correct though! I can change it.

[AlexWaygood]

The source roots here are actually just the root folders.

Possibly I'm still misunderstanding, but if everything's importable from the workspace root, doesn't that imply you can resolve all modules and submodules relative to a single search path, src_root, which would point to the workspace root?

[MichaReiser]

Alex is obviously the authority when it comes to search paths and I'm probably wrong haha 😆

The intended setup is to use a single database for an application (except VS code where you can have multiple workspaces). That gives the best cache reuse. If that's possible, great. If we aren't hitting any perf issues, that's great too.

[carljm]

Looking at the roots for Django that were listed, I think those are all independent roots (as in, they aren't all importable from a single source root), but they can import each other. So I think it would be more efficient to use a single db, by using multiple you are likely repeating work. It's true that the red-knot resolver (because it was built around the more restrictive assumptions of PEP 561 for Python type checkers, rather than a generic "ordered sys.path" abstraction) doesn't obviously accommodate this use case, but I think extra-paths is a totally fine way to handle it, and better than multiple databases. (In fact I think this is pretty much the sort of use-case extra-paths exists to handle.)

This isn't really "correct" since extra_paths take priority over even the stdlib search path in module resolution.

I don't understand this comment: all first-party paths (both the main source root and the extra paths) should always take priority over the stdlib in module resolution. According to PEP 561 order, the first two items in the resolution order are extra paths and then the first-party root. So if you have multiple "first party roots", given that they have to go in some resolution order, I don't see how any incorrectness is introduced by putting N-1 of them as extra paths, and whichever one you want last as the main first-party root.

[AlexWaygood]

This isn't really "correct" since extra_paths take priority over even the stdlib search path in module resolution.

I don't understand this comment: all first-party paths (both the main source root and the extra paths) should always take priority over the stdlib in module resolution.

Ugh, you're quite right. Blame it on the holiday haze... yeah, using extra_paths should be fine then, in that case. Sorry for causing unnecessary work @charliermarsh :(

[carljm]

In the real world, if you have multiple independent first-party packages in a workspace (which may or may not depend on each other), you're likely to have them all editably installed into a venv, and red-knot would statically discover all the first-party packages by reading the pth files in the site-packages directory and add them as search paths that way. But maybe that approach doesn't work here.

In many real-world cases (including Django), you'll have little sub-trees of Python code that aren't packaged at all or installed via Python packaging tools, but still end up on the runtime sys.path in some more bespoke way. That's the sort of scenario extra-paths is supposed to be for.

[MichaReiser]

Should we remove the OptionsMetadata trait for now to exclude the settings from the schema?

[carljm]

Very cool!

[carljm]

This logic (determining a package root for an arbitrary file by walking up the tree looking for __init__.py) is part of Ruff so it must be fairly battle-tested, but I'm a little surprised if it doesn't often fail on big internal codebases. In my experience, "accidental namespace packages" (because someone just forgot an __init__.py, and imports worked anyway) are pretty common. Maybe a lot of people use linters (like Ruff, perhaps!) that prevent this?

Because of that, I tend to be more on the side of, if you're going to be doing multi-file analysis, you should explicitly configure the package root(s), rather than trying to discover files and packages automatically, which is easily broken by namespace packages and therefore requires explicitly configuring namespace packages.

[carljm]

This is more aggressive than I realized -- we don't even look for calls to __import__ or importlib.import_module, we just look for any string that might be a module path.

It seems like this could easily be fooled by cases where an import-path constant is defined in one file (like a Django settings file) but the string is imported and then used in a dynamic import in a different module.

But if it's good enough to match existing tools on the codebases we care about, great!