Skip to content

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Sep 22, 2025

SCC construction should only consider import dependencies, since indirect dependencies are not available during non-incremental runs. I think otherwise SCCs can be different in non-incremental vs incremtal runs.

Attempt to fix an issue with excessively large SCCs computed (sometimes) after #19798 when doing incremental runs. I haven't been able to create a self-contained test case that reproduces the issue, but this appears to fix this issue with an internal codebase.

SCC construction should only consider import dependencies, since
indirect dependencies are not available during non-incremental runs.
I think otherwise SCCs can be different in non-incremental vs incremtal
runs.

Attempt to fix an issue with excessively large SCCs computed
(sometimes) after #19798 when doing incremental runs. I haven't
been able to create a self-contained test case that reproduces
the issue, but this appears to fix this issue with an internal
codebase.
@JukkaL JukkaL requested a review from ilevkivskyi September 22, 2025 16:01

def sorted_components(
graph: Graph, vertices: AbstractSet[str] | None = None, pri_max: int = PRI_ALL
graph: Graph, vertices: AbstractSet[str] | None = None, pri_max: int = PRI_INDIRECT
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The name pri_max is a bit confusing -- dependencies with a priority less than this are considered. I don't want to change this logic at this point, since it would require larger changes as this function is used in a few different places.

@ilevkivskyi
Copy link
Member

@JukkaL

I think otherwise SCCs can be different in non-incremental vs incremtal runs.

I don't think this is possible unless there is a bug somewhere. Indirect dependencies should never create a cycle (and vice versa). So although the PR is technically correct (we don't need indirect dependencies to identify SCCs), I think we should investigate this deeper, otherwise this will bite us later anyway.

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

AutoSplit (https://github.com/Toufool/AutoSplit)
-   File "/tmp/mypy_primer/mypy_old/venv/bin/mypy", line 7, in <module>
+   File "/tmp/mypy_primer/mypy_new/venv/bin/mypy", line 7, in <module>
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/__main__.py", line 15, in console_entry
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/__main__.py", line 15, in console_entry
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/main.py", line 127, in main
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/main.py", line 127, in main
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/main.py", line 211, in run_build
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/main.py", line 211, in run_build
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/build.py", line 196, in build
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/build.py", line 196, in build
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/build.py", line 272, in _build
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/build.py", line 272, in _build
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/build.py", line 2946, in dispatch
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/build.py", line 2946, in dispatch
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/build.py", line 3346, in process_graph
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/build.py", line 3346, in process_graph
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/build.py", line 3475, in process_stale_scc
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/build.py", line 3475, in process_stale_scc
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/build.py", line 2493, in write_cache
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/build.py", line 2493, in write_cache
-   File "/tmp/mypy_primer/mypy_old/venv/lib/python3.13/site-packages/mypy/cache.py", line 28, in __init__
+   File "/tmp/mypy_primer/mypy_new/venv/lib/python3.13/site-packages/mypy/cache.py", line 28, in __init__

@ilevkivskyi
Copy link
Member

FWIW I am suspecting this logic

           while "." in fullname and fullname not in self.modules:
                fullname = fullname.rsplit(".")[0]

in record_imported_symbol() together with something like an ignored missing import or a module/package level __getattr__(). Perhaps we need something like

           and not (isinstance(self.node, Var) and self.node.from_module_getattr)

there, similar to what we have in SymbolTableNode.serialize()?

@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 22, 2025

Yes, I agree the this only works around a deeper underlying issue. That code in record_imported_symbol looks kind of suspicious.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 23, 2025

I suspect that if there is a local variable that has the same name as a top-level module, mypy may generate a spurious indirect dependency to this module. I'll try to come up with a self-contained reproducer. Maybe references to local definitions should be filtered out in record_imported_symbol.

@ilevkivskyi
Copy link
Member

FWIW I think I see a potential way to make the logic there a bit more robust, it seems to me we should be able to simply:

  1. Handle the class-level decorators (not handled now)
  2. Turn the loop into a single split, since then we know the symbol must be top-level

I will try to make a PR later today.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 23, 2025

#19906 stops generating indirect deps for locals, and it also has a test case.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Let's merge both PRs, as this one also has its value (at least as a niche performance optimization).

JukkaL added a commit that referenced this pull request Sep 23, 2025
Don't generate an indirect dependency to module `bar` if a local
variable has name `bar`.

This aims to fix the root cause of the issue #19903 tries to solve.
@JukkaL JukkaL merged commit 354bea6 into master Sep 23, 2025
20 checks passed
@JukkaL JukkaL deleted the fix-scc-prios branch September 23, 2025 13:15
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