Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
Try simple-minded call expression cache#19505
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment has been minimized.
This comment has been minimized.
ilevkivskyi commentedJul 25, 2025
Nice! Interpreted |
ilevkivskyi commentedJul 25, 2025
Hm, both changes in the primer however look suspicious, I will take a look at them later. |
mypy/checkexpr.py Outdated
| # Deeply nested generic calls can deteriorate performance dramatically. | ||
| # Although in most cases caching makes little difference, in worst case | ||
| # it avoids exponential complexity. | ||
| # TODO: figure out why caching within lambdas is fragile. |
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.
I also discovered that in#19408. Accepting a lambda has explicit type context dependency (infer_lambda_type_using_context), so generic calls do really require re-accepting the lambda every time, context from outer generic may have propagated later.
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.
OK, thanks for the pointer. I will update the comment.
| # Errors ignored, so no point generating fancy messages | ||
| returnTrue | ||
| for_watcherinself._watchers: | ||
| ifself._watchers: |
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.
Could you explain this change? Watchers used to be additive and that sounded reasonable to me...
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.
Previously, ifany of the active watchers was ignoring errors, we could use simpler messages, but in presence of caching this is not valid anymore. For example, we can accept an expression when there is enclosing ignoring watcher, but then the caching watcher will record simple message, and if next time we, by chance, accept same expression in same type context, but without the ignoring watcher, an incorrect (i.e. way too terse) error message will be pulled from the cache.
Without this change 6 tests fail because of terse/simplistic error messages are used.
| # Although in most cases caching makes little difference, in worst case | ||
| # it avoids exponential complexity. | ||
| # TODO: figure out why caching within lambdas is fragile. | ||
| elifisinstance(node, (CallExpr,ListExpr,TupleExpr))andnot ( |
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.
Would it be difficult to allow dicts and sets here? Inline dictionaries are relatively common and even heavier than lists, and sets just for consistency.
Also operator exprs can bereally heavy (#14978) and are fundamentally similar toCallExpr, are they worth considering?
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.
The problem with dicts/sets is that I see around 0.3% regression on self-check when I add them (but maybe this is just noise). My reasoning is that most code has a bunch of shallow dictionaries, and for those caching is just busy-work that will never be used (note caching is not free, since mypyc is slow on creating local type maps and watchers).
Anyway, I am open to considering more expression kinds to cache, but lets put those in separate PR(s).
ilevkivskyi commentedJul 25, 2025
OK, it looks like I found the problem with |
Diff frommypy_primer, showing the effect of this PR on open source code: werkzeug (https://github.com/pallets/werkzeug)+ src/werkzeug/datastructures/structures.py:193: error: Generator has incompatible item type "tuple[Union[K, Any], Union[list[V], list[Any]]]"; expected "tuple[K, V]" [misc] |
ilevkivskyi commentedJul 25, 2025
OK, the two previous changes in |
sterliakov commentedJul 25, 2025
LG! I'm still a bit scared to approve this now, will take another look at this tomorrow:) |
ilevkivskyi commentedJul 28, 2025
@sterliakov Did you have time for another look? Btw comparison with current master went below noise level, probably because of#19515. But I also played with some "micro-benchmarks" involving deeply nested calls, and this PR gives performance improvements on order of 100x (no exaggeration). If there are no concrete suggestions, I think we should give this a try. |
sterliakov 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.
Yeah, sorry, I think this is good to go!
comparison with current master went below noise level
that sounds OK: mypy itself isn't generic-heavy, so it shouldn't demonstrate huge improvements.colour changes speak for themselves:)
bd94bcb intopython:masterUh oh!
There was an error while loading.Please reload this page.
Added the disable_expression_cache flag to [tool.mypy] section inpyproject.toml to ensure mypy runs use the workaround everywhere, not justin pre-commit hooks.Added documentation linking to:- PR #19505 that introduced the expression cache bug- TODO to file upstream issue with reproduction case- TODO to remove workaround once bug is fixedThe flag is needed in both places:- .pre-commit-config.yaml: For pre-commit hook runs- pyproject.toml: For direct mypy invocations and IDE integrationsReferences:-python/mypy#19505 (expression cache PR)- mypy 1.18.1+ affected by cross-module Final[str] type loss
This gives a modest 1% improvement on self-check (compiled), but it gives almost 40% on
mypy -c "import colour". Some comments:CallExpr,ListExpr, andTupleExpr, this is not very principled, I found this as a best balance between rare cases likecolour, and more common cases like self-check.In general, this is a bit scary (as this a major change), but also perf improvements for slow libraries are very tempting.