Use HPy to define the ndarray type #2

rlamy · 2021-01-06T19:05:24Z

PyArray_Type is now defined using HPyType_FromSpec(), which is a straightforward change. The difficulty was more in hacking the build and CI systems to support HPy.

Note that there are a few issues with this:

~~It doesn't compile on PyPy (because it lacks _HPyGetContext() ATM)~~
(edit) It doesn't work on PyPy, due to missing support for some legacy_slots.
Setting the buffer slots requires a CPython-specific hack.
Ditto for tp_weaklistoffset.

Also, ensure that the C sources for HPy are compiled in.

mattip · 2021-01-06T21:08:49Z

Maybe you could tie the pypy download to the latest nightly instead of the 7.3.3 release

hodgestar · 2021-01-08T09:33:47Z

setup.py

+        setup_requires=['hpy.devel'],
+        # distuils doesn't load hpy.devel unless hpy_ext_modules is present
+        # as a keyword
+        hpy_ext_modules=[],


@rlamy I'm not sure what the final fix should be yet, but in the mean time this small change made what was happening much more understandable. With it, --hpy-abi no longer gives an error for me. I still need to see what actually happens during the build.

antocuni

LGTM.

It doesn't compile on PyPy (because it lacks _HPyGetContext() ATM)

this comment is no longer true, isn't it?

antocuni · 2021-01-13T15:54:22Z

numpy/core/src/multiarray/arrayobject.c

-    .flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE),
-    .slots = PyArray_Type_slots,
+    .flags = (HPy_TPFLAGS_DEFAULT | HPy_TPFLAGS_BASETYPE),
+    .legacy_slots = PyArray_Type_slots,


just a note: looking at how it is easy to migrate an existing extension, I think that the invention of .legacy_slots is a Very Good Idea :)

numpy/core/src/multiarray/multiarraymodule.c

antocuni · 2021-01-13T16:02:46Z

numpy/core/src/multiarray/multiarraymodule.c

@@ -4767,12 +4773,12 @@ PyMODINIT_FUNC PyInit__multiarray_umath(void) {
    if (initumath(m) != 0) {
        goto err;
    }
-    return m;
+    return HPy_FromPyObject(ctx, m);


ah, this is interesting. I always assumed that the first thing to do to port a module to HPy would be to convert PyModuleDef into HPyModuleDef and thus using HPyModule_Create, but indeed, it's not required.

It is worth adding a note to docs/porting-guide.rst, maybe showing an example of the minimal diff which is required to hpy-ify a module.

antocuni · 2021-01-13T16:10:30Z

pyproject.toml

@@ -4,6 +4,7 @@ requires = [
    "setuptools<49.2.0",
    "wheel<=0.35.1",
    "Cython>=0.29.21,<3.0",  # Note: keep in sync with tools/cythonize.py
+    "hpy.devel @ git+https://github.com/hpyproject/hpy.git@8e20b89116c2993188157c09a6070a64f8efbd82#egg=hpy.devel"


maybe we should start to tag/release "real" revisions

antocuni · 2021-01-13T16:12:33Z

tools/travis-before-install.sh

@@ -49,6 +49,7 @@ pip install --upgrade pip 'setuptools<49.2.0' wheel
 # requirement using `grep cython test_requirements.txt` instead of simply
 # writing 'pip install setuptools wheel cython'.
 pip install `grep cython test_requirements.txt`
+pip install `grep hpy.devel test_requirements.txt`


is this needed for a good reason, or simply because we haven't released hpy.devel on PyPI? In theory the setup_requires=['hpy.devel'] should be enough for setuptools to do the "right thing", isn't it?

Well, I think that the comment about Cython applies to HPy as well: we probably want to ensure that we have a specific version of HPy and setup_requires may not be enough.

Well, I think that the comment about Cython applies to HPy as well

not necessarily: Cython has a weird way of integrating with setuptools: you need to call cythonize before the call to setup(), so setuptools doesn't even have a chance to install it even if you put in setup_requires.
HPy have a different approach, which is the same as cffi: it uses setuptool's entry points, so you don't need to be able to import hpy.devel before calling setup().

we probably want to ensure that we have a specific version of HPy and setup_requires may not be enough.

I think that setup_requires=['hpy.devel==0.123'] would work, once we have proper releases

antocuni · 2021-01-13T16:13:12Z

tools/travis-test.sh

@@ -25,7 +25,8 @@ if [ -n "$PYTHON_OPTS" ]; then
 fi

 # make some warnings fatal, mostly to match windows compilers
-werrors="-Werror=vla -Werror=nonnull -Werror=pointer-arith"
+# werrors="-Werror=vla -Werror=nonnull -Werror=pointer-arith"
+werrors="-Werror=nonnull -Werror=pointer-arith"


why do we need this?

because ATM HPy uses Variable Length Arrays, so the build fails with "-Werror=vla" on.

See discussion in hpyproject/hpy#125.

rlamy · 2021-01-13T21:10:14Z

Current status:

Works on CPython + 'cpython' ABI
Works on CPython + 'universal' ABI, except for one spurious failure
Broken on PyPy. The following legacy_slots need to be implemented: Py_tp_dealloc, Py_tp_getset, Py_tp_alloc, Py_tp_new, Py_tp_free

antocuni · 2021-01-13T22:55:44Z

Broken on PyPy. The following legacy_slots need to be implemented: Py_tp_dealloc, Py_tp_getset, Py_tp_alloc, Py_tp_new, Py_tp_free

I'm confused: currently tp_alloc & co. are commented out from public_api.h: how is it possible that it works with the universal ABI if you can't use them in HPy?
https://github.com/hpyproject/hpy/blob/fdc6047e6db755ecaeded1f805b91823eb11f08d/hpy/tools/autogen/public_api.h#L387-L392

Moreover, I'm not even sure if we can support the very same semantics as tp_alloc, tp_dealloc and tp_free directly, or if we need to find something which is more GC friendly. I need to think.

rlamy · 2021-01-14T12:50:54Z

I'm confused: currently tp_alloc & co. are commented out from public_api.h: how is it possible that it works with the universal ABI if you can't use them in HPy?

These are CPython slots, not HPy slots. They are trivially supported on CPython, with both ABIs. On PyPy, however, it's a lot more difficult. I think that the only way to support them is to fall back to cpyext and create a W_PyCTypeObject instead of a W_HPyTypeObject, which isn't very appealing.

mattip · 2021-01-14T13:13:58Z

I think tp_getset and tp_new are different from tp_alloc, tp_free, tp_dealloc since the first two have python dunder methods, no? The others are more tied to the implementation.

rlamy · 2021-01-18T22:18:29Z

I think tp_getset and tp_new are different from tp_alloc, tp_free, tp_dealloc since the first two have python dunder methods, no? The others are more tied to the implementation.

tp_getset could "easily" be supported. The problem with tp_new is that it returns a PyObject* and how do we turn that into a W_HPyObject?

mattip · 2021-01-18T22:52:01Z

The problem with tp_new is that it returns a PyObject* and how do we turn that into a W_HPyObject?

I think tp_new is outside the scope of what can be done with HPy. Not all the C-API can be converted to opaque handles.

antocuni · 2021-01-19T21:35:42Z

I'm confused again. We already support HPy_tp_new, and it's implemented also by PyPy. See e.g. test_hpytype.py:test_HPy_new.

The problem with tp_new is that it returns a PyObject* and how do we turn that into a W_HPyObject?

well, we already have machinery to convert HPy <==> PyObject*, we use it to implement HPy_AsPyObject, HPy_FromPyObject and in general all the legacy_* features. I fail to see what is the problem, but I might be overlooking something.

Also, keep on mind that using .legacy_slots = {tp_new, ...} is temporary, of course. Eventually, it should become a .slots = {HPy_tp_new, ...}

tp_alloc and tp_free are an open question: the current plan was to NOT support them: the idea is that the allocation/deallocation of memory should belong to the interpreter, to allow PyPy to allocate HPy instances in GC-managed memory, which should bring a big speed bump compared to malloc and free.
However, by doing so we prevent some legit optimizations on CPython: what if I e.g. want to use a free list for my instances?
One option could be to introduce a slot such as HPy_tp_alloc_maybe, and then each implementation is allowed to decide whether to use it or not. Ideally, we should find some concrete cases in which tp_alloc is used in the wild and check whether we want/can/should support those. Probably we should open a proper API design issue to discuss it.

tp_dealloc is a closed issue: we decided that we do NOT want to support it. Instead, we introduced HPy_tp_destroy, which is the moral equivalent of PyPy lightweight destructors: the key feature of HPy_tp_destroy is that it does not receive an HPyContext: this way we are sure that you can't call arbitrary python code from the destroyer, and the only thing you can do is to cleanup your C-level fields. Incidentally, this allows PyPy to implement it using lightweight destructors, see _hpy_universal/interp_type.py:W_HPyObject.__del__.

rlamy · 2021-01-20T20:01:02Z

I'm confused again. We already support HPy_tp_new, and it's implemented also by PyPy. See e.g. test_hpytype.py:test_HPy_new.

That's because you're thinking of HPy slots, again ;-) I'm only discussing legacy CPython slots here.

The problem with tp_new is that it returns a PyObject* and how do we turn that into a W_HPyObject?

well, we already have machinery to convert HPy <==> PyObject*, we use it to implement HPy_AsPyObject, HPy_FromPyObject and in general all the legacy_* features. I fail to see what is the problem, but I might be overlooking something.

The problem is that we'd have an HPy type whose instances are cpyext objects, which is likely to cause some headaches.

In any case, there is a more fundamental issue. In the CPython API, tp_alloc and tp_free are C function pointers that are only called from C code. tp_new and tp_dealloc implement, respectively, the dunder methods __new__() and __del__(). They do call tp_alloc/tp_free at the C-level. However, on pypy, HPyType_FromSpec() doesn't allocate a PyTypeObject, so trying to call PyArray_Type.tp_alloc will segfault.

Also, keep on mind that using .legacy_slots = {tp_new, ...} is temporary, of course. Eventually, it should become a .slots = {HPy_tp_new, ...}

In numpy's case, the temporary solution is likely to last for a long time. It would be rather sad if numpy had to stop supporting pypy because of HPy! But I guess it's fine if we say that tp_new et al. must be converted to be supported on pypy.

antocuni · 2021-01-20T23:21:50Z

The problem is that we'd have an HPy type whose instances are cpyext objects, which is likely to cause some headaches.

yes, but that's something we surely need to solve, probably sooner than later. Some relevant discussions:

pypy/module/_hpy_universal/test/support.py:test_legacy_slots_getsets: this is a test which we need to skip because AsPyObject doesn't work properly at the moment
this IRC conversation between me and @hodgestar about that test. Let me inline the relevant lines here for posterity:

<antocuni>	Hodgestar: I reviewed MR 783. I don't understand what you mean in your last sentence w.r.t issue 83, though
<Hodgestar>	antocuni: Thanks!
<Hodgestar>	antocuni: Re issue 83: The comment in PyPy on legacy_getset not being implemented implies that implementing it relies on hpy #83 being solved, but I'm not sure that's actually the case? If it is, I don't yet understand the link between the two.
<antocuni>	I can't find the comment :)
<antocuni>	ah, found it
<antocuni>	IIRC, the problem is that currently HPy_AsPyObject is "broken" if you call it on an hpy instance
<antocuni>	what happens is the following
<antocuni>	1. you create an HPy type, which is an W_HPyTypeObject
<antocuni>	2. you instantiate it and get a W_HPyObject
<antocuni>	2a. the W_HPyObject has a pointer hpy_data, which points to the user-defined C struct
<antocuni>	3. on CPython, when you call HPy_AsPyObject on such an object, you get a pointer to the C struct
<antocuni>	3a. so for example, if you are implementing a type point, on CPython you get a "struct Point*" on which you can do "p->x" and "p->y"
<antocuni>	4. on PyPy you get something completely different: you get the cpyext proxy for a generic W_Root: so, if you cast the PyObject* to "struct Point*" and do "p->x", things will explode (and/or, you will read nonsense)
<antocuni>	now, look at test_legacy_slot_getsets
<antocuni>	you see that it does exactly that: it receives a PyObject* point but it assumes it's castable to PointObject*
<Hodgestar>	Ah, got it.
<antocuni>	to go forward, we surely need to fix issue 83 kind of soon, but it's not so easy of course

Redesign legacy types and HPy_CAST hpy#83

To have any chance to implement any legacy_* stuff on PyPy, we need a two way HPy <==> PyObject*. I fear that the only reasonable way to implement is to declare that if an HPy type uses some tricky legacy_* stuff, it automatically becomes a cpyext type, and its instances are W_BaseCPyObject.
Then we need a way to implement HPy_FromPyObject: this can be done by adding special-cases here and there inside _hpy_universal, or maybe by introducing a base class which is a common ancestor of both W_HPyObject and W_BaseCPyObject.

What are the "tricky legacy_* stuff"? I don't know for sure. We could say that as soon as you use any legacy_*, your HPy type becomes a cpyext type, or we could try to be smarter and do it only if really needed (e.g., if you only use legacy_methods it might not be needed). It's worth noting that hpy/cpyext types will be slower than pure hpy types in PyPy, but this is maybe a good motivation to complete the porting :)

In any case, there is a more fundamental issue. In the CPython API, tp_alloc and tp_free are C function pointers that are only called from C code. tp_new and tp_dealloc implement, respectively, the dunder methods new() and del(). They do call tp_alloc/tp_free at the C-level. However, on pypy, HPyType_FromSpec() doesn't allocate a PyTypeObject, so trying to call PyArray_Type.tp_alloc will segfault.

well, with my proposed solution above, this problem will be automatically solved I think.

Also, keep on mind that using .legacy_slots = {tp_new, ...} is temporary, of course. Eventually, it should become a .slots = {HPy_tp_new, ...}

In numpy's case, the temporary solution is likely to last for a long time. It would be rather sad if numpy had to stop supporting pypy because of HPy! But I guess it's fine if we say that tp_new et al. must be converted to be supported on pypy.

why do you say so? Is there any fundamental issue which prevents ndarray.__new__ to be implemented in HPy?

rlamy · 2021-01-22T18:16:44Z

Yes, I agree that falling back to a cpyext type is the only solution to make it work on pypy. I guess the exact rules will be determined by the outcome of hpyproject/hpy#156.

commit 9c833bed5879d77e625556260690c349de18b433 Author: Thomas Li <[email protected]> Date: Wed Nov 17 16:21:27 2021 -0800 Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Add Windows config to GHA update script [wheel build] typo [wheel build] fix typo? [wheel build] fix linux builds? [wheel build] typo [wheel build] add license and pin to windows 2016 skip tests [wheel build] pin to windows 2019 instead [wheel build] try to find out the error on windows [wheel build] maybe fix? [wheel build] maybe fix? [wheel build] fix? [wheel build] cleanup [wheel build] Update LICENSE_win32.txt Update LICENSE_win32.txt Update cibw_test_command.sh commit 4bd12df Author: Thomas Li <[email protected]> Date: Mon Nov 15 17:28:47 2021 -0800 # This is a combination of 14 commits. # This is the 1st commit message: Add Windows config to GHA # This is the commit message #2: update script [wheel build] # This is the commit message #3: typo [wheel build] # This is the commit message #4: fix typo? [wheel build] # This is the commit message #5: fix linux builds? [wheel build] # This is the commit message #6: typo [wheel build] # This is the commit message #7: add license and pin to windows 2016 # This is the commit message #8: skip tests [wheel build] # This is the commit message #9: pin to windows 2019 instead [wheel build] # This is the commit message #10: try to find out the error on windows [wheel build] # This is the commit message #11: maybe fix? [wheel build] # This is the commit message #12: maybe fix? [wheel build] # This is the commit message #13: fix? [wheel build] # This is the commit message #14: cleanup [wheel build]

rlamy added 7 commits December 31, 2020 20:49

Start using HPy in multiarraymodule.c

969e1e1

ENH: Create PyArray_Type using HPyType_FromSpec()

8e55aa7

Also, ensure that the C sources for HPy are compiled in.

TST: Install HPy when testing

acee27f

TST: Allow VLAs (currently used in HPy)

337504f

TST: Install HPy before building numpy

46a179c

TST: Add HPy to pyproject.toml

f46629a

BLD: Fix HPy config for PyPy (not working though)

0371393

rlamy force-pushed the ndarray-as-hpy-type branch from 85865ae to 0371393 Compare January 6, 2021 19:12

rlamy and others added 4 commits January 7, 2021 12:01

ENH: Create _multiarray_umath module using HPy

be76a4b

BLD: Use HPy's entry_point to find its sources

abe1d63

fix warning

e6f5941

Add dummy hpy_ext_module keyword.

274de4e

hodgestar reviewed Jan 8, 2021

View reviewed changes

hodgestar and others added 3 commits January 11, 2021 15:02

Add stub writing support to numpy build_extension override.

945ce86

BUG: Hack ctypeslib to let it load a lib from a .hpy.so

8acae98

BUG: Fix np.core._multiarray_umath.__name__ in universal mode (hack!)

aeebf2b

antocuni approved these changes Jan 13, 2021

View reviewed changes

BLD: Select the correct ABI for the Python implementation

24c3c4f

rlamy merged commit 18019cb into hpy Jan 18, 2021

rlamy deleted the ndarray-as-hpy-type branch January 18, 2021 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use HPy to define the ndarray type #2

Use HPy to define the ndarray type #2

rlamy commented Jan 6, 2021 •

edited

Loading

mattip commented Jan 6, 2021

hodgestar Jan 8, 2021

antocuni left a comment

antocuni Jan 13, 2021

antocuni Jan 13, 2021

antocuni Jan 13, 2021

antocuni Jan 13, 2021

rlamy Jan 13, 2021

antocuni Jan 13, 2021

antocuni Jan 13, 2021

rlamy Jan 13, 2021

rlamy Jan 13, 2021

rlamy commented Jan 13, 2021

antocuni commented Jan 13, 2021

rlamy commented Jan 14, 2021

mattip commented Jan 14, 2021

rlamy commented Jan 18, 2021

mattip commented Jan 18, 2021

antocuni commented Jan 19, 2021

rlamy commented Jan 20, 2021

antocuni commented Jan 20, 2021

rlamy commented Jan 22, 2021

Use HPy to define the ndarray type #2

Use HPy to define the ndarray type #2

Conversation

rlamy commented Jan 6, 2021 • edited Loading

mattip commented Jan 6, 2021

Choose a reason for hiding this comment

antocuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlamy commented Jan 13, 2021

antocuni commented Jan 13, 2021

rlamy commented Jan 14, 2021

mattip commented Jan 14, 2021

rlamy commented Jan 18, 2021

mattip commented Jan 18, 2021

antocuni commented Jan 19, 2021

rlamy commented Jan 20, 2021

antocuni commented Jan 20, 2021

rlamy commented Jan 22, 2021

rlamy commented Jan 6, 2021 •

edited

Loading