Skip to content

ci: find and automatically remove unused imports#2318

Open
romanc wants to merge 7 commits intospcl:mainfrom
romanc:romanc/find-and-fix-unused-imports
Open

ci: find and automatically remove unused imports#2318
romanc wants to merge 7 commits intospcl:mainfrom
romanc:romanc/find-and-fix-unused-imports

Conversation

@romanc
Copy link
Contributor

@romanc romanc commented Mar 9, 2026

Unused imports add a performance overhead at runtime, and risk creating import cycles. To automatically detect and remove them in the future, this PR suggests to add ruff as a pre-commit hook to detect unused imports. I've added an exception for __init__.py files to allow re-exports without adding an explicit __all__ list or adding extra annotations.

The PR grew quite big. However, non-automatic changes are only in the following seven files

  • .pre-commit-config.yaml: configure pre-commit to run the ruff linter
  • ruff.toml: configure the ruff linter to only search for unused imports (rule F401)
  • dace/autodiff/library/library.py: make sure we keep the ParameterArray import for backwards compatibility
  • dace/frontend/python/replacements/operators.py: make sure we keep the dace import for evaluation of data types
  • tests/library/include_test.py: make sure we keep the necessary import in the middle of the test
  • dace/sdfg/analysis/schedule_tree/treenodes.py: manually remove the now trivial if TYPE_CHECKING branch

@romanc romanc marked this pull request as ready for review March 9, 2026 19:39
Copy link
Collaborator

@tbennun tbennun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good effort here! Only a few comments on things that I could catch


if __name__ == '__main__':
try:
import tensorflow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import should stay (dependency tester)


if __name__ == '__main__':
try:
import tensorflow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import should stay (dependency tester)


if __name__ == '__main__':
try:
import tensorflow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import should stay (dependency tester)


if __name__ == '__main__':
try:
import tensorflow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import should stay (dependency tester)


if __name__ == '__main__':
try:
import tensorflow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import should stay (dependency tester)

@romanc romanc requested a review from tbennun March 10, 2026 08:17
@tbennun tbennun enabled auto-merge March 10, 2026 15:48
@tbennun
Copy link
Collaborator

tbennun commented Mar 10, 2026

cscs-ci run

cuda codegen had an `isinstance` check that compared ageinst
`dace.nodes.data.Stream`. This worked because `dace.nodes` had an import
that read `from dace import data` (which is implicitly re-exporting
`data` from where it's imported. That `data` import was unused within
`dace/sdfg/nodes.py` and thus was removed by `ruff`.

The proposed solution here is to simply compare the instance against
`dace.data.Stream` without the indirection.
auto-merge was automatically disabled March 11, 2026 09:02

Head branch was pushed to by a user without write access

@romanc
Copy link
Contributor Author

romanc commented Mar 11, 2026

newest commit should fix the GPU tests

@romanc
Copy link
Contributor Author

romanc commented Mar 11, 2026

Now one of the (cpu bound) ML tests is suddenly failing after being fine twice in a row. I've only changed cuda codegen in the last commit. Could it be that this test is unstable? I can't seem to just re-run the ML tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants