Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork3.1k
Make metas more compact; fix indirect suppression#20075
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.
| current_options=manager.options.clone_for_module(id).select_options_affecting_cache() | ||
| current_options=options_snapshot(id,manager) | ||
| ifmanager.options.skip_version_check: | ||
| # When we're lax about version we're also lax about platform. |
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? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's an interesting idea - I didn't even think of that. Nice catch!
Uh oh!
There was an error while loading.Please reload this page.
According tomypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
JukkaL 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.
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) | ||
| ifmanager.options.skip_version_check: | ||
| # When we're lax about version we're also lax about platform. |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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, butselect_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I think we should try this at some point, it may give visible speed-up for large builds (the method usesgetattr() in a loop, that may be slow).
37333c7 intopython:masterUh oh!
There was an error while loading.Please reload this page.
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).