-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix file reloading in dmypy with --export-types #16359
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mypy/dmypy_server.py
Outdated
changed += [ | ||
(bs.module, bs.path) | ||
for bs in sources | ||
if bs.path and (bs.module, bs.path) not in changed |
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.
This is quadratic, and it could be slow if there are many files and many that are changed. Maybe construct a temporary set from changed
and use it in the not in changed
check?
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, good point, fixed now.
mypy/dmypy_server.py
Outdated
changed += [ | ||
(bs.module, bs.path) | ||
for bs in sources | ||
if bs.path and (bs.module, bs.path) not in changed |
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.
Similar to above. Maybe move this to a helper function to avoid code duplication?
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.
Moved to a helper function.
mypy/test/testfinegrained.py
Outdated
@@ -149,6 +149,7 @@ def get_options(self, source: str, testcase: DataDrivenTestCase, build_cache: bo | |||
options.use_fine_grained_cache = self.use_cache and not build_cache | |||
options.cache_fine_grained = self.use_cache | |||
options.local_partial_types = True | |||
options.export_types = True |
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.
What's the purpose of this change?
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.
Starting the daemon with this option set to True
is cleaner for inspect
unit tests, I made this explicit now (so it will not slow down other fine grained tests).
@@ -164,7 +165,7 @@ def get_options(self, source: str, testcase: DataDrivenTestCase, build_cache: bo | |||
return options | |||
|
|||
def run_check(self, server: Server, sources: list[BuildSource]) -> list[str]: | |||
response = server.check(sources, export_types=True, is_tty=False, terminal_width=-1) | |||
response = server.check(sources, export_types=False, is_tty=False, terminal_width=-1) |
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.
Hmm why do we have export_types=False
here, but true above?
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.
Passing it explicitly on each run will force reloading files. Putting it in options from the start is IMO cleaner.
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py): typechecking got 1.09x faster (138.5s -> 127.5s)
(Performance measurements are based on a single noisy sample)
|
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.
Thanks for the updates!
Fixes #15794 Unfortunately, this requires to pass `--export-types` to `dmypy run` if one wants to inspect a file that was previously kicked out of the build.
The script format changelog entries based on commit history and has some rules to filter out some changes, such as typeshed sync and changes cherry-picked to the previous release branch. Example of how to run it: ``` $ python misc/generate_changelog.py 1.7 Generating changelog for 1.7 Previous release was 1.6 Merge base: d7b2451 NOTE: Drop "Fix crash on ParamSpec unification (for real)", since it was in previous release branch NOTE: Drop "Fix crash on ParamSpec unification", since it was in previous release branch NOTE: Drop "Fix mypyc regression with pretty", since it was in previous release branch NOTE: Drop "Clear cache when adding --new-type-inference", since it was in previous release branch NOTE: Drop "Match note error codes to import error codes", since it was in previous release branch NOTE: Drop "Make PEP 695 constructs give a reasonable error message", since it was in previous release branch NOTE: Drop "Fix ParamSpec inference for callback protocols", since it was in previous release branch NOTE: Drop "Try upgrading tox", since it was in previous release branch NOTE: Drop "Optimize Unpack for failures", since it was in previous release branch * Fix crash on unpack call special-casing (Ivan Levkivskyi, PR [16381](#16381)) * Fix file reloading in dmypy with --export-types (Ivan Levkivskyi, PR [16359](#16359)) * Fix daemon crash caused by deleted submodule (Jukka Lehtosalo, PR [16370](#16370)) ... ```
Fixes #15794
Unfortunately, this requires to pass
--export-types
todmypy run
if one wants to inspect a file that was previously kicked out of the build.