add support for abi3t#556
Conversation
|
Ping @kumaraditya303 - we were just talking about this today. |
|
I confirmed that this resolved the runtime error I reported in #555 |
|
I think we should also add a check that when abi3t is used, the minimum numpy version should be 2.5. |
c2b4c58 to
6c722b0
Compare
|
Looks like 32bit windows still has some issues... not sure why though |
|
My guess is that'd be because there are no wheels for |
|
To me it looks like there are cp315-win32 wheels here and they installed fine. The version specific run had no issues as well. It's just the abi3t run that failed, so I think this is a genuine failure, but I'm not sure which fault it could be: ours, pyo3, numpy or python |
|
I think it could alignment issue on 32 bit MSVC, I am not very familiar with rust but it seems flags is defined as |
|
@Icxolu Can you try forcing a 4 byte alignment layout using |
|
If you all don't sort this out in the meantime I'll try to look on Monday. While win32 isn't so important per se, I do find it useful to detect and sort out CPU architecture-sensitive issues. It's possible this issue is in any one of CPython, NumPy, or rust-numpy. Glad to see everything mostly works otherwise! |
|
I did a bit of digging with compiler explorer: https://godbolt.org/z/qYe3Ghz7a |
|
I think I know what the issue is.
It's explicitly I think the fix needs to live in NumPy's headers. There probably needs to be a mirrored change here to ensure correct alignment for the |
|
I opened numpy/numpy#31759 |
|
It turns out my first attempt at a fix had a flaw and I think the only real fix requires NumPy to add new a C API function to fix this issue: numpy/numpy#31762 |
|
I am looking at this on numpy side as well but the following diff fixes the win32 issues on rust side: diff --git a/src/npyffi/objects.rs b/src/npyffi/objects.rs
index 6cd72ca..d3189f7 100644
--- a/src/npyffi/objects.rs
+++ b/src/npyffi/objects.rs
@@ -78,7 +78,14 @@ pub struct PyArray_DescrProto {
pub hash: npy_hash_t,
}
-#[repr(C)]
+#[cfg_attr(
+ all(target_arch = "x86", target_os = "windows", Py_LIMITED_API, Py_GIL_DISABLED),
+ repr(C, packed(4))
+)]
+#[cfg_attr(
+ not(all(target_arch = "x86", target_os = "windows", Py_LIMITED_API, Py_GIL_DISABLED)),
+ repr(C)
+)]
pub struct _PyArray_DescrNumPy2 {
#[cfg(not(all(Py_LIMITED_API, Py_GIL_DISABLED)))]
pub ob_base: PyObject,
@@ -96,7 +103,14 @@ pub struct _PyArray_DescrNumPy2 {
pub reserved_null: [*mut std::ffi::c_void; 2],
}
-#[repr(C)]
+#[cfg_attr(
+ all(target_arch = "x86", target_os = "windows", Py_LIMITED_API, Py_GIL_DISABLED),
+ repr(C, packed(4))
+)]
+#[cfg_attr(
+ not(all(target_arch = "x86", target_os = "windows", Py_LIMITED_API, Py_GIL_DISABLED)),
+ repr(C)
+)]
pub(crate) struct _PyArray_LegacyDescr_fields {
#[cfg(not(all(Py_LIMITED_API, Py_GIL_DISABLED)))]
pub ob_base: PyObject,
|
|
See https://github.com/kumaraditya303/rust-numpy/actions/runs/28233899128/job/83644222553, with the above change CI passes for windows 32 bit builds. |
|
Thank you both for continuing to look into this, much appreciated ❤️. That patch seems to make it run indeed. What I don't really understand is, why this should work. If I got it correctly, then from numpy's definition there are padding bytes inserted to align the flags field on 32bit. If we now tell Rust to use a packed layout, wouldn't we read the padding bytes instead of the field? I'd also assume that this than effects all 32bit platforms, not just windows and not just x86, we just don't have any CI configured for other cases. |
|
I think we need a similar fix on the NumPy side too. This does affect all 32 bit platforms, I think. |
This adds abi3t support by
Todo:
See numpy/numpy#31091
Closes #555