eigenpy icon indicating copy to clipboard operation
eigenpy copied to clipboard

Draft: Follow-up numpy 2.0 (DLLAPI)

Open duburcqa opened this issue 11 months ago • 14 comments

duburcqa avatar Mar 19 '24 07:03 duburcqa

I think I will try to use numpy/_core/include/numpy/npy_2_compat.h instead of dealing with backward compatibility manually. It sounds more reasonable.

duburcqa avatar Mar 19 '24 13:03 duburcqa

I changed my mind. Since npy_2_compat.h must be vendored to get backward compatibility. The current MR with the previous one is basically the same without dedicated header.

duburcqa avatar Mar 20 '24 14:03 duburcqa

For now, something is broken, and it is not clear to me why. I will ask on numpy repo:

 ImportError: /home/runner/.local/lib/python3.10/site-packages/jiminy_py/core/core.cpython-310-x86_64-linux-gnu.so: undefined symbol: EIGENPY_ARRAY_APIPyArray_RUNTIME_VERSION

here is the corresponding issue on Numpy.

duburcqa avatar Mar 20 '24 14:03 duburcqa

After a long discussion with numpy team, it turns out that numpy API should not be exposed the way it is to avoid compatibility issues. Actually every import of numpy headers should be moved to the source file. It is the only way to ensure that EigenPy public API does not leak numpy internal symbols.

duburcqa avatar Mar 20 '24 20:03 duburcqa

Maybe moving the discussion here (also). I am still happy to mitigate the linking issue in NumPy if you/eigenpy devs think it would be a serious help. However, while NumPy may not make things easy, I don't think the setup where eigenpy imports NumPy for a downstream library is great.

In a sense, the obvious solution I (and others) on NumPy see right now is to ask users to do a normal NumPy include with the typical NumPy pattern:

#define Py_ARRAY_UNIQUE_SYMBOL MyModule
#define NPY_NO_IMPORT
#include "numpy/ndarrayobject.h"

in most files, and:

#define Py_ARRAY_UNIQUE_SYMBOL MyModule
#include "numpy/ndarrayobject.h"


module_init_function() {
    import_array()
}

in the main module file before any import eigenpy import. Which should "detach" this transitive use of the NumPy API.

The question is whether that seems easy enough for the eigenpy use-case to avoid the issues (if you compile and run eigenpy together with the downstream library there are no issues). (I would also be interested to discuss other ways we can make this nicer by reorganizing the NumPy headers. That might not help us right now but in the future.)

seberg avatar Mar 27 '24 18:03 seberg

@duburcqa Any feedback on this?

jcarpent avatar Jun 11 '24 09:06 jcarpent

@jcarpent No. Here is the current state of the discussion https://github.com/numpy/numpy/issues/26091. A feedback from Eigenpy team was requested to decide in which direction to go next, since different options are worth to consider. I stopped tracking this issue at that time.

duburcqa avatar Jun 11 '24 10:06 duburcqa

Apparently, changes were merged into Numpy recently https://github.com/numpy/numpy/pull/26103. It is now possible to expose Numpy symbols in release 2.0 as it used to be previously. I will have a look and try to take advantage of this feature to fix the issue with eigenpy if this solution is OK for you. It is the one involving the minimal amount of changes but the maintainers of Numpy were suggesting it may be worth refactoring eigenpy to avoid exposing Numpy symbols completely.

duburcqa avatar Jun 11 '24 10:06 duburcqa

@duburcqa I don't think we backported these to 2.0. We could make a (limited) backport to 2.0 if that is necessary for you, though. (Although, I would generally prefer the pattern that you stop re-exporting it and instead force downstream to include the NumPy headers with the normal pattern before including your headers.)

EDIT: To be clear, not exposing NumPy symbols completely may be a bit harder (although it might make sense to at least consolidate the windows/linux divergence in either direction). Downstream can work around this already (at least I think) by properly including NumPy first. The thing to do here might only be to enforce or at least note that.

seberg avatar Jun 11 '24 10:06 seberg

@seberg Thank you for the clarification. Indeed I made the assumption of backport in Numpy 2.0 since this release was still in a release candidate and not definitive, but it turns out I was wrong !

duburcqa avatar Jun 11 '24 10:06 duburcqa

@duburcqa Could you try the current devel of EigenPy? It seems to work on my side with Numpy2 delivered with conda.

jcarpent avatar Jun 18 '24 13:06 jcarpent

@jcarpent the issue isn't that it doesn't work at all with NumPy 2. What fails due to the current setup is that if you compile for NumPy 2 that extension (which uses eigenpy) cannot use an eigenpy compiled with NumPy 1.x. In the opposite direction, eigenpy is compiled against NumPy 2 and your extension using eigenpy is not, things might just crash in terrible ways (rather than getting an error).

seberg avatar Jun 18 '24 14:06 seberg

Exactly, @seberg summed it up pretty well.

duburcqa avatar Jun 18 '24 15:06 duburcqa

"I'm landing", thanks for the clear clarification.

jcarpent avatar Jun 18 '24 15:06 jcarpent

Hello @duburcqa, @seberg,

Thank for digging the issue. We can close this PR since #496 had been merged. We made the decision to turn EIGENPY_ARRAY_API public like it was in NumPy 1.x.

We know this will not allow binary compatibility but cleaning the way we expose NumPy API will be really hard, especially because eigenpy use a lot of template.

If it become an issue for some users we will try to address this but this will create a new major release I think.

jorisv avatar Sep 26 '24 09:09 jorisv