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

pyright failures in Pillow #11688

Closed
srittau opened this issue Apr 1, 2024 · 10 comments · Fixed by #11689
Closed

pyright failures in Pillow #11688

srittau opened this issue Apr 1, 2024 · 10 comments · Fixed by #11689
Labels
project: infrastructure typeshed build, test, documentation, or distribution related

Comments

@srittau
Copy link
Collaborator

srittau commented Apr 1, 2024

pyright started to fail in unrelated PRs due to "errors" in Pillow: https://github.com/python/typeshed/actions/runs/8508140639/job/23301230769?pr=11687

On first glance, I see nothing wrong with the Pillow stubs and the affected file has last changed in December. But I suspect that #11671 is involved.

@srittau srittau self-assigned this Apr 1, 2024
@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 1, 2024

But I suspect that #11671 is involved.

It's not, it's due to the fact that Pillow just released a minor version that includes a py.typed file.

  • In CI, we install all our non-types dependencies for all packages into a big "venv soup" before running pyright on any stubs
  • Some non-types dependencies for some stubs packages depend on Pillow
  • Pillow just released a minor version that added a py.typed file
  • This is causing pyright to get confused in CI when analysing our stubs for Pillow due to discrepancies between the inline types in Pillow at runtime and our stubs, which are both in the venv at the same time when pyright is run on typeshed

@srittau
Copy link
Collaborator Author

srittau commented Apr 1, 2024

I'm not using pyright, but would a solution be to add a PIL._types stubs-only module that only contains these type aliases and protocols?

@srittau
Copy link
Collaborator Author

srittau commented Apr 1, 2024

That said, the _Mode and _Size type aliases are not really pulling their weight.

@AlexWaygood
Copy link
Member

I think we should just make sure that we install a version of pillow in CI that doesn't have a py.typed file, rather than changing our stubs. Presumably stubsabot will suggest in a few hours that we mark them as obsolete anyway, so stability in the now-legacy stubs is probably the most important thing for our users now :)

@srittau
Copy link
Collaborator Author

srittau commented Apr 1, 2024

I'm not sure whether it's time to mark our stubs obsolete. Looking at the annotations in the source, there are some notable omissions even in the public API.

@AlexWaygood
Copy link
Member

Sure, we can debate that separately (but you're probably right; I haven't looked). I still think the root of the problem is lack of isolation when testing different packages in our CI setup, though, so I think we should ideally fix it by tinkering with the CI setup rather than with the stubs themselves

@srittau srittau removed their assignment Apr 1, 2024
@srittau srittau added the project: infrastructure typeshed build, test, documentation, or distribution related label Apr 1, 2024
@hugovk
Copy link
Member

hugovk commented Apr 3, 2024

Hello from Pillow! I think we're fine if you want to hold off marking as obsolete or removing for a while. Six months aligns nicely with two Pillow releases so hopefully we can fill in missing ones in that timeframe.

It'd be great if you could let us know here, or in a Pillow issue, the important omissions (or better yet, a PR ;)

(A users has already noticed and started: python-pillow/Pillow#7936)

(Also brought up here: python-pillow/Pillow#2625 (comment))

Thanks!

@srittau
Copy link
Collaborator Author

srittau commented Apr 4, 2024

@hugovk The main omissions I spotted immediately are the partly annotated Image.open() and Image.Image.save(). Especially the fp argument is a bit tricky to annotate due to its flexibility:

def open(
fp: str | bytes | Path | SupportsRead[bytes], mode: Literal["r"] = "r", formats: list[str] | tuple[str, ...] | None = None
) -> Image: ...

def save(
self,
fp: str | bytes | Path | _Writeable,
format: str | None = None,
*,
save_all: bool = ...,
bitmap_format: Literal["bmp", "png"] = ..., # for ICO files
optimize: bool = ...,
**params: Any,
) -> None: ...

I could send PRs for some parts. How is Pillow's policy regarding _typeshed types and tight protocols like _Writable?

@hugovk
Copy link
Member

hugovk commented Apr 4, 2024

I could send PRs for some parts.

That would be great!

How is Pillow's policy regarding _typeshed types and tight protocols like _Writable?

Erm, we don't really have a policy, probably better to discuss on the tracker to see what the others think. We don't have any Python dependencies (except for testing) so don't have typing_extensions. We have made https://github.com/python-pillow/Pillow/blob/main/src/PIL/_typing.py for some of those things.

@nulano
Copy link

nulano commented Apr 4, 2024

The main omissions I spotted immediately are the partly annotated Image.open() and Image.Image.save()

I've created python-pillow/Pillow#7944 for annotating Image.open() (edit: and Image.Image.save()).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: infrastructure typeshed build, test, documentation, or distribution related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants