fixes from compiling with warnings by mattip · Pull Request #17 · hpyproject/numpy-hpy

mattip · 2023-11-15T09:40:13Z

These were almost all the fixes I needed to get a warning-less compilation on PyPy. I used this command line to build a wheel:

CFLAGS="-O0 -g3" python setup.py build --warn-error build_src --verbose-cfg bdist_wheel

PyPy still cannot cleanly load numpy, I get this after running the command above, which suggests something is off with the metaclass handling somewhere:

$ pip install dist/numpy-1.23.0.dev0+1466.g23f4a1898d-pp39-pypy39_pp73-linux_x86_64.whl
$ cd dist
$ python -c "import numpy"
$ python -c "import numpy"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/tmp/venv3pypy/lib/pypy3.9/site-packages/numpy/__init__.py", line 143, in <module>
    from . import lib
  File "/tmp/venv3pypy/lib/pypy3.9/site-packages/numpy/lib/__init__.py", line 21, in <module>
    from . import scimath as emath
  File "/tmp/venv3pypy/lib/pypy3.9/site-packages/numpy/lib/scimath.py", line 46, in <module>
    _ln2 = nx.log(2.0)
TypeError: object.__new__(float64) is not safe, use float64.__new__()

mattip · 2023-11-15T09:41:48Z

numpy/core/src/common/npy_hashtable.c


 NPY_NO_EXPORT PyObject *
-PyArrayIdentityHash_GetItem(PyObject *cache_owner, PyArrayIdentityHash const *tb, PyObject *const *key)
+PyArrayIdentityHash_GetItem(PyArrayIdentityHash const *tb, PyObject *cache_owner, PyObject *const *key)


This seems wrong? The argument order should be the same as PyArrayIdentityHash_SetItem

Okay, makes sense. Seems that I was just not consistent in adding the cache_owner argument.

mattip · 2023-11-15T09:42:31Z

numpy/core/src/multiarray/arraytypes.c.src

 {
    const char *s;
-    const char *end;
+    char *end;


this caused many warnings, so either my change or all the uses of end must be const char

mattip · 2023-11-15T09:43:01Z

numpy/core/src/multiarray/datetime_strings.c

                  NPY_ITER_ALLOCATE;

-    iter = NpyIter_MultiNew(2, op, flags, NPY_KEEPORDER, NPY_UNSAFE_CASTING,
+    iter = HNpyIter_MultiNew(ctx, 2, op, flags, NPY_KEEPORDER, NPY_UNSAFE_CASTING,


Since op seems to contain handles, calling the new function seems to make sense.

Yes, we probably just overlooked that.

mattip · 2023-11-15T09:44:58Z

numpy/core/src/multiarray/mapping.c

-        if (hpy_get_view_from_index(ctx, self, self_struct, &view, indices, 2, 0) < 0) {
+        hpy_indices[1].value = PyArray_NDIM(self_struct) - 1;
+        hpy_indices[1].type = HAS_ELLIPSIS;
+        if (hpy_get_view_from_index(ctx, self, self_struct, &view, hpy_indices, 2, 0) < 0) {


I needed to duplicate the indices struct, since there is a call to legacy get_item_pointer for struct dtypes on line 2672 above

Should indices be updated here too since both are being maintained?

No. Only hpy_indices is being maintained. indices is used only in get_item_pointer in a read-only fashion.

mattip · 2023-11-15T09:46:41Z

numpy/core/src/umath/ufunc_type_resolution.c

-    use_min_scalar = should_use_min_scalar(nin, op, 0, NULL);
+    hpy_abort_not_implemented("convert op to PyArrayObject**");
+    PyArrayObject ** dummy;
+    use_min_scalar = should_use_min_scalar(nin, dummy, 0, NULL);


should_use_min_scalar is a legacy interface, op is a HPy *

Is there something preventing us from calling HPy_AsPyObject(op) here:?

That might work, but I would prefer not to add more HPy_AsPyObject.

mattip · 2023-11-15T09:48:12Z

numpy/core/src/umath/wrapping_array_method.c

        }
-        wrapped_meth = (PyArrayMethodObject *)PyTuple_GET_ITEM(item, 1);
-        if (!PyObject_TypeCheck(wrapped_meth, &PyArrayMethod_Type)) {
+        wrapped_meth = (PyArrayMethodObject *)PyTuple_GetItem(item, 1);


Not sure why the macro GET_ITEM causes problems here, something about the type of item?

Me either, but using the macro failed on PyPy.

mattip · 2023-11-27T05:59:02Z

Would anyone like to review this?

mattip · 2023-12-07T10:56:25Z

Added a commit to close #18

mattip · 2023-12-20T17:22:34Z

Anyone want to review this?

hodgestar · 2023-12-20T22:19:20Z

numpy/core/src/multiarray/array_method.h

 } PyBoundArrayMethodObject;

-HPyType_HELPERS(PyBoundArrayMethodObject)
+HPyType_LEGACY_HELPERS(PyBoundArrayMethodObject)


I guess this had to become a legacy type so that someone could call HPy_AsPyObject on it?

🤔 good point. I don't think we ever specified that.
It is not required on CPython because any object is a PyObject anyway. So, it is always possible to convert.
It is not required for GraalPy because we would return a wrapper that emulates the PyObject.
Is it required for PyPy? If so, we should discuss that and maybe properly specify (and enforce spec in debug mode).

PyPy works around it but it would be more efficient if the HELPER was correct.

hodgestar · 2023-12-20T22:20:46Z

numpy/core/src/multiarray/ctors.c

        PyObject *py_range = HPy_AsPyObject(ctx, range);
        CAPI_WARN("calling PyArray_Byteswap");
-        new = PyArray_Byteswap(py_range, 1);
+        new = PyArray_Byteswap((PyArrayObject *)py_range, 1);


Should we perhaps do PyArrayObject *py_range = (PyArrayObject *) HPy_AsPyObject(ctx, range) where pyrange is defined?

I think this was has fewer casts, since most places this is used prefer a PyObject*

numpy/core/src/multiarray/mapping.c

hodgestar · 2023-12-20T22:28:30Z

numpy/core/src/multiarray/textreading/parser_config.h

      */
-     bool python_byte_converters;
-     bool c_byte_converters;
+     npy_bool python_byte_converters;


What is this change for?

npy_bool is the correct type used in the structs. This reduces the number of warnings

hodgestar

On the whole these changes look fairly unobjectionable to me. I asked some questions where I didn't understand things.

I would be awesome if someone from the GraalPy team could try this branch and confirm that it works for them, but if they're short on time at the moment, I think it would also be okay to merge it.

This isn't a production branch for anyone -- it's a place to push things forwards and get stuff working.

fangerer

Overall, looks good to me. Thanks you!

fangerer · 2023-12-21T08:42:24Z

numpy/core/include/numpy/ndarraytypes.h

        int flags;
 } PyArrayFlagsObject;

-HPyType_HELPERS(PyArrayFlagsObject);


Do you know from the top of your head if PyArrayFlagsObject still needs to be legacy? It looks like it could be a pure HPy type but we just forgot to remove PyObject_HEAD.

No, I don't know. We could check it in a follow-up PR

fangerer · 2023-12-21T08:45:47Z

numpy/core/src/common/npy_hashtable.c


 NPY_NO_EXPORT PyObject *
-PyArrayIdentityHash_GetItem(PyObject *cache_owner, PyArrayIdentityHash const *tb, PyObject *const *key)
+PyArrayIdentityHash_GetItem(PyArrayIdentityHash const *tb, PyObject *cache_owner, PyObject *const *key)


Okay, makes sense. Seems that I was just not consistent in adding the cache_owner argument.

fangerer · 2023-12-21T09:01:17Z

numpy/core/src/multiarray/abstractdtypes.c

    .name = "numpy._IntegerAbstractDType",
    .basicsize = sizeof(PyArray_Descr),
    .flags = Py_TPFLAGS_DEFAULT,
+    .builtin_shape = HPyType_BuiltinShape_Legacy


If there is a HPyType(_LEGACY)_HELPERS(PyArray_Descr), we should use SHAPE(PyArray_Descr) here.

right, but as far as I can tell there is no such use of the HEPLERS.

fangerer · 2023-12-21T09:07:26Z

numpy/core/src/multiarray/array_method.h

 } PyBoundArrayMethodObject;

-HPyType_HELPERS(PyBoundArrayMethodObject)
+HPyType_LEGACY_HELPERS(PyBoundArrayMethodObject)


🤔 good point. I don't think we ever specified that.
It is not required on CPython because any object is a PyObject anyway. So, it is always possible to convert.
It is not required for GraalPy because we would return a wrapper that emulates the PyObject.
Is it required for PyPy? If so, we should discuss that and maybe properly specify (and enforce spec in debug mode).

fangerer · 2023-12-21T09:30:43Z

numpy/core/src/multiarray/datetime_strings.c

                  NPY_ITER_ALLOCATE;

-    iter = NpyIter_MultiNew(2, op, flags, NPY_KEEPORDER, NPY_UNSAFE_CASTING,
+    iter = HNpyIter_MultiNew(ctx, 2, op, flags, NPY_KEEPORDER, NPY_UNSAFE_CASTING,


Yes, we probably just overlooked that.

Co-authored-by: Simon Cross <hodgestar+github@gmail.com>

hodgestar · 2025-05-05T12:11:24Z

@mattip How come this got closed rather than merged?

mattip · 2025-05-06T08:11:08Z

I deleted the branch by mistake. It has been over a year with no action. I redid it as #21

fixes from compiling with warnings

9653fe5

mattip commented Nov 15, 2023

View reviewed changes

fix error in check

47ab51b

add tp-traverse to dtype_legacy_defines

37986b0

mattip added 5 commits December 10, 2023 17:26

use legacy builtin_shape for abstract dtypes

01829b6

PyArrayFlagsObject is a legacy struct

d82d3a0

error check, add tp_traverse to flagsobject

cb60cf6

use HPyErr_Format instead of HPyErr_SetString

ccac6a1

make PyBoundArrayMethodObject a legacy type

6c3201b

hodgestar reviewed Dec 20, 2023

View reviewed changes

numpy/core/src/multiarray/mapping.c Outdated Show resolved Hide resolved

hodgestar reviewed Dec 20, 2023

View reviewed changes

hodgestar approved these changes Dec 20, 2023

View reviewed changes

fangerer approved these changes Dec 21, 2023

View reviewed changes

remove unneeded comment

1fa8ee7

Co-authored-by: Simon Cross <hodgestar+github@gmail.com>

mattip closed this May 5, 2025

mattip deleted the pypy-fixes branch May 5, 2025 11:20

mattip mentioned this pull request May 6, 2025

Hpy pypy fixes #21

Open

Conversation

mattip commented Nov 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Nov 27, 2023

Uh oh!

mattip commented Dec 7, 2023

Uh oh!

mattip commented Dec 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodgestar left a comment

Choose a reason for hiding this comment

Uh oh!

fangerer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodgestar commented May 5, 2025

Uh oh!

mattip commented May 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects