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

Add --add/--remove/--update flags to 'dmypy recheck' #5745

Merged
merged 14 commits into from Oct 16, 2018

Conversation

Projects
None yet
3 participants
@gvanrossum
Copy link
Member

gvanrossum commented Oct 6, 2018

Using dmypy recheck --add <files> --remove <files> --update <files> will take the previous list of files, apply adds/removes/updates, and then check the resulting list of sources, without calling stat() on any of the unchanged files. (Useful if the caller is using e.g. Watchman.) Each list of <files> may be empty in case there are no files in that category.

For backwards compatibility, if none of --add, --remove or --update is used, dmypy recheck will revert to its old behavior (i.e. re-check the exact same list of files, calling stat() on each file to determine whether it has changed). Note that even if the list of <files> is empty, the presence of the new flags triggers the new behavior.

There are also some cleanups in the diff. (And some loose ends that I will try to address next week.)

if not self.fine_grained_manager:
return {'error': "Command 'recheck' is only valid after a 'check' command"}
return self.check(self.previous_sources)
print(f"\nadd={add}, remove={remove}, update={update}")

This comment has been minimized.

@gvanrossum

gvanrossum Oct 6, 2018

Member

This print() still needs to be removed.

@gvanrossum gvanrossum changed the title Add support for dmypy recheck with given set add/remove/update Add --add/--remove/--update flags to 'dmypy recheck' Oct 6, 2018

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Oct 7, 2018

I don’t believe this is right though. It ignores typeshed.

"""
self.remove_watched_paths(remove)
self.add_watched_paths(add)
return self._find_changed(add + update)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 11, 2018

Collaborator

I didn't do a careful review, but generally this looks good. I just have one question to clarify (and it is probably worth mentioning in the docstring): Is it OK to pass files to --remove that were never in the build?

My motivation is to avoid duplication of work in the daemon and in the runner script that filters sources (what we do internally). If only files that were previously in the build are allowed in --remove, then we will need to keep full list of the checked files also in the runner script (because there is no way to check whether already deleted file should be filtered or not, if the filtering is content based, like presence of type comments/annotations).

I would prefer therefore to allow passing arbitrary files to --remove and explicitly document this.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 11, 2018

Collaborator

To clarify, from the current implementation in this PR it looks like it should work, I just would like to make this explicit.

gvanrossum added some commits Oct 2, 2018

Changes to 'recheck' to truly support --add/remove/update.
However there's still a flaw: it makes a bunch of stat() calls that
'dmypy recheck' (without --update) doesn't make.  Example (after
updating a comment in mypy/fswatcher.py):
```
add=['mypy/fswatcher.py'], remove=None, update=None
Good stat('mypy/__init__.py')
LOG:  fine-grained increment: cmd_recheck: 0.000s
Good stat('mypy/fswatcher.py')
Real read('mypy/fswatcher.py')
LOG:  fine-grained increment: find_changed: 0.001s
LOG:  fine-grained: ==== update 'mypy.fswatcher' ====
LOG:  fine-grained: --- update single 'mypy.fswatcher' ---
LOG:  Metadata not found for mypy.fswatcher
LOG:  Parsing mypy/fswatcher.py (mypy.fswatcher)
LOG:  fine-grained: triggered: ['<mypy.fswatcher.FileSystemWatcher._find_changed>', '<mypy.fswatcher.FileSystemWatcher.update_changed>', '<mypy.fswatcher.FileSystemWatcher[wildcard]>']
LOG:  fine-grained: process: mypy.dmypy_server.Server.update_changed
Good stat('.')
Good stat('/Users/guido/src/mypy')
Good stat('/Users/guido/src/mypy/mypy/typeshed/stdlib/3.6')
Good stat('/Users/guido/src/mypy/mypy/typeshed/stdlib/3.5')
Good stat('/Users/guido/src/mypy/mypy/typeshed/third_party/3.5')
Good stat('/Users/guido/src/mypy/mypy/typeshed/stdlib/3')
Good stat('/Users/guido/src/mypy/mypy/typeshed/third_party/3')
Good stat('/Users/guido/src/mypy/mypy/typeshed/stdlib/2and3')
Good stat('/Users/guido/src/mypy/mypy/typeshed/third_party/2and3')
LOG:  fine-grained: triggered: []
LOG:  fine-grained: update once: mypy.fswatcher in 0.081s - 0 left
LOG:  fine-grained increment: update: 0.001s
Stats for FileSystemCache:
  len(stat_cache)=11
  len(stat_error_cache)=82
    {'mypy/__init__.pyi': FileNotFoundError(2, 'No such file or directory'), '/Users/guido/src/mypy/test-data/packages/typedpkg/mypy-stubs': FileNotFoundError(2, 'No such file or directory'), '/Users/guido/src/mypy/test-data/packages/typedpkg/mypy/py.typed': FileNotFoundError(2, 'No such file or directory'), '/Users/guido/src/mypy/test-data/packages/typedpkg/mypy/fscache/py.typed': FileNotFoundError(2, 'No such file or directory'), '/Users/guido/src/mypy/test-data/packages/typedpkg/mypy/fscache/File...kages/psutil-stubs': FileNotFoundError(2, 'No such file or directory'), '/Library/Python/3.6/site-packages/psutil/py.typed': FileNotFoundError(2, 'No such file or directory'), '/Users/guido/Library/Python/3.6/lib/python/site-packages/psutil-stubs': FileNotFoundError(2, 'No such file or directory'), '/Users/guido/Library/Python/3.6/lib/python/site-packages/psutil/py.typed': FileNotFoundError(2, 'No such file or directory'), '/usr/local/lib/mypy': FileNotFoundError(2, 'No such file or directory')}
  len(listdir_cache)=9
  len(listdir_error_cache)=18
  len(isfile_case_cache)=18
  len(read_cache)=1
  len(read_error_cache)=0
  len(hash_cache)=1
  len(fake_package_cache)=0
```
Revert "Optimization to avoid stat() calls from is_module()"
This reverts commit d068bf3.

I don't trust the fix (it doesn't take typeshed into account).  Also
it doesn't really belong in this diff -- the same problem appears with
the original `dmypy recheck` command.
@msullivan
Copy link
Collaborator

msullivan left a comment

This looks good to me

sources = self.previous_sources
if remove:
removals = set(remove)
print

This comment has been minimized.

@msullivan

msullivan Oct 16, 2018

Collaborator

Stray print

@gvanrossum gvanrossum force-pushed the gvanrossum:dmypy-recheck-update branch from ed6f019 to 6bc8984 Oct 16, 2018

@gvanrossum gvanrossum merged commit 22b5ac0 into python:master Oct 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gvanrossum gvanrossum deleted the gvanrossum:dmypy-recheck-update branch Oct 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment