-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[mypyc] Support incremental compilation #7887
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
63544d0 to
31226be
Compare
|
Excitingly I can not reproduce the windows CI failure on my local windows machine |
15ee998 to
91c616a
Compare
|
I think I have a bead on the Windows issues and I think it can be hacked around in the test harness. I don't think it is blocking the review |
ilevkivskyi
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.
Not really a review, just a comment about writing cache on Windows.
| f.write(contents) | ||
| if old_contents != encoded_contents: | ||
| with open(path, 'wb') as f: | ||
| f.write(encoded_contents) |
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 am not sure if this is relevant, but long time ago we had problems on Windows with incremental mode (see for example #3215). This is why we have some non-trivial logic in and around FilesystemMetadataStore.write(). Maybe you can use the same here?
5e98506 to
790efff
Compare
|
I switched to using setuptools in the run tests and that seems to fix it. setuptools changes how it copies files into place with |
ca3e1a6 to
122e53e
Compare
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.
Did a quick pass -- looks good! Left a few comments; I'll continue my review tomorrow.
| self.mark = False | ||
|
|
||
|
|
||
| class MypycPlugin(Plugin): |
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.
Maybe move this to a separate module, since a mypy plugin seems like its own thing.
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 plugin is pretty closely tied into other logic in this module, since what it does is enforce the caching and group settings from the rest of the driver logic, so I would prefer to keep it in the same module as that code. (Splitting it out in the most obvious way would introduce a cycle, though that is avoidable without much pain.)
I do think that it will be worth splitting and maybe renaming emitmodule to separate GroupGenerator from the rest. My inclination is to do that in a follow-up PR to keep down the diff noise for this diff and to avoid needing to rebase some follow-up code I've already written, though I could do it now too if you think it's important.
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.
Sounds reasonable.
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.
Looks great! Only left some comments about additional documentation and possible tests. Incremental compilation will be a game-changer for making mypyc suitable for bigger projects.
Have you tried incremental compilation when compiling mypy/mypyc?
| self.mark = False | ||
|
|
||
|
|
||
| class MypycPlugin(Plugin): |
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.
Sounds reasonable.
mypyc/emitmodule.py
Outdated
| if compute_hash(meta_json) != ir_data['meta_hash']: | ||
| return None | ||
|
|
||
| # Check that all of the source files are present and as expected |
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.
Maybe elaborate a bit about when they may not be present or as expected (i.e. is this a normal condition or has something been corrupted).
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.
Will do. Main situation is if the user deleted build/ without deleting .mypy_cache, which is sort of corruption but also reasonable to expect that it will happen.
This works by reworking IR generation to proceed a SCC at a time and writing out caches of serialized IR information so that we can generated code that calls into a module without compiling the module in full. A mypy plugin is used to ensure cache validity by checking that a hash of the metadata matches and that all of the generated source is present and matches. Closes mypyc/mypyc#682.
065b3ef to
78b441f
Compare
This works by reworking IR generation to proceed a SCC at a time and
writing out caches of serialized IR information so that we can
generated code that calls into a module without compiling the module
in full. A mypy plugin is used to ensure cache validity by checking
that a hash of the metadata matches and that all of the generated source
is present and matches.
Closes mypyc/mypyc#682.