Replace is_temporal checks with Cython version #55506
Conversation
| @@ -51,7 +52,7 @@ cdef inline int64_t get_nat() noexcept: | |||
| # -------------------------------------------------------------------- | |||
| # Type Checking | |||
|
|
|||
| cdef inline bint is_integer_object(object obj) noexcept nogil: | |||
There was a problem hiding this comment.
We previously declared is_timedelta64_object / is_datetime64_object to be nogil, which cascaded through the rest of the functions in this pxd file. However, the upstream function is not considered nogil, with some discussion coming back to here:
I think its a mistake for any of the functions in this module that touch CPython objects to be declared nogil, unless something more recent in CPython has declared these functions as such (haven't looked)
There was a problem hiding this comment.
can we just rip this out since we're using the cnp version everywhere?
There was a problem hiding this comment.
oh never mind, i see you did that where relevant below
There was a problem hiding this comment.
You are talking about the nogil right? Yea I did the minimum required for this PR. There are still other functions in util.pxd where it should probably be removed
|
Does this affect our minimum cython/numpy versions? (IIRC cython has a |
mroeschke
added
the
Internals
|
Would this allow us to unpin our Cython |
|
As long as you're doing these, might as well change to use the other stuff upstreamed into numpy in numpy/numpy#16266? |
Ah didn't realize we pinned that. I believe so; at least locally I can build the entire code base with this |
Unfortunately there is a problem where the definition we have of I think that's a bug in Cython - the source file |
This was a bug in Cython, but I'm happy to see that this fixes it. |
|
OK I updated the enum definition upstream in numpy/numpy#24923 (comment) so can come back around for the rest of these when that goes through. Any concern with this PR in its current state? |
|
I'm OK with the fix. Could you also unpin Cython 3.0.3 in our files? |
|
I think we can unpin generally anyways since Cython 3.0.4 is out. |
|
Thanks @WillAyd |
|
nice! |
With Cython 3.0.3 I was unable to build pandas. Looks like is_timedelta / is_datetime functions were getting mixed between static / non-static declarations. I wasn't sure why that was happening, but while investigating noted that these got ported upstream by @jbrockmendel a few years back, so I think it makes sense to leverage that instead (also clears the issue)