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

Support egg installs with PEP 561 searching #5007

Closed
FuegoFro opened this issue May 5, 2018 · 14 comments
Closed

Support egg installs with PEP 561 searching #5007

FuegoFro opened this issue May 5, 2018 · 14 comments
Labels

Comments

@FuegoFro
Copy link
Contributor

FuegoFro commented May 5, 2018

When working on a repo containing types to be distributed in accordance with PEP 561 (such as https://github.com/numpy/numpy-stubs) it is surprisingly difficult to get mypy to actually see and respect those types. In particular, things that do not work (though I would expect them to):

  • Adding the directory containing the -stubs directory to the MYPYPATH
  • Running python setup.py install
  • Running pip install -e .

I eventually was able to make it work by running pip install ., but (particularly for things like running py.test while developing one of these stubs repos) this seems non-optimal.

Interestingly, adding the directory containing the -stubs directory to MYPYPATH did allow me to resolve types in the root of the package we care about (in this case numpy), but not in nested packages (in this case numpy.core). It looks like this might have something to do with this code where dir_chain is an empty string when looking for a top-level package and therefore find_lib_path_dirs doesn't filter out the items in lib_path. When we're trying to get a nested package it's non-empty and can't be found in the lib_path dirs, which then get filtered out. We also don't populate third_party_stubs_dirs in any of the above cases because the items on MYPYPATH aren't looked at for that (only site packages) and the other two methods don't create a bare directory named -stubs in the site packages (one creates a .egg and the other creates a .egg-link).

I did these experiments on commit 7fe60a547c476c8b341a47b87d72fc4460532102 of mypy with Python 3.6.4 on macOS.

Let me know if there's anything else I can provide which would be helpful! I'm not sure if this is more of a bug or a feature request because I couldn't tell from my reading of PEP 561 whether this use case was covered by that.

@emmatyping
Copy link
Collaborator

Running python setup.py install definitely should work and should be supported, as this does the same work that pip does to install packages. Can you please make sure that the package (the folder numpy-stubs) is installed after running python setup.py install? If not, then that is a setuptools issue.

Development installs on the other hand aren't intended to be supported, at least for now, as they don't put the files in the path, so you will need to handle this manually, just like before. I will look at the possibility of adding this.

@FuegoFro
Copy link
Contributor Author

FuegoFro commented May 8, 2018

Thank you for your quick reply!

I did a bit of digging into the python setup.py install case. It looks like I'm getting a numpy_stubs-0.0.1-py3.6.egg zip file installed in site-packages. Indeed at runtime I can do __import__("numpy-stubs") and that allows me to import the module. If I add zip_safe=False to the setup(...) call in setup.py I get a directory with the same name, and the same behavior (works at runtime, mypy still doesn't see the stubs). In both cases the easy-install.pth in site-packages has a line with ./numpy_stubs-0.0.1-py3.6.egg in it. I must admit my knowledge of this area of Python is pretty limited (eg how things are distributed, the difference between things like Eggs and Wheels, how much things like easy_install and pip share underlying mechanisms vs do things differently).

That's good to know and definitely understandable that the development use case (with pip install -e . or MYPYPATH) is not intended to be supported as of yet. One thing that might make this a little bite less confusing is if MYPYPATH didn't work at all; the current behavior where it only works for things in a top-level package can be misleading (or at least it was to me). I'm not sure if it would be a lot of work to make it not work at all, but if not I think that would help a bit :)

FuegoFro pushed a commit to FuegoFro/numpy-stubs that referenced this issue May 8, 2018
The stubs contained an unconditional reference to SupportsBytes, which only exists in Python 3. To make these valid on Python 2, conditionally import that Protocol in Python 3 and otherwise use a dummy class in Python 2. Also have `ndarray` extend `Contains`, while we're here.

This also extends the test suites to run all tests against both Python 2 and Python 3, with the ability to specify that certain tests should only be run against Python 3 (eg to test Python 3 exclusive operators). This should help prevent errors like this moving forward.

One downside of this is that flake8 doesn't understand the `# type:` comments, so it thinks that imports from `typing` are unused. A workaround for this is to add `# noqa: F401` at the end of the relevant imports, though this is a bit tedious.

Finally, change how test requirements are installed and how the `numpy-stubs` package is exposed to mypy, and update the README/Travis file to reflect this. See python/mypy#5007 for more details about the rational behind this change.
@gvanrossum
Copy link
Member

gvanrossum commented May 8, 2018 via email

@emmatyping
Copy link
Collaborator

That's good to know and definitely understandable that the development use case (with pip install -e . or MYPYPATH) is not intended to be supported as of yet.

I just took a look and this doesn't seem too hard to support, essentially we just need to look where the .egg-link file points us. I will try to take a look at it this week.

Hm, it does sound strange that MYPYPATH has any effect here at all (unless it's via the normal pre-PEP-561 code?).

Yes, I agree. @FuegoFro could you give exact commands that you issued? It will be helpful in understanding what the issue is.

I do think it would be nice for both python setup.py install and pip install -e . to work interchangeably, since they do so in other situations as well.

pip install -e . installs a development version of the distribution (like python setup.py develop). But I think you meant pip install ., which I agree should be equivalent to python setup.py install.

The complication is this doesn't always agree well with mypy's view "its a module if its a file in the path, or in a package on the path". This is almost the case except for egg distributions, which are either a directory containing the package, or a zipped version of it.

Though I don't like it, I think we should support egg installs, however weird they are. This would involve special casing files/directories ending in .egg in the site-package directories. If a *.egg item is a file, it is a zip distribution, and we can treat it as such (hopefully this can be abstracted away in the fscache?). Otherwise, just read the directory we want in the *.egg directory.

This will be a bit more painful than supporting editable installs.

@gvanrossum
Copy link
Member

gvanrossum commented May 8, 2018 via email

@emmatyping
Copy link
Collaborator

Hm, but I still wish pip install -e . would also work.

Yeah, this should be pretty easy to handle.

Perhaps we can ask the Python executable which you're asking for the location of its site-packages to do the digging for us?

Unfortunately, this would introduce a lot of overhead because we'd need to shell out for each package. Ideally we can special case egg installations and everything else should "just work".

@gvanrossum
Copy link
Member

gvanrossum commented May 8, 2018 via email

@FuegoFro
Copy link
Contributor Author

FuegoFro commented May 8, 2018

Regarding MYPYPATH, if the numpy-stubs directory is on either MYPYPATH or sys.path it will find the root numpy package but no sub packages. Here are some example commands and outputs (I have run pip uninstall numpy-stubs before running these):

~/src/numpy-stubs/tests$ python -m mypy pass/simple.py
pass/simple.py:4: error: No library stub file for module 'numpy'
pass/simple.py:4: note: (Stub files are from https://github.com/python/typeshed)

~/src/numpy-stubs/tests$  MYPYPATH=.. python -m mypy pass/simple.py
../numpy-stubs/__init__.pyi:4: error: No library stub file for module 'numpy.core._internal'
../numpy-stubs/__init__.pyi:4: note: (Stub files are from https://github.com/python/typeshed)

~/src/numpy-stubs/tests$  (cd ..; python -m mypy tests/pass/simple.py)
numpy-stubs/__init__.pyi:4: error: No library stub file for module 'numpy.core._internal'
numpy-stubs/__init__.pyi:4: note: (Stub files are from https://github.com/python/typeshed)

I'm pretty sure this is due to the find_lib_path_dirs function, which does indeed seem to be pre-PEP-561 code. In particular, when looking at the top level package, dir_chain will be an empty string, so find_lib_path_dirs will return all directories in lib_path. We'll then find the one we want (the numpy-stubs directory) in the subsequent logic. However when we have sub package (eg numpy.core._internal) then dir_chain will have a value like numpy/core. However none of the items on lib_path have a numpy/core directory in them (we instead have numpy-stubs/core), so we won't find that package.

Given this it seems like it wouldn't be too bad to support MYPYPATH by also searching for a dir_chain that modifies the top-level package to have -stubs appended to it, but from the discussions here it is unclear to me if supporting PEP 561 stubs via MYPYPATH is something that is even desired or makes sense.

@gvanrossum
Copy link
Member

gvanrossum commented May 8, 2018 via email

@emmatyping
Copy link
Collaborator

Hm, thinking about it more, looking for foo-stubs along $MYPYPATH (or
really the whole search path that mypy constructs) makes some sense.

Okay, I will try getting this to work after I add develop install support.

shoyer pushed a commit to numpy/numpy-stubs that referenced this issue May 10, 2018
* Ensure stubs are valid for Python 2 and fix running of tests

The stubs contained an unconditional reference to SupportsBytes, which only exists in Python 3. To make these valid on Python 2, conditionally import that Protocol in Python 3 and otherwise use a dummy class in Python 2. Also have `ndarray` extend `Contains`, while we're here.

This also extends the test suites to run all tests against both Python 2 and Python 3, with the ability to specify that certain tests should only be run against Python 3 (eg to test Python 3 exclusive operators). This should help prevent errors like this moving forward.

One downside of this is that flake8 doesn't understand the `# type:` comments, so it thinks that imports from `typing` are unused. A workaround for this is to add `# noqa: F401` at the end of the relevant imports, though this is a bit tedious.

Finally, change how test requirements are installed and how the `numpy-stubs` package is exposed to mypy, and update the README/Travis file to reflect this. See python/mypy#5007 for more details about the rational behind this change.

* Split `pip install .` out of the `test-requirements.txt` file, update Travis and README files accordingly.
@emmatyping emmatyping changed the title PEP 561 types hard to use locally Support egg installs with PEP 561 searching May 11, 2018
@emmatyping
Copy link
Collaborator

Good news! The implementation was a lot easier than I expected. I implemented and tested support for:

  • pip install -e . / python setup.py develop (egg-links)
  • python setup.py install on a setuptools project with zip_safe=False (uncompressed eggs, of which there are two types it seems...)

this leaves:

  • compressed eggs (which sadly is the default for setuptools)
  • possibly foo-stubs on the $MYPYPATH?

The format descriptions here are quite helpful: https://github.com/pypa/setuptools/blob/master/docs/formats.txt

@emmatyping
Copy link
Collaborator

Talking to Jason Coombs, eggs are essentially deprecated. Therefore, I will only work on adding support for egg-link. I will add to the documentation that packages should be installed with pip when I add support for that.

@lambda
Copy link

lambda commented May 19, 2018

I don't think the egg-links are relevant for this use case; as far as I can tell, they are only used for setuptools to be able find package metadata, not for actually setting your search path.

If you install with pip install -e . or python setup.py develop, the directories containing your packages are added to sys.path by virtue of being added to easy-install.pth in your site-packages directory. When site processes your site-packages directory, it loads pathnames from any .pth files and adds them to sys.path, as well as executing any lines beginning with import.

In mypy.sitepackages, only the site packages directories themselves are listed, there is no processing of .pth files:

def getsitepackages():
    # type: () -> List[str]
    if hasattr(site, 'getusersitepackages') and hasattr(site, 'getsitepackages'):
        user_dir = site.getusersitepackages()
        return site.getsitepackages() + [user_dir]
    else:
        return [get_python_lib()]

There does not appear to be any good way to re-use the logic in site for parsing .pth files, as it just imperatively manipulates sys.path. But it wouldn't be hard to parse .pth files by hand; or, you could just return sys.path, or sys.path filtered for only directories if you don't want to have to deal with zips. I've changed the implementation to the following, which seems to work well enough for me:

return [path for path in sys.path if os.path.isdir(path)]

Is there any reason not to just use this to get the exact same search path as you'd get if just executing the code?

@emmatyping
Copy link
Collaborator

emmatyping commented May 21, 2018

@lambda yes, your points are well made. I used the name "egg-link" instead of editable install, but I mean editable install, which is the type of installation you are talking about. I think the best plan is to inspect easy-install.pth, since this is simple and I'm not sure that I want to search sys.path, which should already be searched via lib_path.

Edit: Also keep in mind, this information needs to be statically found (as much as possible) for a python executable handed to mypy.

gvanrossum pushed a commit that referenced this issue Jul 2, 2018
This adds support for setuptool's egg format, which includes support for editable installs (`pip install -e .`/`python setup.py develop`)!

Setuptools creates its own directory to put the package we want to find. These directories are listed in `easy-install.pth` in a site-package directory.

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

No branches or pull requests

4 participants