-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Splitting up the ruff_linter
crate for faster compile times
#1820
Comments
I think another benefit of this could be if we want to have certain features not always included for the pip install. For example, #1628 wants to add spell checking which adds the typos dependency. Since we are adding a dependency and code that will only be used in one place, I feel like this is a great feature to keep in a separate crate. |
We can easily introduce an optional dependency on |
Dev compile times are really getting to me 😅 Have they regressed at all lately? Perhaps we haven't been tracking closely enough to answer that. |
One benefit of separate crates is that it avoids re-compiling unchanged code because Rust skips crates where neither its dependencies nor modules have changed. |
As an initial proposal to tear down, would something like this work?
|
This was my first thought. Has anyone started profiling some of these ideas? I don't think splitting ruff into more crates and introducing features are mutually exclusive, and it's probably a much bigger undertaking to do the former. |
I don't think we want to structure our code by linter at all. I think we rather want to group our AST rules by which AST node they target. In general I think we should firstly improve the structure of our code within the
Reducing such dependencies won't do much for our dev compile time since these dependencies only need to be compiled once. |
I can think of reasons we wouldn't want to do this, but it feels unproductive to debate it right now. Most important is getting the codebase into a state at which we can start breaking it down :) which requires at least (1) changing the project structure (e.g., #2088 or similar) and (2) disentangling the mutual dependencies between rule implementations and |
Will how we build this also determine how we will let people "hook" into ruff and build their own modules into our linter, like flake8 has? Also, once you guys decide on a plan feel free to assign me some refactoring work! |
One thing that seems somewhat straightforward to achieve is to split out the Open questions
I've probably overlooked a few dependencies but it seems less hard than others (e.g. extracting settings is.... challenging) |
I am starting to hack on a few of these problems in #3298, mostly to see what breaks and what problems we run into. (Not ready for review but feedback welcome, etc.) |
This PR productionizes @MichaReiser's suggestion in #1820 (comment), by creating a separate crate for the `ast` module (`rust_python_ast`). This will enable us to further split up the `ruff` crate, as we'll be able to create (e.g.) separate sub-linter crates that have access to these common AST utilities. This was mostly a straightforward copy (with adjustments to module imports), as the few dependencies that _did_ require modifications were handled in #3366, #3367, and #3368.
ruff
crate for faster compile timesruff_linter
crate for faster compile times
@squiddy commented on #1547 with:
Crates can only be compiled in parallel if they don't depend on each other. Matklad has a nice blog post "Fast Rust Builds".
I just opened the PR #1816 to split off the CLI into a separate
ruff_cli
crate, sinceruff_cli
depends onruff
this doesn't really change the total build time (especially becauseruff_cli
can be compiled very quickly ... except the bin generation).These graph were generated via
cargo build --release --timings
, see the timing before the split and the timing after the split.I think the question is if/how we could further split up the
ruff
library. Currently the rule implementations depend onast::Checker
andast::Checker
depends on the rule implementations. I am not sure what we can do about that.With my PR #1816
cargo build -p ruff
for the first time now builds ~100 dependencies fewer thancargo build
for the first time (since it doesn't build the CLI), so this might still be neat for people who just want to contribute a lint and test that it works.The text was updated successfully, but these errors were encountered: