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.

Sign up for GitHub

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

Jump to bottom

Add type hints to openslide package #260

Merged
merged 15 commits into from
Oct 19, 2024
Merged

Add type hints to openslide package #260

merged 15 commits into from
Oct 19, 2024

Conversation

sammaxwellxyz
Copy link
Contributor

Hey folks,

An attempt to address #157

I've typed the main interface and the deepzoom file, with ignoring type hints for the harder-to-type lowlevel file.

I think I could get there for lowlevel as well but would this suffice in the meantime?

Would have a few conflicts with #243, is that still active?

I've strict typed against 3.8 as per the mypy settings in pyproject.toml. Opted for mypy as that's the tool I'm familiar wth

@openslide-bot
Copy link

openslide-bot commented Feb 26, 2024 •
edited
Loading

DCO signed off ✔️

All commits have been signed off. You have certified to the terms of the Developer Certificate of Origin, version 1.1. In particular, you certify that this contribution has not been developed using information obtained under a non-disclosure agreement or other license terms that forbid you from contributing it under the GNU Lesser General Public License, version 2.1.

@bgilbert
Copy link
Member

Thanks for the PR! #243 is currently waiting for me to review it. I have a few other things to finish first, but it's on my list.

mypy for ≥ 3.8 sounds good; that's what we've been using elsewhere. Ideally we'd type lowlevel too, but I'm not sure yet how much of a pain that will be. I know @jaharkes was also working on this.

@sammaxwellxyz
Copy link
Contributor Author

Just fixed the tests, and remembered I also updated some pieces which previously output tuples to lists as their size was dynamic. I imagine this could be a problem and a technically a breaking change for those expecting tuples.

sammaxwellxyz
sammaxwellxyz commented Feb 26, 2024
View reviewed changes
openslide/deepzoom.py Outdated Show resolved Hide resolved
@bgilbert
Copy link
Member

Some initial comments:

  • Please remove the quoting changes. They make the diff hard to read, and (as is hopefully obvious) OpenSlide Python mostly uses single quotes for strings. Likewise, I think the .gitignore changes are unrelated.
  • In other Python-based OpenSlide code (e.g. openslide-bin) we use from __future__ import annotations and enforce its presence via CI. That would avoid the need for moving _OpenSlideMap etc. within the source file. I'm interested in opinions about whether that's the right approach here. I know there have been multiple abandoned attempts to make Python use that import by default, and adding it here would affect the OpenSlide Python API, so it's possible we should avoid it.
  • If we don't add that import, please do the moving of _OpenSlideMap etc. in a separate preparatory commit that doesn't change anything else. That makes the later commit easier to read without move-aware diffing.
  • I'd rather not convert tuple return values to lists, for the reason you mentioned. Is there a reason the tuple[T, ...] syntax won't work here?

Also cc @jaharkes for review.

@bgilbert
Copy link
Member

Also, if we use from __future__ import annotations, we can continue to support Python 3.8 while avoiding the deprecated aliases.

@bgilbert
Copy link
Member

Not only that, but we could also use a | b instead of Union and a | None instead of Optional.

It looks like PEP 649 has opted for a third approach, neither classic annotations nor string-typed annotations, starting in Python 3.13. As I read the backward-compatibility section, annotated objects under PEP 649 will still have slightly different semantics if from __future__ import annotations is active when they're defined. Even so, I'm leaning toward using the import. It would substantially improve the clarity of type annotations within the OpenSlide Python codebase.

@sammaxwellxyz
Copy link
Contributor Author

Please remove the quoting changes. They make the diff hard to read, and (as is hopefully obvious) OpenSlide Python mostly uses single quotes for strings. Likewise, I think the .gitignore changes are unrelated.

sure thing, apologies I assumed that was precommit

In other Python-based OpenSlide code (e.g. openslide-bin) we use from future import annotations and enforce its presence via CI. That would avoid the need for moving _OpenSlideMap etc. within the source file. I'm interested in opinions about whether that's the right approach here. I know there have been multiple abandoned attempts to make Python use that import by default, and adding it here would affect the OpenSlide Python API, so it's possible we should avoid it.

happy to use future

I'd rather not convert tuple return values to lists, for the reason you mentioned. Is there a reason the tuple[T, ...] syntax won't work here?

I think there was but may have just been a different battle with the type checker.

I'll take a proper look at making the changes soon.

Cheers

@sammaxwellxyz sammaxwellxyz force-pushed the typing branch 18 times, most recently from 043d2c8 to 3b621bd Compare March 6, 2024 23:21
sammaxwellxyz
sammaxwellxyz commented Mar 6, 2024
View reviewed changes
openslide/__init__.py Outdated Show resolved Hide resolved
sammaxwellxyz
sammaxwellxyz commented Mar 6, 2024
View reviewed changes
openslide/deepzoom.py Outdated Show resolved Hide resolved
@sammaxwellxyz
Copy link
Contributor Author

hey @bgilbert, suggestions applied and pushed.

  • quotes back to the way they were
  • adopted future annotations, no stringified return type for Self and use of union operator
  • separate commit for the _OpenslideMap, not necessary with future annotation but figured it won't hurt
  • dropped using lists for the n-tuples, but have left in some changes to use 2-tuples

@bgilbert bgilbert force-pushed the typing branch 2 times, most recently from 6f4b6e0 to 01bf0b1 Compare September 8, 2024 00:43
@bgilbert
Copy link
Member

bgilbert commented Sep 8, 2024

I've pushed #277 to require from __future__ import annotations everywhere, rebased this PR to fix CI, and dropped the move of the _OpenSlideMap classes since it's not needed anymore. Will circle back to review this soon.

@bgilbert
Copy link
Member

Rebased again to pick up more CI changes.

@bgilbert bgilbert mentioned this pull request Oct 18, 2024
Merged
sammaxwellxyz and others added 4 commits October 17, 2024 22:46
@sammaxwellxyz @bgilbert
add typing for main interface file
f31c8b2 
Signed-off-by: Sam Maxwell <sam@groundtruthlabs.com>
Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@sammaxwellxyz @bgilbert
type the deepzoom generator
f4b8131 
Signed-off-by: Sam Maxwell <sam@groundtruthlabs.com>
@bgilbert
Minor type fixes
5032e23 
Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Enable type checking for entire openslide package
a6c2f82 
Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Copy link
Member

Rebased onto #281, removed some type: ignore declarations that are obsolete now, and got the PR type-checking cleanly in CI. I'll plan to push some cleanup commits and then squash-merge.

@bgilbert
Return subclass from AbstractSlide.__enter__()
e52cb3a 
AbstractSlide subclasses will always return their specific type from
this method.  Ensure the type system knows this.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Disallow bytes in filename arguments
87495ba 
bytes will not work in at least one case (the OpenSlide initializer), are
not documented to work, and the type annotations are currently
inconsistent about accepting it.  For now, forbid it everywhere for
consistency.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Explicitly hint the possible open_slide() return types
8c26361 
open_slide() can't return any possible AbstractSlide subclass, only the
two that it's documented to return.  Make that explicit in the type hint.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Loosen mapping type guarantees
076e96f 
Avoid overcommitting to internal implementation details in the types being
returned.  It's sufficient to say that the property and associated image
maps implement Mapping.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Drop _OpenSlideCacheWrapper
dccce62 
We don't want to encourage library users to duck-type a replacement for
OpenSlideCache, since any replacement would inherently need to make
assumptions about our implementation details.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Explicitly check for closed ImageSlide
cfd1663 
When an operation is performed on a closed ImageSlide, we've historically
raised an AttributeError upon dereferencing None.  Add proper checks so
the type system understands what we're doing, raising ValueError as
lowlevel does.  This is not an API change because it only affects invalid
caller behavior.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
deepzoom: revert unrolling of _get_tile_info()
f24c1dd 
We don't need to avoid generator expressions to keep the type checker
happy; we can just assert that the resulting tuples have the correct
length.  This lets us avoid carrying redundant unrolled code.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
deepzoom: fix level_dimensions type hint
0d675d6 
level_dimensions is always a tuple of 2-tuples.  Prove this to the type
checker.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Revert unrolling of ImageSlide.read_region()
981684b 
Keep the type checker happy by asserting the correct tuple lengths rather
than unrolling generator expressions.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert bgilbert changed the title Typing Add type hints to openslide package Oct 19, 2024
@bgilbert
Drop redundant return type hint
f1d02fc 
__init__() methods don't need a return type hint unless they take no
arguments.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Narrow some ImageSlide property types
0efc25b 
ImageSlide is hardcoded to a single level, and the level_count property
and get_best_level_for_downsample() return types already reflect this.
Reduce the level_dimensions() and level_downsamples() return types to
1-tuples.  In the latter case we can't reduce to a Literal because literal
floats are disallowed.

Signed-off-by: Benjamin Gilbert <bgilbert@cs.cmu.edu>
@bgilbert
Copy link
Member

Okay, I think this is ready to go. On further reflection, it seems useful to preserve the context from the cleanup commits, so I'll merge normally rather than squashing.

Thank you very much for doing this!

@bgilbert bgilbert merged commit ab8824d into openslide:main Oct 19, 2024
52 checks passed
@bgilbert bgilbert mentioned this pull request Oct 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgilbert bgilbert bgilbert left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sammaxwellxyz @openslide-bot @bgilbert