fixes from compiling with warnings #17

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

@@ -257,7 +257,7 @@ PyArrayIdentityHash_SetItem(PyArrayIdentityHash *tb, PyObject *cache_owner,


 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

@@ -630,7 +630,7 @@ static NPY_INLINE npy_longdouble
 hpy_string_to_long_double(HPyContext *ctx, HPy op)
 {
    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

@@ -1533,7 +1533,7 @@ datetime_as_string_impl(HPyContext *ctx, HPy NPY_UNUSED(ignored), const HPy *arg
    op_flags[1] = NPY_ITER_WRITEONLY|
                  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

@@ -240,8 +244,8 @@ PyUFunc_AddWrappingLoop(PyObject *ufunc_obj,
        if (cmp == 0) {
            continue;
        }
-        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

    HPyField *dtypes; /* PyArray_DTypeMeta **dtypes */
    HPyField method; /* PyArrayMethodObject *method */
    int nargs; /* method->nin + method->nout */
 } 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

@@ -4986,7 +4986,7 @@ HPyArray_ArangeObj(HPyContext *ctx, HPy start, HPy stop, HPy step, HPy /* PyArra
        PyObject *new;
        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

@@ -53,8 +53,8 @@ typedef struct {
      * while normal object to string casts only accept ASCII, so it ensures
      * that that the object array already contains bytes and not strings.
      */
-     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

@@ -930,7 +930,7 @@ typedef struct PyArrayFlagsObject {
        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

@@ -257,7 +257,7 @@ PyArrayIdentityHash_SetItem(PyArrayIdentityHash *tb, PyObject *cache_owner,


 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

@@ -108,18 +108,21 @@ NPY_NO_EXPORT HPyType_Spec HPyArray_PyIntAbstractDType_spec = {
    .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

    HPyField *dtypes; /* PyArray_DTypeMeta **dtypes */
    HPyField method; /* PyArrayMethodObject *method */
    int nargs; /* method->nin + method->nout */
 } 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

@@ -1533,7 +1533,7 @@ datetime_as_string_impl(HPyContext *ctx, HPy NPY_UNUSED(ignored), const HPy *arg
    op_flags[1] = NPY_ITER_WRITEONLY|
                  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 <[email protected]>

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 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes from compiling with warnings #17

fixes from compiling with warnings #17

mattip commented Nov 15, 2023

mattip Nov 15, 2023

fangerer Dec 21, 2023

mattip Nov 15, 2023

mattip Nov 15, 2023

hodgestar Dec 20, 2023

fangerer Dec 21, 2023

mattip Nov 15, 2023

hodgestar Dec 20, 2023

mattip Dec 23, 2023

mattip Nov 15, 2023

hodgestar Dec 20, 2023

mattip Dec 23, 2023

mattip Nov 15, 2023

mattip Dec 23, 2023

mattip commented Nov 27, 2023

mattip commented Dec 7, 2023

mattip commented Dec 20, 2023

hodgestar Dec 20, 2023

fangerer Dec 21, 2023

mattip Dec 23, 2023

hodgestar Dec 20, 2023

mattip Dec 23, 2023

hodgestar Dec 20, 2023

mattip Dec 23, 2023

hodgestar left a comment

fangerer left a comment

fangerer Dec 21, 2023

mattip Dec 23, 2023

fangerer Dec 21, 2023

fangerer Dec 21, 2023

mattip Dec 23, 2023

fangerer Dec 21, 2023

fangerer Dec 21, 2023

fixes from compiling with warnings #17

Are you sure you want to change the base?

fixes from compiling with warnings #17

Conversation

mattip commented Nov 15, 2023

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

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

mattip commented Nov 27, 2023

mattip commented Dec 7, 2023

mattip commented Dec 20, 2023

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

hodgestar left a comment

Choose a reason for hiding this comment

fangerer 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