Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #16637
Drop Python 3.9 support #16637
Changes from 3 commits
8fb0823
6fd04eb
8ef260e
ec74d27
987ce3f
fad288f
cf4ab5b
d7e9b7d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeAlias
moved totyping
in Python 3.10: https://docs.python.org/3/library/typing.html#typing.TypeAliasThere are still other imports from
typing_extensions
incudf-polars
though, so we can't drop the dependency on it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything else in
cudf
usestyping.Callable
.git grep -E 'import.*Callable'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused by this one, is this an auto-upgrade fix that ruff can do for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is one
ruff
will do automatically if asked.But either way... it recommended this for
cudf_polars
but not for the rest ofcudf
, becausecudf_polars
has its ownruff
config separate from the rest ofcudf
.cudf_polars
uses all thepyupgrade
checkscudf/python/cudf_polars/pyproject.toml
Line 89 in 8d6b261
All of the other Python code in the repo only has specific
pyupgrade
checks run over itcudf/pyproject.toml
Lines 87 to 90 in 8d6b261
Happy to do whatever you want me to with this particular check, I don't have a strong preference for either pattern so I chose the one that would at least be consistent with the rest of the Python code in this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wence- could you please let me know what you want to do with this one, and if there's anything else I could help clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typing.Callable
is the deprecated alias, so in cudf-polars can we migrate to usingcollections.abc.Callable
?I realise in cudf-classic this is more work, so we can defer doing it there. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose deferring this change to a separate PR unless we can knock it out today. Completing the drop of Python 3.9 is important so we can start adding Python 3.12. We want all these platform changes done before we get too close to burndown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sure, happy to do that in a follow-up PR after this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put up #16670 making this change across
cudf
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grlee77 pointed out to me that there's actually a performance penalty associated with using
|
instead of a tuple inisinstance()
.rapidsai/cucim#766 (comment)