python-pillow / Pillow

Python Imaging Library (Fork)
https://python-pillow.org
Other
12.35k stars 2.24k forks source link

Added type hints to Image.__init__() #8279

Closed radarhere closed 3 months ago

radarhere commented 3 months ago

Alternative to #8275

Once the None return type is added to Image.__init__(), a variety of other changes are needed. https://github.com/python-pillow/Pillow/blob/d8447de24d47847152ed1d7d14f2b896fca12307/src/PIL/Image.py#L535-L544

mergify[bot] commented 3 months ago

⚠️ The sha of the head commit of this PR conflicts with #7461. Mergify cannot evaluate rules on this PR. ⚠️

akx commented 3 months ago

I'm a tiny bit worried about the cost of these assertions within the core library. I think practically no one* runs Python with -O/PYTHONOPTIMIZE that'd wipe these out.

*: entirely anecdotal

hugovk commented 3 months ago

Some discussion on PYTHONOPTIMIZE, showing it does get used:

https://discuss.python.org/t/does-anyone-use-o-or-oo-or-pythonoptimize-1-in-their-deployments/37671

Please could you run some benchmarks to evaluate the cost?

radarhere commented 3 months ago

There's a variety of changes here, but running

python3.9 -m timeit -r 100 -c 'from PIL import Image;Image.new("L", (1, 1)).copy()'

without this change, I get

100000 loops, best of 100: 2.36 usec per loop

With this change, I get

100000 loops, best of 100: 2.37 usec per loop

hugovk commented 3 months ago

I think that's close enough not to worry about. There's some variation as well. Running twice:

without this change, I get:

100000 loops, best of 100: 2.45 usec per loop 100000 loops, best of 100: 2.37 usec per loop

with this change, I get:

100000 loops, best of 100: 2.39 usec per loop 100000 loops, best of 100: 2.41 usec per loop

And similar results across 3.12.

Without:

200000 loops, best of 100: 1.49 usec per loop

With:

200000 loops, best of 100: 1.48 usec per loop

@akx Any further concerns?

akx commented 3 months ago

Now that I think of it, can self.im actually ever practically be None?

Would all of this be simpler if we went for practicality and not purity, and said

self.im: core.ImagingCore = None  # type: ignore[assignment]

or alternatively left self.im unassigned in __init__ and just did

im: core.ImagingCore

as an annotation?

I think https://github.com/python-pillow/Pillow/pull/7461 is related here.

radarhere commented 3 months ago

There are other places where it is set to None https://github.com/python-pillow/Pillow/blob/11b4df3ff9c02757eaf687ac13374f3525c8cb71/src/PIL/PngImagePlugin.py#L871 https://github.com/python-pillow/Pillow/blob/11b4df3ff9c02757eaf687ac13374f3525c8cb71/src/PIL/GifImagePlugin.py#L158 https://github.com/python-pillow/Pillow/blob/11b4df3ff9c02757eaf687ac13374f3525c8cb71/src/PIL/GifImagePlugin.py#L437

The current code does also check that it isn't None https://github.com/python-pillow/Pillow/blob/11b4df3ff9c02757eaf687ac13374f3525c8cb71/src/PIL/Image.py#L890 https://github.com/python-pillow/Pillow/blob/11b4df3ff9c02757eaf687ac13374f3525c8cb71/src/PIL/Image.py#L907

akx commented 3 months ago

If it's only about 5 places, would it make sense to consider whether those places need to do that?

They could delattr(self, "im") and hasattr(self, "im") instead, maybe?

radarhere commented 3 months ago

Now that I think of it, can self.im actually ever practically be None?

I think https://github.com/python-pillow/Pillow/pull/7461 is related here.

This isn't just about the edge case of calling Image.Image() directly. It is None before an image is loaded.

>>> from PIL import Image
>>> im = Image.open("Tests/images/hopper.png")
>>> im.im is None
True
>>> im.load()
<PixelAccess object at 0x105d86510>
>>> im.im is None
False

They could delattr(self, "im") and hasattr(self, "im") instead, maybe?

If code simplicity is the concern rather than speed, how about this idea as a slightly smaller change? I've pushed a commit so that using @property, im.im always returns core.ImagingCore. im._im can be used to check for None.