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

Drop Python 3.9 support #766

Merged
merged 8 commits into from
Aug 22, 2024
Merged

Conversation

jameslamb
Copy link
Member

Description

Contributes to rapidsai/build-planning#88

Finishes the work of dropping Python 3.9 support.

This project stopped building / testing against Python 3.9 as of rapidsai/shared-workflows#235.
This PR updates configuration and docs to reflect that.

Notes for Reviewers

How I tested this

Checked that there were no remaining uses like this:

git grep -E '3\.9'
git grep '39'
git grep 'py39'

And similar for variations on Python 3.8 (to catch things that were missed the last time this was done).

@jameslamb jameslamb added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Aug 21, 2024
@jameslamb jameslamb changed the title Drop Python 3.9 support WIP: Drop Python 3.9 support Aug 21, 2024
@jameslamb
Copy link
Member Author

Any file changes here that aren't just removing a "3.9" or changing it to a "3.10" were made automatically by ruff.

There are also these additional new ruff errors that weren't auto-fixed:

python/cucim/src/cucim/core/operations/color/jitter.py:38:10: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
python/cucim/src/cucim/core/operations/color/jitter.py:54:17: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/jitter.py:55:15: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/jitter.py:56:17: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/jitter.py:57:10: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/jitter.py:60:5: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/jitter.py:61:5: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/jitter.py:62:5: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/jitter.py:63:5: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/stain_normalizer.py:449:22: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/color/stain_normalizer.py:454:19: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/intensity/zoom.py:227:15: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/intensity/zoom.py:228:15: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/intensity/zoom.py:246:15: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/core/operations/intensity/zoom.py:247:15: UP007 Use `X | Y` for type annotations
python/cucim/src/cucim/skimage/_shared/utils.py:745:8: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
python/cucim/src/cucim/skimage/_vendored/_ndimage_util.py:46:10: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
python/cucim/src/cucim/skimage/_vendored/_texture.py:197:10: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
python/cucim/src/cucim/skimage/filters/thresholding.py:327:12: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
python/cucim/src/cucim/skimage/measure/tests/test_regionprops.py:1232:12: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
python/cucim/src/cucim/skimage/measure/tests/test_regionprops.py:1381:12: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
python/cucim/src/cucim/skimage/measure/tests/test_regionprops.py:13[84](https://github.com/rapidsai/cucim/actions/runs/10498836421/job/29084514063?pr=766#step:6:85):12: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`

(build link)

@grlee77 how would you like to handle these? I can do the work of either applying or ignoring those suggestions, but will defer to how you want the code to look.

@grlee77
Copy link
Contributor

grlee77 commented Aug 22, 2024

Thanks @jameslamb,

I am in favor of accepting the suggestions related to UP007 unions for type hints.

I think we should add

ignore = ["UP038"]

to the ruff config in pyproject.toml. It seems that there is a performance penalty to using | instead of a tuple in isinstance calls (see here and here)

@grlee77
Copy link
Contributor

grlee77 commented Aug 22, 2024

While updating pyproject.toml, it seems we should update the ruff section names to match current recommendations. Here is a patch including the suggested UP038 ignore:

diff --git a/python/cucim/pyproject.toml b/python/cucim/pyproject.toml
index 2df37c5a..c664659b 100644
--- a/python/cucim/pyproject.toml
+++ b/python/cucim/pyproject.toml
@@ -147,9 +147,6 @@ filterwarnings = [
 ]
 
 [tool.ruff]
-# see: https://docs.astral.sh/ruff/rules/
-select = ["E", "F", "W", "I", "UP"]
-fixable = ["ALL"]
 exclude = [
     # TODO: Remove this in a follow-up where we fix __all__.
     ".tox",
@@ -166,10 +163,16 @@ exclude = [
 line-length = 80
 fix = true
 
-[tool.ruff.per-file-ignores]
+[tool.ruff.lint]
+# see: https://docs.astral.sh/ruff/rules/
+select = ["E", "F", "W", "I", "UP"]
+ignore = ["UP038"]
+fixable = ["ALL"]
+
+[tool.ruff.lint.per-file-ignores]
 # "src/cucim/skimage/util/tests/test_shape.py" = ["E201", "E202"]
 
-[tool.ruff.isort]
+[tool.ruff.lint.isort]
 combine-as-imports = true
 force-single-line = false
 known-first-party = ["cucim"]

@jameslamb
Copy link
Member Author

Thanks @grlee77 ! I've applied changes similar to that patch you provided. Using the new format of ruff configuration required updating ruff, so I updated it to its latest available version (0.6.1).

There's one class of new error that I'd like your opinion on. They look like this:

python/cucim/src/cucim/skimage/morphology/tests/test_misc.py:52:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
   |
50 |     expected_out = cp.empty_like(test_image, dtype=out_dtype)
51 | 
52 |     if out_dtype != bool:
   |        ^^^^^^^^^^^^^^^^^ E721
53 |         # object with only 1 label will warn on non-bool output dtype
54 |         exp_warn = ["Only one label was provided"]
   |

python/cucim/src/cucim/skimage/segmentation/_chan_vese.py:417:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
415 |     float_dtype = _supported_float_type(image.dtype)
416 |     phi = _cv_init_level_set(init_level_set, image.shape, dtype=float_dtype)
417 |     if type(phi) != cp.ndarray or phi.shape != image.shape:
    |        ^^^^^^^^^^^^^^^^^^^^^^^ E721
418 |         raise ValueError(
419 |             "The dimensions of initial level set do not "

Are you comfortable with is / is not for those types of comparisons? Or are there cases where == is really desirable (e.g. because of some compatibility code in objects' __eq__() methods that we want to run)?

full list of those cases (click me)
python/cucim/src/cucim/skimage/_shared/tests/test_utils.py:94:16: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
   |
92 |         assert (
93 |             _validate_interpolation_order(dtype, None) == 0
94 |             if dtype == bool
   |                ^^^^^^^^^^^^^ E721
95 |             else 1
96 |         )
   |

python/cucim/src/cucim/skimage/_shared/tests/test_utils.py:101:10: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
 99 |         with pytest.raises(ValueError):
100 |             _validate_interpolation_order(dtype, order)
101 |     elif dtype == bool and order != 0:
    |          ^^^^^^^^^^^^^ E721
102 |         # Deprecated order for bool array
103 |         with pytest.raises(ValueError):
    |

python/cucim/src/cucim/skimage/_shared/utils.py:812:21: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
811 |     if order is None:
812 |         return 0 if image_dtype == bool else 1
    |                     ^^^^^^^^^^^^^^^^^^^ E721
813 | 
814 |     if order < 0 or order > 5:
    |

python/cucim/src/cucim/skimage/_shared/utils.py:819:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
817 |         )
818 | 
819 |     if image_dtype == bool and order != 0:
    |        ^^^^^^^^^^^^^^^^^^^ E721
820 |         raise ValueError(
821 |             "Input image dtype is bool. Interpolation is not defined "
    |

python/cucim/src/cucim/skimage/color/colorlabel.py:252:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
251 |     new_type = np.min_scalar_type(int(label.max()))
252 |     if new_type == bool:
    |        ^^^^^^^^^^^^^^^^ E721
253 |         new_type = np.uint8
254 |     label = label.astype(new_type)
    |

python/cucim/src/cucim/skimage/exposure/exposure.py:502:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
500 |         # pair of values: always return float.
501 |         return utils._supported_float_type(image_dtype)
502 |     if type(dtype_or_range) == type:
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E721
503 |         # already a type: return it
504 |         return dtype_or_range
    |

python/cucim/src/cucim/skimage/measure/_regionprops.py:141:63: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
139 | }
140 | 
141 | OBJECT_COLUMNS = [col for col, dtype in COL_DTYPES.items() if dtype == object]
    |                                                               ^^^^^^^^^^^^^^^ E721
142 | 
143 | PROP_VALS = set(PROPS.values())
    |

python/cucim/src/cucim/skimage/measure/tests/test_regionprops.py:1226:20: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
     |
1225 |         if col in OBJECT_COLUMNS:
1226 |             assert COL_DTYPES[col] == object
     |                    ^^^^^^^^^^^^^^^^^^^^^^^^^ E721
1227 |             continue
     |

python/cucim/src/cucim/skimage/measure/tests/test_regionprops.py:1244:17: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
     |
1242 |         if cp.issubdtype(t, cp.floating):
1243 |             assert (
1244 |                 COL_DTYPES[col] == float
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^ E721
1245 |             ), f"{col} dtype {t} {msg} {COL_DTYPES[col]}"
1246 |         elif cp.issubdtype(t, cp.integer):
     |

python/cucim/src/cucim/skimage/measure/tests/test_regionprops.py:1248:17: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
     |
1246 |         elif cp.issubdtype(t, cp.integer):
1247 |             assert (
1248 |                 COL_DTYPES[col] == int
     |                 ^^^^^^^^^^^^^^^^^^^^^^ E721
1249 |             ), f"{col} dtype {t} {msg} {COL_DTYPES[col]}"
1250 |         else:
     |

python/cucim/src/cucim/skimage/morphology/tests/test_misc.py:52:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
   |
50 |     expected_out = cp.empty_like(test_image, dtype=out_dtype)
51 | 
52 |     if out_dtype != bool:
   |        ^^^^^^^^^^^^^^^^^ E721
53 |         # object with only 1 label will warn on non-bool output dtype
54 |         exp_warn = ["Only one label was provided"]
   |

python/cucim/src/cucim/skimage/segmentation/_chan_vese.py:417:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
415 |     float_dtype = _supported_float_type(image.dtype)
416 |     phi = _cv_init_level_set(init_level_set, image.shape, dtype=float_dtype)
417 |     if type(phi) != cp.ndarray or phi.shape != image.shape:
    |        ^^^^^^^^^^^^^^^^^^^^^^^ E721
418 |         raise ValueError(
419 |             "The dimensions of initial level set do not "
    |

python/cucim/src/cucim/skimage/transform/_geometric.py:916:16: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
914 |             # combination of the same types result in a transformation of this
915 |             # type again, otherwise use general projective transformation
916 |             if type(self) == type(other):
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^ E721
917 |                 tform = self.__class__
918 |             else:
    |

python/cucim/src/cucim/skimage/transform/_warps.py:173:17: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
171 |     if anti_aliasing is None:
172 |         anti_aliasing = (
173 |             not input_type == bool
    |                 ^^^^^^^^^^^^^^^^^^ E721
174 |             and not (cp.issubdtype(input_type, cp.integer) and order == 0)
175 |             and any(x < y for x, y in zip(output_shape, input_shape))
    |

python/cucim/src/cucim/skimage/transform/_warps.py:178:8: E721 Use `is` and `is not` for type comparisons, or `isinstance()` for isinstance checks
    |
176 |         )
177 | 
178 |     if input_type == bool and anti_aliasing:
    |        ^^^^^^^^^^^^^^^^^^ E721
179 |         raise ValueError("anti_aliasing must be False for boolean images")
    |

Found 15 errors.

@jakirkham
Copy link
Member

jakirkham commented Aug 22, 2024

Thanks James! 🙏

Generally those suggestions sound reasonable. Would be interested in Greg's thoughts as well

With the dtype checks, unfortunately this might just be a case where ruff is leading us in the wrong direction. Please see this example

In [1]: import numpy as np

In [2]: np.dtype(bool) == bool
Out[2]: True

In [3]: np.dtype(bool) is bool
Out[3]: False

@grlee77
Copy link
Contributor

grlee77 commented Aug 22, 2024

@jameslamb

Thanks @grlee77 ! I've applied changes similar to that patch you provided. Using the new format of ruff configuration required updating ruff, so I updated it to its latest available version (0.6.1).

There's one class of new error that I'd like your opinion on. They look like this:

Thanks, I think some of those suggestions may be unwanted. Can we just ignore these E721 errors for now in this MR? Then we can open an issue to remind me to take a look at them individually later on.

@jameslamb
Copy link
Member Author

Yep absolutely! I'll ignore add that code to the list ruff ignores, and open an issue for you.

@jameslamb
Copy link
Member Author

Put up #767 for you.

@jameslamb jameslamb marked this pull request as ready for review August 22, 2024 18:54
@jameslamb jameslamb requested review from a team as code owners August 22, 2024 18:54
@jameslamb jameslamb requested a review from AyodeAwe August 22, 2024 18:54
@jameslamb jameslamb changed the title WIP: Drop Python 3.9 support Drop Python 3.9 support Aug 22, 2024
@jameslamb jameslamb requested review from jakirkham and grlee77 August 22, 2024 18:54
@jakirkham
Copy link
Member

Skimmed through the changes. Generally these all look like solid improvements

If Greg agrees and CI passes, we should ship it

Thanks for all your hard work here James! 🙏

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks James, the changes look great!

@jakirkham
Copy link
Member

Thanks all! 🙏

Let's ship this :shipit:

@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit de6bfbb into rapidsai:branch-24.10 Aug 22, 2024
37 checks passed
@jameslamb jameslamb deleted the drop-python-3.9 branch August 22, 2024 20:04
@jakirkham jakirkham added this to the v24.10.00 milestone Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants