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

Distribute and install py.typed to provide type information #9279

Merged
merged 1 commit into from Feb 7, 2021
Merged

Distribute and install py.typed to provide type information #9279

merged 1 commit into from Feb 7, 2021

Conversation

jdufresne
Copy link
Contributor

Complies with PEP 561:
https://www.python.org/dev/peps/pep-0561/#packaging-type-information

By distributing and installing the py.typed file, mypy will use pip's
type information when imported into other projects. For example, the
pip-tools project can use pip's types and mypy to help verify
correctness.

mypy docs:
https://mypy.readthedocs.io/en/stable/installed_packages.html#making-pep-561-compatible-packages

@uranusjr
Copy link
Member

uranusjr commented Dec 15, 2020

pip does not support being imported directly. You can do it, but it’s not supported. I’m -1 unless py.typed has another valid use case.

@jdufresne
Copy link
Contributor Author

Thanks for taking the time to respond.

I understand pip's policy and I agree with it continuing. However, in this case, including py.typed is nearly trivial maintenance (it is only an empty file) and would assist projects importing pip anyway without the need for pip developers to provide API promises.

The facts remain:

  1. pip-tools is importing pip to achieve its goals
  2. Users find pip-tools' use cases useful enough such that it is a popular project
  3. pip includes some type information that could be used to help manage correctness of downstream projects

At some point, there will be the need for pip-adjacent features that are out of scope to be implemented in pip. Right now, pip-tools fills one of those roles. pip-tools isn't asking for any API stability, just a hand by providing PEP-561 compatible type information.

As the inclusion is only a single empty file, would you reconsider?

Thanks.

@pradyunsg
Copy link
Member

I don't think we're in any way concerned about including an empty file, but rather about what it indicates. ;)


None the less, I think adding this in for pip-tools is OK -- I'm a little worried that some folks might take this as an indicator for probing into pip's internals, but we got https://pip.pypa.io/en/stable/user_guide/#using-pip-from-your-program for that. :)

@pradyunsg
Copy link
Member

FWIW, you'll need to rebase this PR on the current master branch.

@uranusjr
Copy link
Member

Can we add content to py.typed to say we specifically do it for pip-tools? Not that it’d change anything (nobody reads documentation, less a supposed-to-be-empty file in source code), but that’s be a pretty cool thing to do :p

@jdufresne
Copy link
Contributor Author

you'll need to rebase this PR on the current master branch.

Done.

I'm a little worried that some folks might take this as an indicator for probing into pip's internals

In that case, shall I exclude the changelog note so as not to draw attention to it?

@jdufresne
Copy link
Contributor Author

@uranusjr That is a good idea! I'll look into it.

@pradyunsg
Copy link
Member

In that case, shall I exclude the changelog note so as not to draw attention to it?

Yea, that'd work.

@pfmoore
Copy link
Member

pfmoore commented Dec 15, 2020

For the record, I agree with @pradyunsg - it's more what having this file implies that bothers me.

I don't know enough about mypy to know why having an empty file with that name present makes such a difference, or why pip-tools can't get the same effect (which I presume is to do with taking account of the type annotations in pip?) by any other means, so I'm relying on other people's judgement over that point.

@pradyunsg
Copy link
Member

pradyunsg commented Dec 15, 2020

@pfmoore Quoting from the PEP linked in OP:

Package maintainers who wish to support type checking of their code MUST add a marker file named py.typed to their package supporting typing. This marker applies recursively: if a top-level package includes it, all its sub-packages MUST support type checking as well. To have this file installed with the package, maintainers can use existing packaging options such as package_data in distutils, shown below.

Basically, it indicates to mypy that it can use type annotations in-source on the project, rather than looking for them in typeshed (the common place for standard library + some third-party type annotations).

setup.py Outdated Show resolved Hide resolved
@pfmoore
Copy link
Member

pfmoore commented Dec 15, 2020

Basically, it indicates to mypy that it can use type annotations in-source on the project

That's what I gather. And it's impossible for pip-tools to tell mypy that it should use the annotations present in pip's code base unless pip adds that file? That sounds surprisingly strict to me - in the spirit of "consenting adults", I'd have expected a way to do this without needing explicit permission. And it's not possible for pip-tools to temporarily add that file to their copy of pip when type checking?

I guess the answer is that this just makes it more convenient for pip-tools, rather than there being no possible other way of doing it. But that comes back to whether we want to make it convenient for people to use pip's internals, even popular and important projects like pip-tools...

Ultimately, though, I don't feel strongly enough to object, as long as it's clear to everyone that it doesn't imply any sort of commitment on pip's part.

@jdufresne
Copy link
Contributor Author

After some experimenting, I have confirmed that the contents of py.typed do not matter. This can also be verified by inspecting mypy source code, which checks for existence only:

https://github.com/python/mypy/blob/05d9fb15dcec8bde4e7184ca387e7d8acea68792/mypy/modulefinder.py#L186-L187

So, the file now contains wording to emphasize that pip types and API are not stable. Suggestions on the wording are most welcome.

All other review feedback has also been included in the latest revision.

I'm not sure why my .trivial.rst file is is creating a CI error. If anyone understands what I should change, please let me know.

@pfmoore pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label Dec 15, 2020
@pfmoore
Copy link
Member

pfmoore commented Dec 15, 2020

I've labelled the PR as trivial so you don't need the .trivial.rst file.

Complies with PEP 561:
https://www.python.org/dev/peps/pep-0561/#packaging-type-information

By distributing and installing the py.typed file, mypy will use pip's
type information when imported into other projects. For example, the
pip-tools project can use pip's types and mypy to help verify
correctness.

mypy docs:
https://mypy.readthedocs.io/en/stable/installed_packages.html#making-pep-561-compatible-packages
@jdufresne
Copy link
Contributor Author

Thanks. Dropped the news file.

@pradyunsg
Copy link
Member

I'm not sure why my .trivial.rst file is is creating a CI error.

There's a bug in the pypa-bot, that I've not been able to identify yet. :(

@jdufresne
Copy link
Contributor Author

why pip-tools can't get the same effect (which I presume is to do with taking account of the type annotations in pip?) by any other means

...

And it's not possible for pip-tools to temporarily add that file to their copy of pip when type checking?

pip-tools recently started running mypy: jazzband/pip-tools#1275

Like pip itself, the project is using a pre-commit hook to run mypy. As far as I know, pre-commit can't be easily configured to run arbitrary commands prior to the hook (e.g. touch .../pip/py.typed). Even if it were able, to me, it feels like working against the tools when compared to the ease of pip distributing this empty file.

So ultimately, it could workaround this, but it would be cumbersome and inconvenient. I consider that a last resort option for the pip-tools project.

@uranusjr
Copy link
Member

uranusjr commented Jan 2, 2021

I think it is a worthwhile discussion whether a downstream package should be able to say this library does not contain a py.typed, but please pretend it does to typing tools. Someone should open that discussion somewhere (if it is not already).

@jdufresne
Copy link
Contributor Author

jdufresne commented Jan 2, 2021

Perhaps python/mypy#8545 and python/mypy#8512 cover your suggestion?

@jdufresne
Copy link
Contributor Author

pip-tools has been adding more type hints lately: jazzband/pip-tools@d9f50f1

Any thoughts on reconsidering this to help it out?

@pradyunsg
Copy link
Member

I'm on board for adding this.

Worst case, it's gonna be helpful to someone accessing pip._internal and, well... the "_internal" should've deterred them. 🤷🏽

@uranusjr
Copy link
Member

uranusjr commented Feb 7, 2021

Nobody seems to really object.

@uranusjr uranusjr merged commit bbf8466 into pypa:master Feb 7, 2021
@jdufresne jdufresne deleted the py.typed branch February 7, 2021 16:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants