Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

MAINT: Convert multiarray to multi-phase init (PEP 489)#29022

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
seberg merged 1 commit intonumpy:mainfromAA-Turner:multi-phase/multiarray
May 29, 2025

Conversation

AA-Turner
Copy link
Contributor

@AA-TurnerAA-Turner commentedMay 21, 2025
edited
Loading

@AA-Turner
Copy link
ContributorAuthor

The failures here are (will be) as the relevant parts ofnpy_static_pydata are not cleared on module deallocation, so e.g.npy_cpu_dispatch_tracer_init() and other functions checking the global state fail. We can add them_clear/m_free slots to the moduledef, but I'm not sure which bits we should reset.

The other option is to keep treating those variables as global and instead re-add the same object to the new module. This may cause other problems though, as I don't believe it's the intended execution model.

There also appears to be a SIGSEV around the call todtypemeta_wrap_legacy_descriptor() inarraytypes.c.src, but I'm not familiar enough with the templating process here.

cc@ngoldbaum if you'd be able to suggest anything?

A

@AA-TurnerAA-Turnerforce-pushed themulti-phase/multiarray branch 2 times, most recently from760440b tof5eed00CompareMay 22, 2025 05:47
@seberg
Copy link
Member

Nice, will be great to make progress here!

We have a lot of global state, but much of it can probably just stay intact within the same interpreter (some cannot, e.g. because new singletons are created but this was always incorrect, so for a first start...).

So without multi-stage init, CPython would not run the module initially again if the so was already loaded (so deleting it fromsys.modules and importing again doesn't trigger a second init)?

One problem withdtypemeta_wrap_legacy_descriptor is that it mutates global singleton instances. The actual problem should be indtypemeta_wrap_legacy_descriptor indtypemeta.c.

There is a check to catch types that are set to bad values, I honestly don't remember why it is there, and the loop definitely looks bad (it should just look up the type number of themdescr and not loop!).
So it may just be replacing the loop withPyArray_DescrFromType(descr->type_num);.

So long we don't actually support subinterpreters, the old type object should actually be good to use.@AA-Turner maybe you can try applying something like this:

diff --git a/numpy/_core/src/multiarray/dtypemeta.c b/numpy/_core/src/multiarray/dtypemeta.cindex 0b1b0fb391..adb268097c 100644--- a/numpy/_core/src/multiarray/dtypemeta.c+++ b/numpy/_core/src/multiarray/dtypemeta.c@@ -1075,6 +1075,11 @@ dtypemeta_wrap_legacy_descriptor(     _PyArray_LegacyDescr *descr, PyArray_ArrFuncs *arr_funcs,     PyTypeObject *dtype_super_class, const char *name, const char *alias) {+    if (Py_TYPE(descr) != NULL && NPY_DTYPE(descr)->singleton == (PyArray_Descr *)descr) {+        /* We seem to already have created a type, multiple inits? */+        return 0;+    }+     int has_type_set = Py_TYPE(descr) == &PyArrayDescr_Type;      if (!has_type_set) {

(but there will likely be a long tail of these type of things, and of course the real fix will be to move the singletons into proper module state!)

AA-Turner reacted with thumbs up emoji

@AA-TurnerAA-Turnerforce-pushed themulti-phase/multiarray branch 2 times, most recently from0683e1d toc7378beCompareMay 28, 2025 14:43
@AA-Turner
Copy link
ContributorAuthor

Linux Qemu tests / riscv64 (pull_request) failure unrelated:E: Failed to fetch http://ports.ubuntu.com/ubuntu-ports/dists/jammy-updates/universe/binary-riscv64/Packages.gz File has unexpected size (1163769 != 1163765). Mirror sync in progress? [IP: 91.189.91.104 80]

@ngoldbaum
Copy link
Member

Yeah we get spurious failures due to network issues quite a bit, safe to ignore.

Also, wow, awesome, all the CI passes otherwise 🎉

@AA-Turner
Copy link
ContributorAuthor

Also, wow, awesome, all the CI passes otherwise 🎉

Before this is merged, one question. I've had to changetest_reloading asmultiarray is the first extension module imported, and hence we are now hitting the defensive error in

msg=f"""
IMPORTANT: PLEASE READ THIS FOR ADVICE ON HOW TO SOLVE THIS ISSUE!
Importing the numpy C-extensions failed. This error can happen for
many reasons, often due to issues with your setup or how NumPy was
installed.
{bad_c_module_info}
We have compiled some common reasons and troubleshooting tips at:
https://numpy.org/devdocs/user/troubleshooting-importerror.html
Please note and check the following:
* The Python version is: Python{major}.{minor} from "{sys.executable}"
* The NumPy version is: "{__version__}"
and make sure that they are the versions you expect.
Please carefully study the information and documentation linked above.
This is unlikely to be a NumPy issue but will be caused by a bad install
or environment on your machine.
Original error was:{exc}
"""

Is it worth adding a special case for where we detect re-initialisation? I've not done so currently because it is less work, but I'd be remiss for not asking.

A

@seberg
Copy link
Member

Would be nice to avoid printing out that monster if it isn't too hard, considering that it is -- for once -- really unrelated.

@AA-TurnerAA-Turnerforce-pushed themulti-phase/multiarray branch fromc7378be to67034c7CompareMay 28, 2025 16:45
@AA-Turner
Copy link
ContributorAuthor

I've updated_core/__init__.py to avoid raising the wall-of-text error for the reinitialisation exception, hopefully this works.

A

return 0;

err:
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

Not a big thing, but can you just remove theerr label? No point in usinggoto err if there is no cleanup needed.

AA-Turner reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Done; I avoided doing so in the first instance as it makes the diff much larger.

@AA-TurnerAA-Turnerforce-pushed themulti-phase/multiarray branch from67034c7 tod4b3f9bCompareMay 29, 2025 09:04
@sebergseberg changed the titleGH-29021: Convert multiarray to multi-phase init (PEP 489)MAINT: Convert multiarray to multi-phase init (PEP 489)May 29, 2025
@sebergseberg merged commite107f24 intonumpy:mainMay 29, 2025
74 checks passed
@AA-TurnerAA-Turner deleted the multi-phase/multiarray branchMay 29, 2025 12:09
MaanasArora pushed a commit to MaanasArora/numpy that referenced this pull requestJul 10, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@sebergsebergseberg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@AA-Turner@seberg@ngoldbaum

[8]ページ先頭

©2009-2025 Movatter.jp