-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
CI: Test Python 3.12 #53743
CI: Test Python 3.12 #53743
Conversation
For those interested, we have Not bad, given most are parametrized, but definitely much more than last year 😭 . |
I pushed up a fix to use |
Thanks for updating this, and sorry for losing track of this PR. I have the Docker image for Python 3.12 set up on my laptop, so I'll try to get some debugging in tomorrow for the Index failures, unless you beat me to it again :). |
Down to 4 tests
I think I regressed one of the indexing tests in my fixes. |
This should be ready now. cc @jbrockmendel @phofl for indexing changes Not sure who to ping for eval/sql changes. |
@@ -165,7 +168,7 @@ def __contains__(self, key: Any) -> bool: | |||
hash(key) | |||
try: | |||
self.get_loc(key) | |||
except (KeyError, TypeError, ValueError): | |||
except (KeyError, TypeError, ValueError, InvalidIndexError): |
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.
what cases get here?
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.
if is_hashable(key): | ||
# Convert generator to list before going through hashable part | ||
# (We will iterate through the generator there to check for slices) | ||
if is_iterator(key): |
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.
why does changing the order matter? the current order is pretty fine-tuned for perf
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.
We need to check for slices in iterators by iterating through them, since they aren't allowed as hash keys.
(It doesn't make sense to have slices as keys in an Index and it breaks way too many things).
This exhausts the generator.
IIUC, this should only lower perf for generators?
(at least the is_iterator
docstring only mentions that it will only return True for generators not list and co.)
Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
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.
One comment and a small test failure otherwise looks good
os: [ubuntu-22.04, macOS-latest, windows-latest] | ||
# TODO: Disable macOS for now, Github Actions bug where python is not | ||
# symlinked correctly to 3.12 | ||
# xref https://github.com/actions/setup-python/issues/701 |
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.
the linked issue is now closed; can this be reenabled?
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.
Matt enabled this again a while back, I think.
if is_iterator(key): | ||
key = list(key) | ||
|
||
if is_hashable(key) and not isinstance(key, slice): |
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.
very late review comment: would it make sense to make is_hashable_non_slice or something? it wouldn't surprise me if many places that use is_hashable current assume non-slice but didn't get updated by this PR
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.
Yeah sure, I just updated the places that broke in the tests.
Is there a case where we would actually want is_hashable
for a slice to equal True, though?
(I was thinking it might be cleaner to make is_hashable
always return False for slices. This would be a breaking API change, though).
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 think having is_hashable be anything other than a try/except around hash would cause problems. im suggesting a new function to de-duplicate the hashable-but-not-slice checks this introduces
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.
Made an issue #55152.
I don't have the same time that I had in summer to work on pandas, but I'll try to have a look (no promises, though).
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.