oracle / graalpython

GraalPy – A high-performance embeddable Python 3 runtime for Java
https://www.graalvm.org/python/
Other
1.24k stars 108 forks source link

numpy: return type of PyArray_FillFunc #168

Closed rahulmutt closed 4 years ago

rahulmutt commented 4 years ago

I'm currently attempting to compile tensorflow from source using the sulong llvm toolchain and a graalpython virtual environment and I've hit this error:

tensorflow/python/lib/core/bfloat16.cc:591:31: error: assigning to 'PyArray_FillFunc *' (aka 'void (*)(void *, long, void *)') from incompatible type 'int (void *, npy_intp, void *)' (aka 'int (void *, long, void *)'): different return type ('void' vs 'int')
  NPyBfloat16_ArrFuncs.fill = NPyBfloat16_Fill;
                              ^~~~~~~~~~~~~~~~

Looking further this appears to be due to the fact that installing numpy with python -m ginstall install numpy applies this patch:

https://github.com/graalvm/graalpython/blob/master/graalpython/lib-graalpython/patches/numpy/sdist/numpy.patch#L138-L139

-typedef int (PyArray_FillFunc)(void *, npy_intp, void *);
+typedef void (PyArray_FillFunc)(void *, npy_intp, void *);

What was the reason that this patch was applied? Will it break any graalpython assumptions if I changed the return type from void to int?

msimacek commented 4 years ago

@fangerer do you happen to know?

rahulmutt commented 4 years ago

Just some update on this: I did remove that particular hunk from the patch file locally and I was able to build the numpy C extensions without issue with ginstall.

fangerer commented 4 years ago

This patch was required because there were functions that did actually not return anything. This is not a compile-time problem but will be a problem at run time because Sulong strictly expects an int as return value. We did this change because it was the minimal one but we could also do it by not changing the signature.

fangerer commented 4 years ago

Since we contributed a fix for NumPy some time ago, all we needed to do is to remove this patch (fixed on current master; commit 3f41de3). Thanks for pointing that out.