Skip to content

Conversation

rgommers
Copy link
Member

Reverts #25812

@rgommers rgommers requested a review from ngoldbaum February 20, 2024 16:57
@charris
Copy link
Member

charris commented Feb 20, 2024

Not sure what is going on with CircleCI, but it isn't isolated to this PR.

@ngoldbaum
Copy link
Member

I spent some time this morning trying to sort out how we can both have PyArray_GETITEM and PyArray_SETITEM exposed in ndarraytypes.h and still hide the actual implementation of PyArray_GetArrFuncs in another header, and I don't see how to do it. If we don't revert the change, this means that any use of ndarraytypes.h without importing the array API will lead to warnings and any use of PyArray_GETITEM and PyArray_SETITEM without the API will lead to compiler errors (I think?).

So all-in-all, unless we can figure out how to expose PyArray_GetArrFuncs without these issues, I agree with reverting and we can come back to this later if we figure out a way to do this while still maintaining API stability.

@ngoldbaum
Copy link
Member

The circleCI build is failing because the docs build depends on pandas and the pandas build is broken because of the change this PR is reverting:

FAILED: pandas/_libs/json.cpython-311-x86_64-linux-gnu.so.p/src_vendored_ujson_python_objToJSON.c.o
      cc -Ipandas/_libs/json.cpython-311-x86_64-linux-gnu.so.p -Ipandas/_libs -I../../pandas/_libs -I../../../../pip-build-env-kuumhs0x/overlay/lib/python3.11/site-packages/numpy/_core/include -I../../pandas/_libs/include -I/home/circleci/.pyenv/versions/3.11.4/include/python3.11 -fvisibility=hidden -fdiagnostics-color=always -DNDEBUG -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=c11 -O3 -DNPY_NO_DEPRECATED_API=0 -DNPY_TARGET_VERSION=NPY_1_21_API_VERSION -fPIC -MD -MQ pandas/_libs/json.cpython-311-x86_64-linux-gnu.so.p/src_vendored_ujson_python_objToJSON.c.o -MF pandas/_libs/json.cpython-311-x86_64-linux-gnu.so.p/src_vendored_ujson_python_objToJSON.c.o.d -o pandas/_libs/json.cpython-311-x86_64-linux-gnu.so.p/src_vendored_ujson_python_objToJSON.c.o -c ../../pandas/_libs/src/vendored/ujson/python/objToJSON.c
      ../../pandas/_libs/src/vendored/ujson/python/objToJSON.c: In function ‘NpyArr_iterBegin’:
      ../../pandas/_libs/src/vendored/ujson/python/objToJSON.c:408:62: error: ‘PyArray_Descr’ {aka ‘struct _PyArray_Descr’} has no member named ‘f’
        408 |   npyarr->getitem = (PyArray_GetItemFunc *)PyArray_DESCR(obj)->f->getitem;
            |                                                              ^~

@rgommers
Copy link
Member Author

Thanks for investigating @ngoldbaum.

Looks like CI is happy modulo the two failures we understand.

@ngoldbaum
Copy link
Member

Merging. @seberg let me know if you want to chat or another set of eyes on getting this change done once you're back.

I'll manually trigger wheel builds after merging this.

@ngoldbaum ngoldbaum merged commit 3dad509 into main Feb 20, 2024
@charris charris deleted the revert-25812-set-getitem-rewire branch February 20, 2024 17:55
@rgommers
Copy link
Member Author

I'll manually trigger wheel builds after merging this.

thanks

@lithomas1
Copy link
Collaborator

Is this happening for numpy 2.0?

I'm trying to figure out if I need to change pandas to adapt to this C API change or not.

@charris
Copy link
Member

charris commented Feb 21, 2024

@ngoldbaum Is it possible to define the function in two places, perhaps with guards?

@seberg
Copy link
Member

seberg commented Feb 21, 2024

I spent some time this morning trying to sort out how we can both have PyArray_GETITEM and PyArray_SETITEM exposed in ndarraytypes.h and still hide the actual implementation of PyArray_GetArrFuncs in another header, and I don't see how to do it.

Hmmm, I wonder if the old API made me miss things (macro rather than inline function use). What was the big issue here!?
Requiring API for getitem/setitem doesn't seem that bad to me. So isn't the issue really just that this should have been in ndarrayobject.h instead which requires import_array?

@ngoldbaum
Copy link
Member

Yes, that’s right, I was trying to figure out how to get this to work without moving the functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants