Skip to content
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

Daemon support for --follow-imports=normal #5870

Closed
msullivan opened this issue Nov 5, 2018 · 8 comments · Fixed by #8654
Closed

Daemon support for --follow-imports=normal #5870

msullivan opened this issue Nov 5, 2018 · 8 comments · Fixed by #8654

Comments

@msullivan
Copy link
Collaborator

Currently the daemon only supports errors and ignore, which is a big limitation.
Additionally, while we reject follow-imports=normal as a top-level option, we
silently allow it for module-level definitions while not correctly implementing it.
(Including for all stubs, as in #5566.)

@msullivan
Copy link
Collaborator Author

The main difficulty in supporting import following comes from detecting when a module that is imported but where the error for the missing import is suppressed. For example if we have:

import foo  # type: ignore
foo.bar(10)

and we fail to find foo, we will give everything in it an Any type and carry on. If in a subsequent run a foo.py or foo.pyi (or foo/__init__.py or foo/__init__.pyi... and some -stubs directories) is added somewhere on mypy's search path, we need to process it and then recheck any uses of foo.

To make things worse, files that do exist can get preempted by another module being found earlier in the search path.

The problem is that there can be a lot of places that any given module might live. We infer a search path of root directories based on the sources passed in and where __init__.py files live, and if you have ~200 root directories, there are about ~1000 places where each module might be found. We can't watch all of those files.

@emmatyping
Copy link
Collaborator

if you have ~200 root directories, there are about ~1000 places where each module might be found. We can't watch all of those files.

If I am understanding things correctly, it sounds to me like we should keep track of changes via something like watchdog? Then we can greatly limit the search path of changes to just those where a file was created? In fact, it should be a lot easier to just filter the file paths watchdog gives us while the daemon is running by the set of search directories, then we know exactly what relevant changes were made. The number of total changes made should be relatively small (in the dozens) instead of huge (in the 1000s).

@ilevkivskyi
Copy link
Member

@ethanhs the recently added daemon flags --update/--remove allow simple "external" integration with such tools (watchman is another example).

If we will not find other solution, then we can consider making one of such services a dependency. We can make it an optional dependency, for example dmypy start --follow-imports=error will work as previously, but dmypy start --follow-imports=normal will fail unless run as dmypy start --follow-imports=normal --fswatcher=<service>. Here <service> may be a shell script with minimal API, or maybe choice of supported services for which we know the API like watchdog or watchman.

@emmatyping
Copy link
Collaborator

@ilevkivskyi ah, then I guess we just need to have a good interface for selecting a backend for this information.

I like the dmypy start --follow-imports=normal --fswatcher=<service> spelling. I would strongly suggest just calling watchman/watchdog APIs directly, as shell scripts are not particularly cross platform.

@matangover
Copy link
Contributor

matangover commented Oct 21, 2019

This limitation is blocking usage of the daemon with PEP 561 packages that use inline type information. However, PEP 561 packages that have type information in pyi files do work (#5566).

This inconsistency is quite surprising. I can imagine a package moving their type info from pyi to py and suddenly ceasing to work with the daemon.

Personally, I think package imports should be followed by the daemon. The original concern of tracking updates isn't really relevant here – installed packages rarely change (unlike user code). In case of installing/updating/removing a package, the daemon can be restarted.

So maybe there should be a follow_imports_for_installed_packages option? Similar to follow_imports_for_stubs, when set to 'False' it will override follow_imports to always use 'normal' for installed packages. I would argue that this should be set to False by default, but then there's backwards compat concerns (?). In any case, it seems like full --follow-imports=normal for the daemon will not be supported anytime soon because it's difficult to implement and isn't a common use case.

Repro steps:

$ echo "import hello" > test.py

# Package with inline type information: works with mypy, not with dmypy
$ pip install "git+https://github.com/ymyzk/pep-561-samples#subdirectory=hello-typed1"
$ mypy test.py
Success: no issues found in 1 source file
$ dmypy run -- --follow-imports=error test.py; dmypy stop
test.py:1: error: Cannot find module named 'hello'

# Package with type information in stubs: works with both mypy and dmypy
$ pip uninstall hello-typed1
$ pip install "git+https://github.com/ymyzk/pep-561-samples#subdirectory=hello-typed2"
$ mypy test.py
Success: no issues found in 1 source file
$ dmypy run -- --follow-imports=error test.py; dmypy stop
Success: no issues found in 1 source file

If it's useful to anyone, there is a workaround that uses the 'bug' described by @msullivan in the first post – creating mypy.ini with the following contents will allow the daemon to follow imports to package that use inline type info:

[mypy]
[mypy-hello]
follow_imports=normal

@matangover
Copy link
Contributor

matangover commented Oct 21, 2019

Another idea: a --follow-imports=once option (supported by daemon only) which is essentially the current functionality of 'follow-imports=normal' but makes it explicit for users that the daemon will not track updates to dependencies that aren't specified in the sources on the command line.

I can work on a PR if you accept.

@msullivan
Copy link
Collaborator Author

That is a reasonable strategy, I think. I would probably accept a PR to do that.

@JukkaL JukkaL assigned JukkaL and unassigned msullivan Apr 1, 2020
JukkaL added a commit that referenced this issue Apr 3, 2020
This includes a bunch of changes to following imports:

* Fix bug with cached modules without ASTs by loading the AST as needed
* Improve docstrings and simplify code
* Rename `follow_imports`
* Merge `seen` and `sources_set`
* Address some TODO items

This may be easier to review by looking at individual commits.

I think that following imports is ready to be enabled after this has been
merged. We can always revert the change before the next release if we
we encounter major issues.

Major things I know about that still don't work (I'll try to address at least the 
first one before the next release):

* `-m` and `-p` don't work correctly with dmypy (I think that happens also
  in other modes, but might be worse when following imports)
* Suppressed modules are incorrect with namespace modules (happens
  also in other modes)
* File events are not supported (but these are undocumented, so it's
  not critical?)
* Performance with a huge number of files is unknown (other import modes
  have better performance and can be used as fallbacks if this is a problem)

Work towards #5870.

Continues progress made in #8590.
JukkaL added a commit that referenced this issue Apr 9, 2020
We already have a bunch of tests that use this. Now enable it on the
command line so that it's possible to start using this for real and to
do interactive testing.

Closes #5870.
JukkaL added a commit that referenced this issue Apr 9, 2020
We already have a bunch of tests that use this. Now enable it on the
command line so that it's possible to start using this for real and to
do interactive testing.

Closes #5870.
@vjpr
Copy link

vjpr commented Apr 22, 2020

FYI: The linked PR has enabled this on master.

python3 -m pip install -U git+git://github.com/python/mypy.git

@JukkaL Any idea on timeline for this being released?

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

Successfully merging a pull request may close this issue.

6 participants