Make metas more compact; fix indirect suppression#20075
Make metas more compact; fix indirect suppression#20075ilevkivskyi merged 2 commits intopython:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
| current_options = manager.options.clone_for_module(id).select_options_affecting_cache() | ||
| current_options = options_snapshot(id, manager) | ||
| if manager.options.skip_version_check: | ||
| # When we're lax about version we're also lax about platform. |
There was a problem hiding this comment.
Could you explain this? It isn't a new line, but this behavior is really confusing to me. How "accept a cache generated by another mypy version" is related to "use cache for a potentially wrong platform"?
There was a problem hiding this comment.
Unfortunately I don't have context on this decision, probably the idea is that --skip-version-check implies the user assumes manual control over the cache. Also this behaviour is there from 2017, so I guess those who use it, are fine with it.
There was a problem hiding this comment.
I think the motivation is that you can easily debug e.g. caches generated on Linux on a macOS development machine. --skip-version-check is designed to make debugging incremental issues easier -- e.g. you can ask a user to send a .tgz with a cache dump which reproduces some issue, and you can use that cache on your development machine, even if it has a different OS and you might not have the exact same mypy version (e.g. with local changes with a fix attempt).
There was a problem hiding this comment.
That's an interesting idea - I didn't even think of that. Nice catch!
Co-authored-by: Stanislav Terliakov <[email protected]>
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Nice, cache meta files were quite verbose, and were probably impacting performance in large incremental runs. I don't understand all the changes related to indirect dependencies, but since you probably have the best understanding of them at this point, I trust that the changes are reasonable.
| current_options = manager.options.clone_for_module(id).select_options_affecting_cache() | ||
| current_options = options_snapshot(id, manager) | ||
| if manager.options.skip_version_check: | ||
| # When we're lax about version we're also lax about platform. |
There was a problem hiding this comment.
I think the motivation is that you can easily debug e.g. caches generated on Linux on a macOS development machine. --skip-version-check is designed to make debugging incremental issues easier -- e.g. you can ask a user to send a .tgz with a cache dump which reproduces some issue, and you can use that cache on your development machine, even if it has a different OS and you might not have the exact same mypy version (e.g. with local changes with a fix attempt).
| Separately store only the options we may compare individually, and take a hash | ||
| of everything else. If --debug-cache is specified, fall back to full snapshot. | ||
| """ | ||
| snapshot = manager.options.clone_for_module(id).select_options_affecting_cache() |
There was a problem hiding this comment.
Random thing I noticed (no need to fix here, but could be useful to fix eventually if I'm right): clone_for_module does a bunch of caching to avoid repeated computation, but select_options_affecting_cache recomputes everything on each call. I think a similar caching strategy would work for it, which might improve options processing performance. If you agree with this, I can create an issue about this. The caching should probably also cover `hash_digest(json_dumps(...)``.
There was a problem hiding this comment.
Yes, I think we should try this at some point, it may give visible speed-up for large builds (the method uses getattr() in a loop, that may be slow).
3 participants
This makes cache metas ~twice smaller with two things:
--debug-cacheis set.dep_hashesas a list instead of a dictionary.Note that while implementing the second part, the assert I added failed, so I started digging and fond that suppression was handled inconsistency for indirect dependencies. I am fixing this here, now indirect dependencies are (un)suppressed just like any other dependency. Note this allowed to simplify/delete some parts of the code.
I the next PR I am going to use fixed format serialization for
CacheMeta(when enabled).