Skip to content
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

[feature request] checking for slow imports recursively #15770

Open
luketaverne opened this issue Jan 27, 2025 · 4 comments
Open

[feature request] checking for slow imports recursively #15770

luketaverne opened this issue Jan 27, 2025 · 4 comments
Labels
wish Not on the current roadmap; maybe in the future

Comments

@luketaverne
Copy link

luketaverne commented Jan 27, 2025

Description

Hi everyone. First of all, thanks for making such great software, it's a joy to work with.

I have a feature proposal which I would like your feedback on. Afterwards, I'm happy to implement it :)

Background:

  • We have a python monorepo, which contains many projects alongside each other, and some common folders, which shared code should go in.
  • Each project is managed by a different team, and may have different preferences for linting with ruff.
  • Project A wants to ban top-level imports for tensorflow. We can do this with a project_a/ruff.toml containing the following:
[lint.flake8-tidy-imports]
banned-module-level-imports = ["tensorflow"]
  • This will work for anything inside of project_a that imports tensorflow directly, but any additional nesting will not be detected. e.g. if project_a/one.py has from common import uses_tensorflow, which imports tensorflow at the top-level, this will not be detected.

Proposal:

Leverage the ruff-graph crate to traverse the import tree starting at a given path and apply ruff check recursively.

To the user, this would look something like:

ruff check --recursive projects/project_a or explicitly without the ruff.toml in the project directory: ruff check --recursive --config 'lint.flake8-tidy-imports.banned-module-level-imports=["tensorflow"]' --select TID projects/project_a

This could effectively be hardcoded behind something like a ruff analyze check-recursive command, for convenience.

Questions:

  • Where does this fit the best, as to not hurt the performance of existing commands? e.g. rust check at the top-level of a monorepo, with this rule in place, does not need the import traversal. Maybe nested inside of the ruff analyze or ruff analyze graph commands, or an entirely separate command?
  • Side-effect: collision with rules from other project directories. With this example rule (banned-module-level-imports), the rules in another project are irrelevant. i.e. if project_b allows top-level tensorflow imports, that's fine, we only want a linter error if project_a imports one of those. For other rules, like say line-length of 60 in project_a and 120 in project_b, how should we avoid generating errors about the line length in project_b when starting with project_a? Maybe this ask is so specific, it should only be looking for the nested import lint?
  • Setting up the ruff Python AST: Simply recursing the dependency tree generated by ruff analyze graph yields both top-level and local imports (which makes sense for the original use case, but not for this one). I started looking at how to get the AST set up, so that I could differentiate between local and top-level imports when traversing the dependency graph, but it started to get quite "hacky". Do you have any tips for easily getting the AST set up for detecting local/module level imports?

I will work on getting some example code over to you, but wanted to hear if you have any thoughts on this in the meantime 🙂.

@luketaverne luketaverne changed the title [feature request] [feature request] checking for slow imports recrusively Jan 27, 2025
@luketaverne luketaverne changed the title [feature request] checking for slow imports recrusively [feature request] checking for slow imports recursively Jan 27, 2025
@dylwil3 dylwil3 added the wish Not on the current roadmap; maybe in the future label Jan 27, 2025
@MichaReiser
Copy link
Member

Thanks for opening this issue. Let me summarize what I understand:

You want to point ruff to an "entry point" and apply the lint rules to all files reachable from that entry point (while excluding third party dependencies). This is different from what Ruff does today where it applies the checks to all files in a given directory.

I can see how this is useful. I think TypeScript has a similar mode but I don't feel comfortable implementing this on top of ruff-analze because it isn't very accurate and it requires parsing all modules twice. We should be in a better position to support this with red knot -- our new semantic analysis

@luketaverne
Copy link
Author

Hi Micha. Yes that sums it up well. For my use case, it's only this specific rule, but allowing user-configurable rules would be useful. It would be important to only apply the selected rules and ignore any ruff config files that might be encountered in the reachable files / modules (i.e. a common folder has a conflicting config with the rule specified, and we don't want to pick up and enable additional rules from a config)

For my use case, I would like to ignore any local imports (inside functions), because this is about chasing down modules which cause long import times. For others, maybe it would be a useful option to follow local imports as well. Perhaps that would be an extension of the ruff-graph crate, that could be used to generate a full dependency tree from a given file or module. With the speed that ruff runs at, I imagine there could be a number of tools built on top of a fast dependency graph generator.

Is red knot at a point where this is something I could contribute? I have read that it's a new effort, and saw some discussion here about it not being ready yet #12143 (comment). I have some working code for building this on top of ruff-analyze, but it isn't able to skip local imports. I'd be happy to adapt that to work with red knot, if it's ready for contributions 🙂.

@MichaReiser
Copy link
Member

Is red knot at a point where this is something I could contribute?

Not yet, I'm sorry. Our current focus is on building a type checker and we're intentionally keeping the scope narrow (at least, as narrow as possible for a type checker). It will be a while before we can consider new feature additions

@luketaverne
Copy link
Author

Okay, thanks Micha 🙂. Looking forward to seeing red knot develop. Please let me know if anything should change, I'm happy to contribute where I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wish Not on the current roadmap; maybe in the future
Projects
None yet
Development

No branches or pull requests

3 participants