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

Include py.typed file in distribution (PEP 561) #7073

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

tlambert03
Copy link
Contributor

Description

In order for the .pyi files to be useful, scikit-image needs to include a py.typed file in top level module of the distribution.

See PEP 561 and the mypy documentation for more information.

currently, when using scikit-image in an IDE, you get:
Screen Shot 2023-07-21 at 7 48 08 PM

if you drop py.typed file in the skimage folder of site-packages you get:

Screen Shot 2023-07-21 at 7 48 44 PM

@tlambert03 tlambert03 changed the title Pytyped Include py.typed file in distribution (PEP 561) Jul 22, 2023
@lagru lagru added the 🔧 type: Maintenance Refactoring and maintenance of internals label Jul 22, 2023
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this Talley! Seems obvious that we want to include this change.

Though, I can't seem to reproduce this with PyCharm on my side; behavior is identical with and without py.typed. And PyCharm finds the docstring without the file. Which IDE are you using, a vscode-based one?

@tlambert03
Copy link
Contributor Author

Yep, vscode

@tlambert03
Copy link
Contributor Author

If I recall correctly, pycharm actually does dynamic imports for a number of things?

@lagru lagru merged commit 8c3c324 into scikit-image:main Aug 15, 2023
22 of 23 checks passed
@lagru
Copy link
Member

lagru commented Aug 15, 2023

Thanks @tlambert03. :)

@stefanv stefanv added this to the 0.22 milestone Aug 15, 2023
@tlambert03 tlambert03 deleted the pytyped branch September 11, 2023 17:54
@darosio
Copy link

darosio commented Oct 4, 2023

I was used to a single:
import skimage # type: ignore
or a few:
from skimage.morphology import disk # type: ignore

Now I need a "# type: ignore" for every still untyped function I use to make the mypy check happy.
Which is fine if you are planning to add types to all/most functions in the near future. Is that the case? I could not find in the docs related planning.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Oct 4, 2023

You can use the mypy configuration to batch ignore, or selectively ignore specific errors (or all errors):
https://mypy.readthedocs.io/en/stable/config_file.html#suppressing-errors

example overriding specific modules here: https://mypy.readthedocs.io/en/stable/config_file.html#example-pyproject-toml

@johnthagen
Copy link
Contributor

I was surprised that py.typed was added without many common functions being type annotated (img_as_float, img_as_ubyte, resize, imsave, etc.) This leads to a large number of type check errors when running mypy on a code base that uses scikit-image.

I would recommend, if possible, that you only add a py.typed file if your whole (or at least vast majority) of the public API is type annotated. Otherwise, every downstream user of your library that type checks their code will need to do suppression work.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Oct 5, 2023

for a little added context:
the motivation here goes back to #5101 which introduced lazy submodule imports for many modules in sci-kit image. Because modules were no longer listed as literal imports, type checkers and IDEs were no longer able to infer imports and namespaces for intellisense/autocomplete.

So, in order to tell IDEs what scikit-image has available, two options were presented (#6435 and #6429), and a variant of #6435 was merged in #6577, using pyi stubs to indicate to IDEs what modules & functions scikit-image had to offer. However, in order for static type checkers to use these static hints, you must include a py.typed file (as per PEP 561).

My understanding of the implications of py.typed from PEP 561 is not to indicate some critical mass or coverage of typed functions in a package, but simply to indicate to type checkers that they should search this package for stubs, as opposed to a third party stubs package (or typeshed) which was the historical/legacy way that many packages incrementally offered types. Users who wish to use mypy with strict setting (I'm definitely a fan) often do need to add a bit of configuration for partially typed dependencies. (and I feel like having to add import skimage # type: ignore is not dramatically different from adding an ignore_errors=True to your module overrides?)

I don't particularly have a preference here, i leave it to the maintainers of scikit-image to decide... but if you would vote for the removal of py.typed, then I think you also need to suggest how you would deal with the inability of IDEs to detect anything in scikit-image given the use of lazy imports.

@johnthagen
Copy link
Contributor

The problem is that the way the suppressions work, you can't add a blanket ignore for problems in a third party library (at least I could not get this to work) because the calls to skimage happen in user code. If you have these calls scattered all over your code base, you end up having to suppress disallow_untyped_calls everywhere, which opens you up for the type checker missing real errors in other usages of your code.

For example, I tried:

[[tool.mypy.overrides]]
module = "skimage"
disallow_untyped_calls = false

But this does not suppress skimage issues:

my/package/module.py:1: error: Call to untyped function "img_as_ubyte" in typed context  [no-untyped-call]

I would be happy to be proven wrong if there is a Mypy configuration that can suppress only skimage-caused no-untyped-call errors globally.

@tlambert03
Copy link
Contributor Author

tlambert03 commented Oct 5, 2023

my apologies, this feature was merged in mypy a couple days after the most release release in August: python/mypy#15845

so, it will be available soon. but until then, you're correct that's it doesn't appear possible to enforce strict typing for your own library, while using scikit-image without local ignores. I don't have too much more to add, but will just re-iterate that this is a genuinely tricky problem.

  • If you remove py.typed, then at least some if not all lazy modules will not give you any IDE autocompletion or namespace hints at all. (I'm not sure this avenue has been exhausted, but a vote to remove py.typed should be accompanied with a proposal to solve this problem too)
  • If you include py.typed, then dependent libraries who want to disallow untyped calls within their own libraries will have to locally ignore calls to skimage until Add option to selectively disable --disallow-untyped-calls python/mypy#15845 makes it into a release
  • outside of the challenges of lazy imports, if scikit-image ever wants to offer type hints for a broader portion of the library, it will inevitably happen incrementally, so this challenge will certainly come up again in the future. (again, hopefully solved by the untyped_calls_exclude in mypy)

cheers!

@soupault
Copy link
Member

soupault commented Oct 5, 2023

if scikit-image ever wants to offer type hints for a broader portion of the library

I personally find type-hinting very useful, both in prototyping and proper development scenarios. So, in my mind, we should definitely go that path.

@johnthagen
Copy link
Contributor

so, it will be available soon

Thanks for the reference! That certainly will limit the impact of this change and allow users to suppress this particular issue. I agree that after the next Mypy release, the negative impacts of this PR will not be nearly as severe.

@stefanv
Copy link
Member

stefanv commented Oct 5, 2023

It sounds like the problem will be resolved by (a) a new feature now available in mypy and (b) our stated attempt to type annotate and implement protocols as part of skimage v2.

Perhaps @lagru and @jni have more to say about that plan.

@lagru
Copy link
Member

lagru commented Oct 6, 2023

@johnthagen, sorry for the py.typed related annoyance from our side.

We currently have a grant that also entails typing our API. It's a big effort and we are also trying to revise our API in the process but you can follow along in

From skimming the discussion above, our best bet seems to be to continue work on that and leave the py.typed be.

@johnthagen
Copy link
Contributor

As of Mypy 1.6.1, you can now suppress only the untyped call errors in skimage with:

[tool.mypy]
strict = true
untyped_calls_exclude = [
    "skimage"
]

grlee77 added a commit to grlee77/cucim that referenced this pull request Jan 6, 2024
needed in order for the .pyi files to be useful:
see: scikit-image/scikit-image#7073
grlee77 added a commit to grlee77/cucim that referenced this pull request Jan 6, 2024
needed in order for the .pyi files to be useful:
see: scikit-image/scikit-image#7073
grlee77 added a commit to grlee77/cucim that referenced this pull request Jan 22, 2024
needed in order for the .pyi files to be useful:
see: scikit-image/scikit-image#7073
rapids-bot bot pushed a commit to rapidsai/cucim that referenced this pull request Jan 29, 2024
This MR does not add any new features. It just has minor fixes ported from scikit-image 0.22. Most changed lines are from moving additional modules to use `__init__.pyi` files as for upstream scikit-image.

Regarding introduction of `py.typed`, see [discussion on the scikit-image repo](scikit-image/scikit-image#7073). The TLDR version is that it enables VS CODE (and potentially other IDEs) to use the `.pyi` files for intellisense capabilities. Those using mypy with strict type checking, may have to add an exclusion like the following to disable warnings about most of the cuCIM API which does not currently use type hints:

```toml
[tool.mypy]
strict = true
untyped_calls_exclude = [
    "cucim"
]
```

Authors:
  - Gregory Lee (https://github.com/grlee77)
  - https://github.com/jakirkham

Approvers:
  - Gigon Bae (https://github.com/gigony)
  - https://github.com/jakirkham
  - Ray Douglass (https://github.com/raydouglass)

URL: #670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants