-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] cache Type::is_redundant_with #20477
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
Conversation
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-10-16 11:31:13.724737373 +0000
+++ new-output.txt 2025-10-16 11:31:17.072755647 +0000
@@ -1,5 +1,5 @@
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(d017)): execute: too many cycle iterations`
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(16443)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(d417)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(16c43)): execute: too many cycle iterations`
_directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
_directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
_directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__` |
CodSpeed Performance ReportMerging #20477 will improve performances by 12.52%Comparing Summary
Benchmarks breakdown
Footnotes
|
This comment was marked as outdated.
This comment was marked as outdated.
89b9a18 to
3c75a77
Compare
|
(Rebasing this on main to see the benchmark results) |
|
I took a look at the mypy primer hang for pandas-stubs and I'm starting to believe that this is the same as astral-sh/ty#1111. The cycle head is I've been able to reproduce the hang on almost every run using But I haven't yet been able to reproduce it after pulling in #20515 What I don't understand is why this is only happening when ty runs multi-threaded. Is it just because the cycle nesting is different and, depending on the nesting, the cycle converges faster? Or is there a multithreaded bug my hacky "re-use nested fixpoint values" implementation only happens to bypass the problematic code? |
|
I added some logging to the The MRO for Name("DatetimeIndex"): Iteration 122 vs 123 https://www.diffchecker.com/EJa9XVe0/ I suspect what's happening is that, depending on the entry query, the In fact, the query does panic fairly quickly. I added a few logging statements to Salsa and the output for a run where ty hangs is: which confirms that there's still a multi threaded bug in Salsa when a query participating in a cycle panics (as discussed here awslabs/shuttle#193 (comment)). For now, I suggest that we add some tracing logs to Salsa similar to what I did so that this becomes easier to discover (and doesn't require a 3h debugging session). I'll spend some time today to see if I can figure out why Salsa ends up hanging if there's a panic but I don't plan on spending too much time on it because astral-sh/ty#1111 seems more important than fixing something that improves our debugging experience. |
|
The fix that I've in mind for astral-sh/ty#1111 might help here because we would iterate all cycles at once, rather than having many nested cycles. I think that could help because I suspect that whether we run into this issue depends on how the |
|
I bumped the Salsa version to a temporary commit that contains the upstream fix and mypy primer is now failing with: I'll consider this "solved" from the salsa side (except for the optimisation discussed in astral-sh/ty#1111 which I expect to help here). @carljm ping me if you want me to look into anything else. |
|
65cf8dc to
5ee2420
Compare
|
|
Nice, mypy primer now completes with the new salsa version... The test failure is somewhat concerning. Is this new due to the salsa update? |
|
TODO: Address |
Nope, the test was failing before my update commit. @carljm maybe something you could take a look at (I've no idea what's happening there 😆 ) |
The There's also a panic in recursive protocol tests, though :-| |
|
My biggest concern with this PR is the memory increase - memo metadata = ~89MB
+ memo metadata = ~103MBThat's pretty substantial and can easily be a few GB for large projects (I suspect it uses around as much memory as Could we limit the caching scope? Would it even make sense to cache |
I expect this would be worse for memory usage, as the return type of It may be interesting to look at this again after #20602 and only cache |
These test failures should be fixed by #20653. We had some test output that depended on the ordering of Salsa IDs |
Caching |
c1695e1 to
8ef874d
Compare
|
Hmm, I did a rebase and one test is now stack overflowing. @carljm do you remember if this already happened before? I don't think it's something I should be able to mess up with salsa. Or did I mess up the rebase? |
|
The memory usage now looks better but it also isn't a perf improvement anymore |
it might be because Carl now needs to cache |
Pretty sure that both of these changes are because union simplification now uses Not sure about the stack overflow. I wouldn't expect this PR in and of itself to cause a stack overflow. |
but the mdtest you've added as part of this PR causes us to overflow on |
Oh yes, of course! Totally forgot I'd added that test in this PR. |
8ef874d to
ad69224
Compare
|
I switched the PR to cache There does also seem to be a new fuzzer failure on seed 102, though. |
|
This PR resolves some hangs. So I'm fine accepting the memory regression, and we can look into adding support for within a revision LRU to salsa (for queries that return a cloneable value). |
|
The fuzzer failure should be fixed with the latest salsa version (of my fixpoint rewrite PR) |
ad69224 to
081f95a
Compare
|
Added a tracking issue astral-sh/ty#1367 |
AlexWaygood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's go
…rable * origin/main: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
…nt-sets * dcreager/non-non-inferable: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
Summary
TODO
Test Plan
Added test that stack overflowed before this PR.