The mother of all pull requests: add --incremental #1292

gvanrossum · 2016-03-14T18:47:57Z

tl;dr: Please review in this order: semanal.py+tests; nodes.py+types.py; fixup.py; build.py; main.py.

See this Google Doc for some technical debt (read the paper from the top for a more complete overview).

Despite that technical debt I'd like to merge this into master in the current state, because at this point it works well enough and in particular I've tested it with typeshed's runtests.py script as well as with the selection of files in the internal Dropbox server repo. Some considerations:

The new (SCC + topsort) algorithm for determining dependencies is always used
The incremental (cache) feature is only used when -i/--incremental is used
After merging this would require additional real-world testing before we can do a release
My own biggest concern is the hack for the sqlalchemy cycle: gvanrossum@ede8184

Don't be fooled by the large number of commits! There are a lot of dead ends and U-turns that you can ignore. Most of the changes are limited to a small number of files:

build.py: the new dependency manager (I ripped out the old one)
nodes.py, types.py: added [de]serialize() methods to many classes
fixup.py: new module to fix up symbol tables after deserialization
main.py: added the -i/--incremental option
semanal.py: a fix for anonymous namedtuples needed by serialization
some tests added/changed for the namedtuple fix
.gitignore the .mypy_cache directory (though not even sure I want this)

Somehow GitHub refuses to show the diffs for build.py. Sorry about this; I guess you have to view them locally. You could also try to view the incremental diffs to this file starting with 0b4ef76. (Also note that the dates GitHub lists for the diffs are bogus -- they represent the most recent rebase. The actual work started over a month ago.)

Some questions about how to merge this monster into master. I've got a feeling that the detailed commits from my branch are not that useful. I propose the following order, as separate commits to master:

the semanal.py changes and related tests
(de)serialization methods in nodes.py, types.py
the rest: fixup.py, build.py, main.py, .gitignore

This may also represent a reasonable review order.

… every type occurring in mypy.

…ough check on what we wrote.

…ging.

… tack for fixup.

…ion of OverloadedFuncDef (type was missing).

…ping/abc cluster.

… to use transitive closure.

…ways lists.

…D_IMPORTED.

gvanrossum · 2016-03-31T01:47:02Z

Awesome! Looks like you caught quite a few things (and found a lot of prints :-). Hopefully I can add finishing this as one of our OKRs for the next half-sprint.

…for error conditions instead.

Serialize some additional fields, clarify a few places, add some comments, replace a print() with an assert.

Made a few things non-optional, removed some duplicate defs, added a TODO comment.

- Add/update many docstrings and comments. - Remove dead code at end of write_cache(). - Turn cache inconsistency into assert False. - Use keyword args for State() constructor calls.

gvanrossum · 2016-04-06T20:40:30Z

Hi @JukkaL, I've pushed a new version that incorporates my responses to all your feedback except for the long last one (starting with "Also L1089 in build.py"). I decided not to write individual "done / no, here's why" responses but instead just do everything or explain what's going on in comments.

Next I'm going to work on testing.

…ockers. Also move normalize_error_messages() to helpers, where it belongs. This is in preparation of introducing tests for incremental mode. CompileError is still raised for blockers.

gvanrossum · 2016-04-07T19:33:31Z

Jukka, I now have a decent start on the unittest infrastructure.

There's a new programmatic test module, testgraph.py, that has some tests for topsort() and friends -- it's easy to add more as long as the tests don't require too much setup.
There's also a new mechanism for data-driven tests of incremental mode. This is implemented in testcheck.py; the new behavior (roughly what you suggested) is triggered either by a test filename containing 'incremental' or by a test name containing 'Incremental'. (We can change the exact conditions easily.) The mechanism runs each such test once with a cold cache expecting no errors, then again with a warn cache after updating files for which a ".next" version is found, expecting the errors from the test's [out] section. The code also checks that valid cache files exist exactly for those modules that don't have any errors (after each run).
I've only written three very basic data-driven tests for now, but it should be easy to add more.

gvanrossum · 2016-04-07T20:32:46Z

So I think I have addressed everything in your review now except

Serializing line numbers (I haven't found the need so far)
Your "Bonus" bullet for the test architecture (expecting specific files to be rechecked)
The answer on BFS vs. DFS -- I added a reminder comment, but I'm not even sure the old code did DFS
Adding more extensive tests (but you didn't really ask for that -- you said "a few" and I've got that :-)

I think I'd like to leave all those unaddressed.

JukkaL · 2016-04-08T15:16:09Z

mypy/nodes.py

@@ -630,7 +653,7 @@ def is_generic(self) -> bool:
        return self.info.is_generic()

    def serialize(self) -> JsonDict:
-        # Not serialized: defs, base_type_exprs
+        # Not serialized: defs, base_type_exprs,d ecorators


typo: d ecorators

JukkaL · 2016-04-08T16:32:19Z

OK, looks good now! Feel free to merge whenever you like. After merging I suggest filing some new issues (for 0.3.2, I assume):

Add extensive tests for incremental type checking. I'd like more tests before turning incremental mode on by default (+ more manual testing).
Check which files were rechecked in incremental mode test cases (and which weren't).

I'm not sure either sure whether the old code used DFS, but I have a vague feeling that it might.

gvanrossum · 2016-04-08T17:46:14Z

Filed: #1349 (add tests), #1350 (check which files we rechecked).

Guido van Rossum added 30 commits March 12, 2016 21:07

Dump/load infrastructure set up (data [de]serialization is bogus).

3055df2

Major redo; dependency processing now uses build manager states.

466eb79

Start on serialization.

895fe54

Totally different approach to serialization.

9ab4020

Improved log()/trace() API. Improved handling cached dependencies.

68b4326

Serialize more types. We can now serialize (but not yet deserialize!)…

0f8dcdc

… every type occurring in mypy.

Start fixing up nodes after deserialization().

44b2828

Another big step towards correct [de]serialization. Added fairly thor…

f232056

…ough check on what we wrote.

Progress towards fixing up more cross references. But also more debug…

d2491cd

…ging.

Do not serialize mro -- recompute it.

6dc9ba7

Instance.serialize() always writes the TypeInfo's fullname. Different…

f5aabc2

… tack for fixup.

Fixing the fixup code. Too much debugging prints though.

bf291ab

Less debug output. The type fixer should now be (mostly) complete.

a182fd7

Lots more debugging around MRO calculation. Fixed a bug in serializat…

e6d7f5c

…ion of OverloadedFuncDef (type was missing).

Fix a tricky issue where MRO linearization failed for the builtins/ty…

7f2e698

…ping/abc cluster.

Improved (de)serialization of TypeInfo. Multi-pass fixup algorithm.

90338c5

Remove redundant final_passes(); it was a no-op.

c48ed17

Refactor build states, on our way to cycle resilience.

2fcc2e0

Various small cleanups.

7a83a16

Rewrite fixit.lookup_qualified(). Rewrite is_ready() for cache states…

924b08f

… to use transitive closure.

Be less shy about the type of variables and bound_vars -- they are al…

e7d5772

…ways lists.

Serialize decorators (at least simple ones). Explicitly ignore UNBOUN…

2d1c366

…D_IMPORTED.

Fix up ct.bound_vars. Missing arg types are okay.

dadb7e8

Use log() instead of print() for "FIXING MODULE PASS N".

56dc1e2

Add visit decorator and overloaded.

080b498

Serialize UnboundType.

2ed9bb2

Prototype approach to class C(NamedTuple("C", ...)): ...

93e14d1

Make the type fixer/cleaner recurse into Instance.args.

adf7d6e

Serialize TypeList and EllipsisType.

92ecd7e

Serialize Argument.type_annotation.

2754695

Guido van Rossum added 6 commits April 5, 2016 16:55

Merge branch 'master' into incremental

165a8a1

Merge branch 'master' into incremental

54d70ce

Remove all print statements and pdb calls from fixup.py. Add asserts …

cd5ec0d

…for error conditions instead.

Respond to code review for nodes.py.

9d2c327

Serialize some additional fields, clarify a few places, add some comments, replace a print() with an assert.

Respond to code review for types.py.

3a2cc0b

Made a few things non-optional, removed some duplicate defs, added a TODO comment.

Respond to code review for build.py.

ba1553d

- Add/update many docstrings and comments. - Remove dead code at end of write_cache(). - Turn cache inconsistency into assert False. - Use keyword args for State() constructor calls.

Guido van Rossum added 4 commits April 6, 2016 15:15

Add a few unit tests for the graph processing in build.py.

84f0d86

Add manager and error messages to BuildResult; don't raise for non-bl…

d6800fe

…ockers. Also move normalize_error_messages() to helpers, where it belongs. This is in preparation of introducing tests for incremental mode. CompileError is still raised for blockers.

Fix docstrings for AssertionFailure and SkipTestCaseException.

a9cdd9b

Added support for checking incremental mode.

5594a1b

gvanrossum mentioned this pull request Apr 7, 2016

Faster, incremental type checking #932

Closed

Guido van Rossum added 3 commits April 7, 2016 12:43

Fully annotate testcheck.py

700d959

Fully annotate testgraph.py.

842a5e1

Merge remote-tracking branch 'origin/master' into incremental

bb7f9c1

Add missing annotations to helpers.py.

cb00db6

JukkaL reviewed Apr 8, 2016
View reviewed changes

Fix typo in comment.

b513603

gvanrossum merged commit 288f319 into python:master Apr 8, 2016

gvanrossum mentioned this pull request Apr 8, 2016

Turn on the incremental cache by default #1352

Closed

gvanrossum added the topic-incremental label Apr 8, 2016

elazarg mentioned this pull request Sep 29, 2016

Add a new, lower priority for imports inside "if MYPY" #2167

Merged

hauntsaninja mentioned this pull request Aug 6, 2021

Fix caching behavior of PEP561-installed namespace packages #10937

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The mother of all pull requests: add --incremental #1292

The mother of all pull requests: add --incremental #1292

gvanrossum commented Mar 14, 2016 •

edited

Loading

gvanrossum commented Mar 31, 2016 via email

gvanrossum commented Apr 6, 2016

gvanrossum commented Apr 7, 2016

gvanrossum commented Apr 7, 2016

JukkaL Apr 8, 2016

JukkaL commented Apr 8, 2016

gvanrossum commented Apr 8, 2016

The mother of all pull requests: add --incremental #1292

The mother of all pull requests: add --incremental #1292

Conversation

gvanrossum commented Mar 14, 2016 • edited Loading

gvanrossum commented Mar 31, 2016 via email

gvanrossum commented Apr 6, 2016

gvanrossum commented Apr 7, 2016

gvanrossum commented Apr 7, 2016

JukkaL Apr 8, 2016

Choose a reason for hiding this comment

JukkaL commented Apr 8, 2016

gvanrossum commented Apr 8, 2016

gvanrossum commented Mar 14, 2016 •

edited

Loading