Add daemon mode -- highly experimental #4130

gvanrossum · 2017-10-18T04:00:14Z

Many things probably need to change still, but I'd like to get this in before the merge conflicts get out of hand (there are a fair number of small changes to build.py, and 1-2 to main.py).

- Don't die if client closed commection. - Support `@file` to read arguments from a file. - Call build.build() directly.

…ta = None')

gvanrossum · 2017-10-18T04:02:00Z

mypy/main.py

+orig_stat = os.stat
+
+
+def stat_proxy(path: str) -> os.stat_result:


Hm, we probably don't need this function. (It was an experiment to count stat() calls, and while the numbers are useful, the implementation feels pretty wrong.)

gvanrossum · 2017-10-18T04:03:10Z

mypy/test/testdmypy.py

@@ -0,0 +1,308 @@
+"""Type checker test cases"""


This started out as a clone of testcheck.py, but there are many small changes.

I'd really like to be able to refactor the duplications in the test files in order to eventually clean up the test logic and improve the usability (#3973 being a first step). A comment about the nature of these differences may help with that.

gvanrossum · 2017-10-18T04:03:53Z

test-data/unit/check-incremental.test

@@ -3020,7 +3020,6 @@ tmp/mod.py:7: error: Revealed type is 'builtins.bytes'
 # cmd: mypy -m a
 # cmd2: mypy -m b
 # flags: --follow-imports=silent
-# flags2: --follow-imports=silent


This was just redundant, and its presence thwarted my logic for skipping test that vary flags across runs.

gvanrossum · 2017-10-18T04:04:29Z

setup.py

      data_files=data_files,
      classifiers=classifiers,
      cmdclass={'build_py': CustomPythonBuild},
-      install_requires = ['typed-ast >= 1.1.0, < 1.2.0'],
+      install_requires = ['typed-ast >= 1.1.0, < 1.2.0',
+                          'psutil >= 5.4.0, < 5.5.0',


This is used by dmypy only, to report memory usage.

gvanrossum · 2017-10-18T04:04:50Z

scripts/dmypy

@@ -0,0 +1,20 @@
+#!/usr/bin/env python3


Cloned from scripts/stubgen.

You might want to also add a entry_point in setup.py if you want something runnable from an installed mypy distribution.

It's there: https://github.com/python/mypy/pull/4130/files#diff-2eeaed663bd0d25b7e608891384b7298R107

Ah! Sorry, I missed that.

gvanrossum

@elazarg I've added some comments. In addition you should just diff testcheck.py and testdmypy.py to see some details (I scrubbed a lot of stuff because incremental is always true and the step is always >= 1 in testdmypy.py).

gvanrossum · 2017-10-18T20:34:53Z

mypy/test/testdmypy.py

+
+    @classmethod
+    def cases(cls) -> List[DataDrivenTestCase]:
+        if sys.platform == 'win32':


This is new (dmypy is not supported on Windows, and doesn't work there).

gvanrossum · 2017-10-18T20:35:47Z

mypy/test/testdmypy.py

+                  if cls.has_stable_flags(case) and cls.is_incremental(case)]
+        return c
+
+    def run_case(self, testcase: DataDrivenTestCase) -> None:


This method is specialized for incremental==True.

gvanrossum · 2017-10-18T20:36:21Z

mypy/test/testdmypy.py

+            tc = parse_test_cases(os.path.join(test_data_prefix, f),
+                                  None, test_temp_dir, True)
+            c += [case for case in tc
+                  if cls.has_stable_flags(case) and cls.is_incremental(case)]


These filters were added.

gvanrossum · 2017-10-18T20:37:38Z

mypy/test/testdmypy.py

+                    raise ValueError(
+                        'Output file {} exists though test case only has {} runs'.format(
+                            file, num_steps))
+        self.server = None  # type: Optional[dmypy.Server]


Reset the server here.

gvanrossum · 2017-10-18T20:37:56Z

mypy/test/testdmypy.py

+        for step in range(1, num_steps + 1):
+            self.run_case_once(testcase, step)
+
+    @classmethod


Next two methods are new.

gvanrossum · 2017-10-18T20:38:30Z

mypy/test/testdmypy.py

+        if os.path.exists(dn):
+            shutil.rmtree(dn)
+
+    def run_case_once(self, testcase: DataDrivenTestCase, incremental_step: int) -> None:


No default for incremental_step, and assert it's >= 1 below.

gvanrossum · 2017-10-18T20:40:50Z

mypy/test/testdmypy.py

+
+        # Parse options after moving files (in case mypy.ini is being moved).
+        options = self.parse_options(original_program_text, testcase, incremental_step)
+        if incremental_step == 1:


Initialize server here.

gvanrossum · 2017-10-18T20:41:37Z

mypy/test/testdmypy.py

+        for module_name, program_path, program_text in module_data:
+            # Always set to none so we're forced to reread the module in incremental mode
+            sources.append(build.BuildSource(program_path, module_name, None))
+        response = self.server.check(sources, alt_lib_path=test_temp_dir)


Call server.check() instead of build.build().

gvanrossum · 2017-10-18T20:42:18Z

mypy/test/testdmypy.py

+            update_testcase_output(testcase, a)
+        assert_string_arrays_equal(output, a, msg.format(testcase.file, testcase.line))
+
+        manager = self.server.last_manager


Get manager from server instead of from BuildResult.

gvanrossum · 2017-10-18T20:43:12Z

mypy/test/testdmypy.py

+                all(flag not in flag_list for flag in ['--python-version', '-2', '--py2'])):
+            options.python_version = testcase_pyversion(testcase.file, testcase.name)
+
+        options.use_builtins_fixtures = True


Hardcoding these flags here (rather than way up there).

JukkaL

This looks like a good starting point for the daemon mode. Left a bunch of minor comments.

JukkaL · 2017-10-19T14:02:19Z

mypy/dmypy.py

+# Argument parser.  Subparsers are tied to action functions by the
+# @action(subparse) decorator.
+
+parser = argparse.ArgumentParser(description="Client for mymy daemon mode",


JukkaL · 2017-10-19T14:38:40Z

mypy/dmypy.py

+    sys.stderr.flush()
+    pid = os.fork()
+    if pid:
+        npid, sts = os.waitpid(pid, 0)


Add comment saying that this happens in the original/current process.

JukkaL · 2017-10-19T14:42:02Z

mypy/dmypy.py

+# Misc utilities.
+
+def receive(sock: socket.socket) -> Any:
+    """Receive data from a socket until EOF."""


Document that this reads JSON data.

JukkaL · 2017-10-19T14:49:13Z

mypy/dmypy.py

+        if method is None:
+            return {'error': "Unrecognized command '%s'" % command}
+        else:
+            return method(self, **data)


The contents of data are not verified here, though elsewhere JSON is checked pretty carefully. At least add a comment about this here.

This is still pending, but this can be merged anyway.

JukkaL · 2017-10-19T14:53:13Z

mypy/build.py

@@ -122,7 +123,9 @@ def is_source(self, file: MypyFile) -> bool:
 def build(sources: List[BuildSource],
          options: Options,
          alt_lib_path: Optional[str] = None,
-          bin_dir: Optional[str] = None) -> BuildResult:
+          bin_dir: Optional[str] = None,
+          saved_cache: Optional['SavedCache'] = None,


Document saved_cache. Are we making some assumptions about it? Does it get mutated?

JukkaL · 2017-10-19T15:07:17Z

mypy/dmypy.py

+
+import mypy.build
+import mypy.errors
+import mypy.main


Eventually we could import most mypy modules lazily so that the client would run quicker in case the server is already running.

JukkaL · 2017-10-19T15:08:25Z

mypy/dmypy.py

+    """
+    try:
+        pid, sockname = get_status()
+    except SystemExit as err:


I'd prefer having a separate exception class for daemon errors, and a top-level try/except that will translate it to SystemExit.

JukkaL · 2017-10-19T15:30:50Z

mypy/dmypy.py

+    def check(self, sources: List[mypy.build.BuildSource],
+              alt_lib_path: Optional[str] = None) -> Dict[str, Any]:
+        bound_gc_callback = lambda phase, info: self.gc_callback(phase, info)
+        self.gc_start_time = None  # type: Optional[float]


I'd recommend moving the stats handling code to separate methods to keep the logic of this method less cluttered.

JukkaL · 2017-10-19T15:45:34Z

I encountered something that looks like a performance problem. I did these steps:

Remove mypy cache.
Create a tiny new file mypy/x.py with a single function that generates a type check error.
Run scripts/dmypy check mypy (takes about 13 seconds). It generates an error as expected.
Run scripts/dmypy recheck. It still takes about 13 seconds but I'd expect it to only take a fraction of a second, since only mypy.x needs to be processed.

gvanrossum · 2017-10-19T15:57:04Z

The performance problem is apparently not new -- I can reproduce this with standard -i mode as well. No cache entries for the mypy package are written somehow -- perhaps because mypy.x precedes all other submodules of mypy. This could be due to an edge case in the topsort.

JukkaL · 2017-10-19T16:14:16Z

In that case I'll create a new issue to track it.

JukkaL · 2017-10-19T16:43:06Z

Created #4135 to track the performance issue.

This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.

ilevkivskyi · 2017-10-20T21:17:25Z

mypy/build.py

@@ -339,6 +350,27 @@ def default_lib_path(data_dir: str,
 # silent mode or simply not found.


+def cache_meta_from_dict(meta: Dict[str, Any], data_json: str) -> CacheMeta:
+    sentinel = None  # type: Any  # the values will be post-validated below


This comment should be now "validated by the caller" or similar.

This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.

Guido van Rossum added 30 commits October 13, 2017 11:24

Add dmypy: experimental daemon mode.

f389250

Turn serving code into a class

0108b64

Tweaks to daemonize()

97d6c6d

Different approach to debugging. Different approach to running mypy.

bfde4ac

Improve processing of file args

9688716

Simplify command dispatch

b4ede7d

Fix crash in validate_meta() when --skip-version-check given

6bf9c7e

Small fixes

f92c849

- Don't die if client closed commection. - Support `@file` to read arguments from a file. - Call build.build() directly.

Disallow report-generating options, --no-incremental, and --quick

34fe874

Save sources, not raw strings

6635e95

Primitive tracing of os.stat() calls

5c3786a

Save and reuse meta and tree

68cc680

Revive self.meta when writing cache (hope this works)

1f754b4

Fix mypy and lint errors

9148acf

Tweak logging code

398d649

Delete saved cache entry; tweak log message

75a9a16

Added tests for dmypy (some still failing)

c221225

Skip tests that specify varying flags/options

6739a06

Include incremental tests from other files (not many)

af6db88

Fix 6 test errors caused by not deleting meta for files with errors

7cd4e44

Fix broken check for mypy.ini.* in has_stable_flags()

cf0469e

Don't compare options; it doesn't work right

798b7e8

Revive cache verification after each test step

d916f29

Fix final two failing tests (it was wrong to delete the line 'self.me…

69de33a

…ta = None')

Fix mypy errors; clean up file/socket better

d4bcd07

New approach to instrumentation

f8034d3

Add GC statistics (and increase GC threshold!)

a4d5d79

Report memory usage using getrusage() and psutil

657a55d

Older Pythons don't load JSON from bytearray

b72c16d

Refactor status reading/reporting

9e0df24

Guido van Rossum added 2 commits October 17, 2017 17:13

Add support for daemon mode to misc/incremental_checker.py

80e60b0

When starting daemon wait for it to be serving

2caaf0b

gvanrossum commented Oct 18, 2017

View reviewed changes

Guido van Rossum added 3 commits October 17, 2017 21:15

Add testdmypy to PYTEST_FILES (fixes runtests.py)

808ac7f

Skip dmypy tests on Windows

e6b46ea

Move 'import resource' into the function that uses it

bc15106

gvanrossum commented Oct 18, 2017

View reviewed changes

JukkaL reviewed Oct 19, 2017

View reviewed changes

Respond to code review (in some cases by adding a TODO)

c9bb0d1

JukkaL mentioned this pull request Oct 19, 2017

Slow incremental run when single module has errors #4135

Closed

Guido van Rossum added 3 commits October 19, 2017 09:49

Fix stricter mypy errors

517129d

Fix lint errors

ba827e2

Simplify bound_gc_callback

626566b

JukkaL approved these changes Oct 20, 2017

View reviewed changes

JukkaL merged commit 2dbeaf7 into python:master Oct 20, 2017

JelleZijlstra added a commit that referenced this pull request Oct 20, 2017

add psutil to test-requirements.txt

c54b706

This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.

JelleZijlstra mentioned this pull request Oct 20, 2017

add psutil to test-requirements.txt #4141

Merged

gvanrossum pushed a commit that referenced this pull request Oct 20, 2017

add psutil to test-requirements.txt (#4141)

7b65ff5

This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.

ilevkivskyi reviewed Oct 20, 2017

View reviewed changes

gvanrossum pushed a commit to gvanrossum/mypy that referenced this pull request Oct 20, 2017

Fix comment in response to python#4130 review

5ab0364

gvanrossum pushed a commit to gvanrossum/mypy that referenced this pull request Oct 24, 2017

Fix comment in response to python#4130 review

466998e

gvanrossum pushed a commit to gvanrossum/mypy that referenced this pull request Oct 25, 2017

Fix comment in response to python#4130 review

8ee1d21

gvanrossum pushed a commit to gvanrossum/mypy that referenced this pull request Oct 27, 2017

Fix comment in response to python#4130 review

9acaae3

JukkaL pushed a commit that referenced this pull request Oct 31, 2017

add psutil to test-requirements.txt (#4141)

285f5c3

This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.

gvanrossum pushed a commit to gvanrossum/mypy that referenced this pull request Nov 1, 2017

Fix comment in response to python#4130 review

18efb5c

crusaderky mentioned this pull request May 3, 2019

psutil version limitations #6758

Closed

		orig_stat = os.stat


		def stat_proxy(path: str) -> os.stat_result:

Add daemon mode -- highly experimental #4130

Add daemon mode -- highly experimental #4130

Conversation

gvanrossum commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmatyping Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL commented Oct 19, 2017

gvanrossum commented Oct 19, 2017

JukkaL commented Oct 19, 2017

JukkaL commented Oct 19, 2017

Choose a reason for hiding this comment

emmatyping Oct 18, 2017 •

edited

Loading