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

Added type hints to Image.__init__() #8279

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

radarhere
Copy link
Member

Alternative to #8275

Once the None return type is added to Image.__init__(), a variety of other changes are needed.

Pillow/src/PIL/Image.py

Lines 535 to 544 in d8447de

def __init__(self):
# FIXME: take "new" parameters / other image?
# FIXME: turn mode and size into delegating properties?
self.im = None
self._mode = ""
self._size = (0, 0)
self.palette = None
self.info = {}
self.readonly = 0
self._exif = None

Copy link
Contributor

mergify bot commented Aug 2, 2024

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

@akx
Copy link
Contributor

akx commented Aug 6, 2024

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
Copy link
Member

hugovk commented Aug 6, 2024

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
Copy link
Member Author

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
Copy link
Member

hugovk commented Aug 7, 2024

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
Copy link
Contributor

akx commented Aug 7, 2024

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 #7461 is related here.

@radarhere
Copy link
Member Author

There are other places where it is set to None

self.im = None

self.im = None

self.im = None

The current code does also check that it isn't None

if self.im is not None and self.palette and self.palette.dirty:

if self.im is not None:

@akx
Copy link
Contributor

akx commented Aug 7, 2024

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
Copy link
Member Author

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

I think #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.

Comment on lines +546 to +551
@property
def im(self) -> core.ImagingCore:
if isinstance(self._im, DeferredError):
raise self._im.ex
assert self._im is not None
return self._im
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't self._im still be None here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is, the assert will stop the code before the return statement is reached.