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 for typing? #2625

Closed
neiljp opened this issue Jul 15, 2017 · 32 comments
Closed

Support for typing? #2625

neiljp opened this issue Jul 15, 2017 · 32 comments

Comments

@neiljp
Copy link
Contributor

neiljp commented Jul 15, 2017

I've been looking at applying typing to some code which uses PIL/pillow.

Is there any feeling regarding the addition of type annotations within Pillow, or if these belong outside the code in eg. typeshed? This influences whether I might work on a PR against Pillow itself, or generate stubs for typeshed.

On the basis of supporting python 2 and 3, I expect annotations would be in comments, and there would be an import of the typing module near the top, to define required types.

@szabolcsdombi
Copy link

szabolcsdombi commented Jul 16, 2017

Type hints would be great. I am using vscode and linting helps a lot.

I am willing to contribute.

@emmeowzing
Copy link

I am as well. I think this would be a great addition to the project.

@wiredfool
Copy link
Member

It sounds like a good idea, at least at the top 'interface' level to help ensure a well defined interface.

Typeshed would be a simple way to get it up and running without much interference with the current repo, but in the long run I think that the appropriate place for the type annotations would be in this repo, so that they can be kept in sync and (potentially at least) help keep bugs out of the code base in the testing phase.

@emmeowzing
Copy link

What problems would the project face with Python 2/3 compatibility?

@neiljp
Copy link
Contributor Author

neiljp commented Aug 15, 2017

I've been working with mypy in Zulip, and at least until recently they use 2 and 3, and mypy seems to work for that.

@neiljp
Copy link
Contributor Author

neiljp commented Aug 15, 2017

@wiredfool I'll explore expanding typeshed then, if that's a go-ahead - typeshed is clear on seeking approval from project maintainers first.

That said, if direct annotation PRs would be accepted, that'd be something I'd look at working on instead.

@wiredfool
Copy link
Member

I'm not up on the technical bits for doing type annotations. But, as I understand, there are three ways to do it:

  1. In the function signatures. Requires 3.5ish+. Not an option here due to our continuing support for older pythons.
  2. In function docstrings.
  3. In Typeshed.

The advantage of docstrings is that they're next to the code, and once it's all in, it can be maintained and not drift out of sync with the code. The disadvantage is that they're going to require PRs into this repo.

Can you do a function or two as an example against Image.py where we've got autodoc extracting the documentation? And then possibly see if we can test against that in the test suite?

@emmeowzing
Copy link

I wouldn't opt for writing type annotations in docstrings since it's so easy to edit code and forget to update them if the types should change, even marginally. I do, however, like the methods mentioned here. Adding single (or multiple) line comments describing a function or method's type just before a docstring is checked by Mypy (and highlighted by Pycharm).

@neiljp
Copy link
Contributor Author

neiljp commented Aug 16, 2017

Annotating in docstrings is maybe coming to mypy through a plugin, IIRC. However, yes, I was just going to take a look at this using comments.

@wiredfool
Copy link
Member

From my POV docstrings and a comment prior to the docstring are roughly equivalent, They're both not 'in code' to the point of breaking on 2.7, but they're in the same source file.

We'd need to make the typing module a conditional import, as it doesn't need to be required.

@neiljp
Copy link
Contributor Author

neiljp commented Aug 16, 2017

I'm working on this now.

@neiljp
Copy link
Contributor Author

neiljp commented Aug 17, 2017

Provisional work is now in my fork here https://github.com/neiljp/Pillow/tree/test_annotate

Should I make a WIP PR? That's a lot of functions with annotations, but not all quite fully there, and mypy doesn't 'pass' the code fully (and that's just with that file). There are some code changes which I could make to help the code pass and improvements/hints from mypy, but I wasn't sure whether to just make annotations, or perhaps also add related potential changes to the code but in separate commits in the same PR?

Anyhow, interested to hear what the feeling is so far.

@wiredfool
Copy link
Member

Go ahead and make a PR for that. It'll give us a good place to discuss it, I can ask questions and hopefully arrive at something that works/passes.

@mat100payette
Copy link

Any reason why this is open and abandoned ?

@emmeowzing
Copy link

@mat100payette this effort was led by @neiljp and @wiredfool in #2687, but work hasn't progressed it looks in ~2yrs, now.

@mat100payette
Copy link

@bjd2385 I see. It seems like this past argument isn't valid anymore:

In the function signatures. Requires 3.5ish+. Not an option here due to our continuing support for older pythons.

since pillow 8+ supports python 3.6 to 3.9.

Overall, pillow has really jank type inference without proper hints...

@wiredfool
Copy link
Member

#2941 (comment)

Basically, what happened here is that we/I pushed it forward, and we basically ran into a place where:

  1. we needed tests on the types to prevent regressions.
  2. It was a lot of work, and from the point of view of code quality for Pillow, it's a wash. The types that we have aren't expressive enough to have the concept of a loaded image without significant rework of the public interface, and we're not into that for backwards compatibility issues. There's too many ignores and other optional stuff.
  3. The type infra didn't really seem fully baked at that point.
  4. I ran out of time in early 2018.

@gsingh93
Copy link

gsingh93 commented Mar 3, 2023

I see that there are some type annotations on typeshed now that can be installed with the types-pillow package: https://github.com/python/typeshed/tree/main/stubs/Pillow

Anyone know the status of these? It seems to be working for me so far.

@Freyert
Copy link

Freyert commented May 29, 2023

I know very little about the type system and wanted to learn some more. Here's all the extra context I've collected:

  1. This most recent work was to implement nominal (by name) subtyping as described in PEP 484.
  2. The PIL Project looks to maintain backwards compatibility so a "type comments" approach was taken as recommended in PEP 484: Suggested syntax for Python 2.7 and straddling code.
  3. Ultimately, work was stopped because PEP 484 typing doesn't work well with the current PIL flexible interfaces (Source). Or the linters at the time had inconsistent behavior.

Currently there exist typeshed "stubs" which can be installed "in tandem". However, these are likely not complete. Side note: typeshed is a part of PEP484 and therefore is a common way of getting types for libraries without typing (PEP484: The Typeshed Repo).

The last work at adding types made heavy use of stub files (_imaging.pyi).


It's really worth reading through the 2 PRs related to adding types. There's a lot to learn there.


I was reading through PEPs and came across PEP 544 which supports "structural typing". This allows type semantics similar to Go's interfaces. In PEP 544 they choose the term "protocol" instead of interface.

👉 I'm wondering if any of the contributors for the earlier work had thoughts on using structural types? It seems like it may fit better than nominal types.

For example, structural types could work well for the PIL.ImageFilter module as most conform to an "implicit" structural type (they all have a "filter" method).

Perhaps having a typing system that works well with "duck typing" would make things easier?

I'm not sure what the benefits would be, and maybe 544 implies that code is typed using PEP 484.

@Avasam
Copy link

Avasam commented Oct 16, 2023

I feel like #2941 is trying to do too much at once (and there's a couple issues with it, along with merge conflicts).

I would suggest a more incremental, step-by-step plan to achieve this goal:

  1. Fix all immediately fixable typing errors seen by mypy with non-strict type checking, targeting only public API.
    • Configure/disable errors that cannot be dealt with at this time, like missing imports, untyped references, etc.
    • Enforce mypy in the CI to prevent errors from coming back.
  2. Do the same with pyright
  3. Merge stubs from typeshed
  4. Enable rules in mypy & pyright to ensure that no type annotations are missing (function definitions, return types, class members, etc)
  5. Publish setuptools as py.typed (typeshed will mark the stubs as "obsolete" for 6 months before removal)
  6. Start type-checking private/internal (non-vendored) modules as well
  7. Turn on strict typing in mypy & pyright. Disable every rule that fails.
  8. Progressively re-enable relevant rules and fix them in code.

Starting from step 3 type-checking users get useful hints on par with typeshed (as long as they allow their type-checkers to use untyped modules)
Step 5 closes this issue. Everything else is bonus for Pillow itself.

@radarhere
Copy link
Member

Pillow now runs mypy as part of our CI (#7622) without excluding any files (#7808).

@hugovk
Copy link
Member

hugovk commented Mar 31, 2024

After initial attempts in 2017 and 2018, and 51 PRs over the past ~3 months, the next Pillow 10.3.0 will be released with type hints, the py.typed file and Typing :: Typed Trove classifier (#7822).

Thanks everyone for helping out!

@hugovk hugovk closed this as completed Mar 31, 2024
Pillow automation moved this from Icebox to Closed Mar 31, 2024
@nulano
Copy link
Contributor

nulano commented Mar 31, 2024

To be clear, the type hints are not yet complete (e.g. PIL.Image has only a few type hints), but a large part of Pillow now has complete type hints.

@hugovk
Copy link
Member

hugovk commented Mar 31, 2024

Thanks, yes, we'll still need to add some, and likely adjust some newly-added ones, but we've added the PEP 561 py.typed metadata so I think this tracking issue can be closed. But happy to re-open if anyone would like.

@WhyNotHugo
Copy link

Thanks for everyone who made this possible!

Even if types are partially incomplete, it's a great base on which smaller contributions can be added as needed :)

@Avasam
Copy link

Avasam commented Mar 31, 2024

Thank you so much everyone who contributed to this.

To be clear, the type hints are not yet complete (e.g. PIL.Image has only a few type hints), but a large part of Pillow now has complete type hints.

Thanks, yes, we'll still need to add some, and likely adjust some newly-added ones, but we've added the PEP 561 py.typed metadata so I think this tracking issue can be closed. But happy to re-open if anyone would like.

Do you know if the current Pillow annotations are at least on par with typeshed's ? As soon as Pillow is released with a py.typed marker, typeshed's stubs will be marked as obsolete. We'd then stop development on types-Pillow and remove it from typeshed's repo 6 months later.
It'd be unfortunate for users to loose on existing annotations, if other maintainers are left to believe that types-Pillow doesn't need to be updated anymore. But if needed, the typeshed stubs can also be kept and made partial, to fill in the gaps in the mean time. Hence I'm asking.

@hugovk
Copy link
Member

hugovk commented Mar 31, 2024

I checked typeshed a bit near the start, but I think on the whole we've added the hints from scratch. From a quick spot comparison, the coverage looks pretty good here.

Six months aligns nicely with two Pillow quarterly releases, which is a good time to fill in any important gaps. If you find some, we're happy for issues and PRs here. And I'd be fine with keeping the typeshed stubs around a bit longer if needed.

@aclark4life
Copy link
Member

@hugovk Does closing this mean we now support type hints in enough of the code to declare that we support type hints? Just curious if we'll ever reach 100%, as I'm reviewing all the type hints PRs continuing to getting merged. Thanks for any info!

@radarhere
Copy link
Member

We declared that we support type hints in #7822. mypy gives us a pass, at least.

However, #8029 has been opened, questioning whether we should have done that before reaching 100%.

@aclark4life
Copy link
Member

Ah! Thanks @radarhere . In that case, I'd maybe consider reopening this one and making the new standard for closing it "We're done adding type hints for now". Either way thanks again for the explanation. 👍

@hugovk
Copy link
Member

hugovk commented May 31, 2024

Yeah, we maybe added py.typed and the classifier a bit early (#2625 (comment)), but I think we can use #8029 to track current progress. The definition of 100% also depends on the mypy settings used -- what are the "correct" settings? We could consider removing py.typed for the next quarterly release, but I welcome PRs from Pillow users who want more hints for certain files :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Closed
Pillow
  
Closed
Development

Successfully merging a pull request may close this issue.