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

BUG: Fastcall fix ufunc methods (NumPy 1.21 support) #7449

Closed
wants to merge 3 commits into from

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Sep 30, 2021

This is a start to support NumPy 1.21 use of FASTCALL methods
in ufuncs. There are probably better ways to achieve this support.

NumPy further uses tp_vectorcall_offset on newer versions. This
PR does not add support for this (meaning that a DUfunc will be
unnecessarily slow on NumPy >=1.21 when kwargs are used).

Closes gh-7423


This is a start to close the NumPy 1.21 issues. I am not extremely happy with this dance of duplicating the wrapper functions, but I did not have a better idea.
(Sorry, this is not really tested, yet aside from the fact that it compiles, so I may abuse CI to see if it actually is correct...)

I am sure there will be fix-ups necessary, please don't hesitate to just push/overwrite the changes here directly.

@seberg seberg changed the title Fastcall fix ufunc methods BUG: Fastcall fix ufunc methods Sep 30, 2021
@seberg seberg changed the title BUG: Fastcall fix ufunc methods BUG: Fastcall fix ufunc methods (NumPy 1.21 support) Sep 30, 2021
@seberg seberg force-pushed the fastcall-fix-ufunc-methods branch 5 times, most recently from 4bcc1e8 to a2f027e Compare September 30, 2021 20:34
numba/np/ufunc/_internal.c Outdated Show resolved Hide resolved
@seberg
Copy link
Contributor Author

seberg commented Sep 30, 2021

Sorry, but the CI setup is confusing me with the fact that NumPy 1.21 cannot be found when added this way, not sure how to fix/approach that.

@stuartarchibald
Copy link
Contributor

Sorry, but the CI setup is confusing me with the fact that NumPy 1.21 cannot be found when added this way, not sure how to fix/approach that.

Thanks for opening the PR and working on this, much appreciated.

Numba CI uses conda packages where possible, this line will be what's creating the envs for the additions to the build matrix made in this PR:

conda create -n $CONDA_ENV -q -y ${EXTRA_CHANNELS} python=$PYTHON numpy=$NUMPY pip gitpython pyyaml

pip is available via:

PIP_INSTALL="pip install -q"

Something like this might work (untested, needs the NumPy pip part filling in):

--- a/buildscripts/incremental/setup_conda_environment.sh
+++ b/buildscripts/incremental/setup_conda_environment.sh
@@ -40,7 +40,7 @@ conda list
 # NOTE: pyyaml is used to ensure that the Azure CI config is valid
 # NOTE: 32 bit linux... do not install NumPy, there's no conda package for >1.15
 # so it has to come from pip later
-if [[ "$CONDA_SUBDIR" == "linux-32" || "$BITS32" == "yes" ]]; then
+if [[ "$CONDA_SUBDIR" == "linux-32" || "$BITS32" == "yes" || "$NUMPY" == "1.21"]]; then
     conda create -n $CONDA_ENV -q -y ${EXTRA_CHANNELS} python=$PYTHON pip gitpython pyyaml
 else
     conda create -n $CONDA_ENV -q -y ${EXTRA_CHANNELS} python=$PYTHON numpy=$NUMPY pip gitpython pyyaml
@@ -80,6 +80,11 @@ if [[ "$CONDA_SUBDIR" == "linux-32" || "$BITS32" == "yes" ]] ; then
     $PIP_INSTALL numpy==$NUMPY scipy
 fi
 
+# if needing NumPy 1.21
+if [[ "$NUMPY" == "1.21" ]] ; then
+    $PIP_INSTALL <NOT SURE WHERE TO GET NUMPY=1.21?> scipy
+fi
+
 # Install latest llvmlite build
 $CONDA_INSTALL -c numba/label/dev llvmlite

This is a start to support NumPy 1.21 use of FASTCALL methods
in ufuncs.  There are probably better ways to achieve this support.

NumPy further uses `tp_vectorcall_offset` on newer versions.  This
PR does not add support for this (meaning that a DUfunc will be
unnecessarily slow on NumPy >=1.21 when kwargs are used).
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

might need a space at the end?

buildscripts/incremental/setup_conda_environment.sh Outdated Show resolved Hide resolved
buildscripts/incremental/setup_conda_environment.sh Outdated Show resolved Hide resolved
@seberg
Copy link
Contributor Author

seberg commented Sep 30, 2021

The fact that 1.21 broke numba is a bit ironic, because my new changes in the ufuncs carefully work around the fact that Numba sometimes mutates type list to avoid just that...
If this (unrelated, and from my side accidental/unexpected) change broke Numba without breaking the world, I should likely just pull out that compatibility layer soon... Next time around we should try to have the compatible numba released before the NumPy release, though!

The current NumPy main branch seems to run into a problem: not finding the loop that should be found by legacy-fallback. That should be easy enough to fix, but I am curious if I could convince you to create a (non-failing) CI job against https://anaconda.org/scipy-wheels-nightly/numpy or using pip install ... --pre?

@stuartarchibald
Copy link
Contributor

The fact that 1.21 broke numba is a bit ironic, because my new changes in the ufuncs carefully work around the fact that Numba sometimes mutates type list to avoid just that... If this (unrelated, and from my side accidental/unexpected) change broke Numba without breaking the world, I should likely just pull out that compatibility layer soon... Next time around we should try to have the compatible numba released before the NumPy release, though!

The current NumPy main branch seems to run into a problem: not finding the loop that should be found by legacy-fallback. That should be easy enough to fix, but I am curious if I could convince you to create a (non-failing) CI job against https://anaconda.org/scipy-wheels-nightly/numpy or using pip install ... --pre?

@seberg Thanks for taking extra care over the ufunc work to try and not break Numba, it's very much appreciated.

RE: future changes/testing against NumPy mainline, I wonder if https://github.com/numba/numba-integration-testing could help here? CC @esc

@stuartarchibald
Copy link
Contributor

This C code changes needed for NumPy 1.21 (df8442f) were committed as cff3dd6 and merged as part of #7483. That same PR updated the rest of Numba to accommodate NumPy 1.21 too. Thanks again for submitting the patches for this @seberg, much appreciated.

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

Successfully merging this pull request may close these issues.

numpy 1.21 ufunc reduce segfault
2 participants