From ae77bf6871e9ebf4caf847c9122445081c9dcbbb Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Wed, 9 Oct 2024 13:18:58 +0200 Subject: [PATCH] Fix handling of conversion of 0d buffers (#5092) Also fixes conversion for 64 bit integer buffers. I added a buffer-test package so we can test 0d buffer conversion code without using numpy. In the future ideally we should add more test buffer types to buffer-test so that we can move more coverage of the buffer conversion code out of test_numpy and into the main test suite. Resolves pyodide#5084. --- .circleci/config.yml | 12 +-- docs/project/changelog.md | 6 ++ .../buffer-test/buffer-test/buffer-test.c | 98 +++++++++++++++++++ packages/buffer-test/buffer-test/setup.py | 16 +++ packages/buffer-test/meta.yaml | 10 ++ packages/buffer-test/test_buffer.py | 94 ++++++++++++++++++ src/core/jslib.c | 4 +- src/core/jsproxy.c | 2 +- src/core/python2js_buffer.js | 20 ++-- 9 files changed, 246 insertions(+), 16 deletions(-) create mode 100644 packages/buffer-test/buffer-test/buffer-test.c create mode 100644 packages/buffer-test/buffer-test/setup.py create mode 100644 packages/buffer-test/meta.yaml create mode 100644 packages/buffer-test/test_buffer.py diff --git a/.circleci/config.yml b/.circleci/config.yml index 3966d4883..8d26d9264 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -659,7 +659,7 @@ workflows: - test-main: name: test-core-chrome-nodylink - test-params: --runtime=chrome-no-host -m 'not requires_dynamic_linking' -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ + test-params: --runtime=chrome-no-host -m 'not requires_dynamic_linking' -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ packages/buffer-test/ requires: - build-core-nodylink filters: @@ -668,7 +668,7 @@ workflows: - test-main: name: test-core-firefox-nodylink - test-params: --runtime=firefox-no-host -m 'not requires_dynamic_linking' -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ + test-params: --runtime=firefox-no-host -m 'not requires_dynamic_linking' -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ packages/buffer-test/ requires: - build-core-nodylink filters: @@ -677,7 +677,7 @@ workflows: - test-main: name: test-core-node-nodylink - test-params: --runtime=node-no-host -m 'not requires_dynamic_linking' -k "not cmdline_runner" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ + test-params: --runtime=node-no-host -m 'not requires_dynamic_linking' -k "not cmdline_runner" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ packages/buffer-test/ requires: - build-core-nodylink filters: @@ -686,7 +686,7 @@ workflows: - test-main: name: test-core-chrome - test-params: --runtime=chrome-no-host -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ + test-params: --runtime=chrome-no-host -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ packages/buffer-test/ requires: - build-core filters: @@ -695,7 +695,7 @@ workflows: - test-main: name: test-core-firefox - test-params: --runtime=firefox-no-host -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ + test-params: --runtime=firefox-no-host -k "not webworker" src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ packages/buffer-test/ requires: - build-core filters: @@ -704,7 +704,7 @@ workflows: - test-main: name: test-core-node - test-params: --runtime=node-no-host src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ + test-params: --runtime=node-no-host src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ packages/buffer-test/ requires: - build-core filters: diff --git a/docs/project/changelog.md b/docs/project/changelog.md index 4b663ca62..1a18e83dc 100644 --- a/docs/project/changelog.md +++ b/docs/project/changelog.md @@ -63,6 +63,12 @@ myst: for namespace packages. {pr}`5039` +- {{ Fix }} It now works to convert a 0d Python buffer to JavaScript. + {pr}`5092` + +- {{ Fix }} It now works to convert buffers of 64 bit signed or unsigned integers to JavaScript. + {pr}`5092` + ### Packages - Upgraded `scikit-learn` to 1.5.2 {pr}`4823`, {pr}`5016`, {pr}`5072` diff --git a/packages/buffer-test/buffer-test/buffer-test.c b/packages/buffer-test/buffer-test/buffer-test.c new file mode 100644 index 000000000..be513ee5f --- /dev/null +++ b/packages/buffer-test/buffer-test/buffer-test.c @@ -0,0 +1,98 @@ +#define PY_SSIZE_T_CLEAN +#include + +// clang-format off +typedef struct +{ + PyObject_HEAD + Py_ssize_t byteLength; // invariant: byteLength should be equal to length * itemsize + Py_ssize_t length; + char data[16]; + char format[2]; + Py_ssize_t itemsize; +} ZeroDBufferObject; +// clang-format on + +static int +ZeroDBuffer_init(PyObject* o, PyObject* args, PyObject* kwds) +{ + ZeroDBufferObject* self = (ZeroDBufferObject*)o; + Py_buffer buf; + int fmt; + if (!PyArg_ParseTuple(args, "Cy*", &fmt, &buf)) { + return -1; + } + for (int i = 0; i < buf.len && i < 16; i++) { + self->data[i] = ((char*)buf.buf)[i]; + } + self->itemsize = buf.len; + PyBuffer_Release(&buf); + self->format[0] = fmt; + self->format[1] = 0; + return 0; +} + +static void +ZeroDBuffer_dealloc(PyObject* self) +{ +} + +static int +ZeroDBuffer_GetBuffer(PyObject* obj, Py_buffer* view, int flags) +{ + ZeroDBufferObject* self = (ZeroDBufferObject*)obj; + view->obj = NULL; + // This gets decremented automatically by PyBuffer_Release (even though + // bf_releasebuffer is NULL) + Py_INCREF(self); + + view->buf = &self->data; + view->obj = (PyObject*)self; + view->len = 1; + view->readonly = 0; + view->itemsize = self->itemsize; + view->format = self->format; + view->ndim = 0; + view->shape = NULL; + view->strides = NULL; + view->suboffsets = NULL; + + return 0; +} + +static PyBufferProcs ZeroDBuffer_BufferProcs = { + .bf_getbuffer = ZeroDBuffer_GetBuffer, + .bf_releasebuffer = NULL, +}; + +static PyTypeObject ZeroDBufferType = { + .tp_name = "ZeroDBuffer", + .tp_basicsize = sizeof(ZeroDBufferObject), + .tp_dealloc = ZeroDBuffer_dealloc, + .tp_as_buffer = &ZeroDBuffer_BufferProcs, + .tp_flags = Py_TPFLAGS_DEFAULT, + .tp_doc = PyDoc_STR("An internal helper buffer"), + .tp_init = ZeroDBuffer_init, + .tp_new = PyType_GenericNew, +}; + +static struct PyModuleDef module = { + PyModuleDef_HEAD_INIT, + "buffer_test", /* name of module */ + "Tests for buffers", /* module documentation, may be NULL */ + -1, /* size of per-interpreter state of the module, + or -1 if the module keeps state in global variables. */ +}; + +PyMODINIT_FUNC +PyInit_buffer_test(void) +{ + PyObject* module_object = PyModule_Create(&module); + if (module_object == NULL) { + return NULL; + } + if (PyModule_AddType(module_object, &ZeroDBufferType) == -1) { + return NULL; + } + return module_object; +} diff --git a/packages/buffer-test/buffer-test/setup.py b/packages/buffer-test/buffer-test/setup.py new file mode 100644 index 000000000..077306d5b --- /dev/null +++ b/packages/buffer-test/buffer-test/setup.py @@ -0,0 +1,16 @@ +from setuptools import Extension, setup + +setup( + name="buffer-test", + version="0.1.1", + author="Hood Chatham", + author_email="roberthoodchatham@gmail.com", + description="Test Python buffers", + long_description_content_type="text/markdown", + classifiers=[ + "Programming Language :: Python :: 3", + "License :: OSI Approved :: MIT License", + "Operating System :: OS Independent", + ], + ext_modules=[Extension("buffer_test", ["buffer-test.c"])], +) diff --git a/packages/buffer-test/meta.yaml b/packages/buffer-test/meta.yaml new file mode 100644 index 000000000..353867f23 --- /dev/null +++ b/packages/buffer-test/meta.yaml @@ -0,0 +1,10 @@ +package: + name: buffer-test + version: "0.1.1" + tag: + - core + - pyodide.test + top-level: + - buffer_test +source: + path: buffer-test diff --git a/packages/buffer-test/test_buffer.py b/packages/buffer-test/test_buffer.py new file mode 100644 index 000000000..d91728907 --- /dev/null +++ b/packages/buffer-test/test_buffer.py @@ -0,0 +1,94 @@ +import pytest +from pytest_pyodide import run_in_pyodide + + +@pytest.mark.requires_dynamic_linking +@run_in_pyodide(packages=["buffer-test"]) +def test_zerod_buffers(selenium): + from buffer_test import ZeroDBuffer + + from pyodide.ffi import to_js + + int8Buf = ZeroDBuffer("b", bytes([((~18) & 255) + 1])) + jsInt8Buf = to_js(int8Buf) + assert jsInt8Buf.constructor.name == "Int8Array" + assert jsInt8Buf.length == 1 + assert jsInt8Buf.byteLength == 1 + assert jsInt8Buf[0] == -18 + + uint8Buf = ZeroDBuffer("B", bytes([130])) + jsUint8Buf = to_js(uint8Buf) + assert jsUint8Buf.constructor.name == "Uint8Array" + assert jsUint8Buf.length == 1 + assert jsUint8Buf.byteLength == 1 + assert jsUint8Buf[0] == 130 + + int16Buf = ZeroDBuffer("h", bytes([18, 2])) + jsInt16Buf = to_js(int16Buf) + assert jsInt16Buf.constructor.name == "Int16Array" + assert jsInt16Buf.length == 1 + assert jsInt16Buf.byteLength == 2 + assert jsInt16Buf[0] == 18 + 2 * 256 + + uint16Buf = ZeroDBuffer("H", bytes([18, 2])) + jsUint16Buf = to_js(uint16Buf) + assert jsUint16Buf.constructor.name == "Uint16Array" + assert jsUint16Buf.length == 1 + assert jsUint16Buf.byteLength == 2 + assert jsUint16Buf[0] == 18 + 2 * 256 + + int32Buf = ZeroDBuffer("i", bytes([18, 2, 0, 1])) + jsInt32Buf = to_js(int32Buf) + assert jsInt32Buf.constructor.name == "Int32Array" + assert jsInt32Buf.length == 1 + assert jsInt32Buf.byteLength == 4 + assert jsInt32Buf[0] == 18 + 2 * 256 + 1 * 256 * 256 * 256 + + uint32Buf = ZeroDBuffer("I", bytes([18, 2, 0, 1])) + jsUint32Buf = to_js(uint32Buf) + assert jsUint32Buf.constructor.name == "Uint32Array" + assert jsUint32Buf.length == 1 + assert jsUint32Buf.byteLength == 4 + assert jsUint32Buf[0] == 18 + 2 * 256 + 1 * 256 * 256 * 256 + + int64Buf = ZeroDBuffer("q", bytes([18, 2, 0, 1, 0, 0, 0, 1])) + jsInt64Buf = to_js(int64Buf) + assert jsInt64Buf.constructor.name == "BigInt64Array" + assert jsInt64Buf.length == 1 + assert jsInt64Buf.byteLength == 8 + assert jsInt64Buf[0] == 18 + 2 * 256 + 1 * 256 * 256 * 256 + pow(256, 7) + + uint64Buf = ZeroDBuffer("Q", bytes([18, 2, 0, 1, 0, 0, 0, 1])) + jsUint64Buf = to_js(uint64Buf) + assert jsUint64Buf.constructor.name == "BigUint64Array" + assert jsUint64Buf.length == 1 + assert jsUint64Buf.byteLength == 8 + assert jsUint64Buf[0] == 18 + 2 * 256 + 1 * 256 * 256 * 256 + pow(256, 7) + + float32Buf = ZeroDBuffer("f", bytes([0, 0, 224, 64])) + jsFloat32Buf = to_js(float32Buf) + assert jsFloat32Buf.constructor.name == "Float32Array" + assert jsFloat32Buf.length == 1 + assert jsFloat32Buf.byteLength == 4 + assert jsFloat32Buf[0] == 7 + + float64Buf = ZeroDBuffer("d", bytes([0, 0, 0, 0, 0, 0, 28, 64])) + jsFloat64Buf = to_js(float64Buf) + assert jsFloat64Buf.constructor.name == "Float64Array" + assert jsFloat64Buf.length == 1 + assert jsFloat64Buf.byteLength == 8 + assert jsFloat64Buf[0] == 7 + + # Attempt to convert float16 array should fail with a ConversionError + float16Buf = ZeroDBuffer("e", bytes([0, 71])) + err = None + try: + to_js(float16Buf) + except Exception as e: + err = e + + assert err + try: + assert str(err.__cause__) == "Error: Javascript has no Float16 support." + finally: + del err diff --git a/src/core/jslib.c b/src/core/jslib.c index ec9ed0624..766aace8a 100644 --- a/src/core/jslib.c +++ b/src/core/jslib.c @@ -373,7 +373,7 @@ EM_JS_VAL(JsVal, JsvPromise_Resolve, (JsVal obj), { // clang-format off EM_JS_NUM(errcode, jslib_init_buffers_js, (), { - const dtypes_str = ["b", "B", "h", "H", "i", "I", "f", "d"].join( + const dtypes_str = Array.from("bBhHiIqQfd").join( String.fromCharCode(0) ); const dtypes_ptr = stringToNewUTF8(dtypes_str); @@ -391,6 +391,8 @@ EM_JS_NUM(errcode, jslib_init_buffers_js, (), { ["Uint32Array", [dtypes_map["I"], 4, true]], ["Float32Array", [dtypes_map["f"], 4, true]], ["Float64Array", [dtypes_map["d"], 8, true]], + ["BigInt64Array", [dtypes_map["q"], 8, true]], + ["BigUint64Array", [dtypes_map["Q"], 8, true]], // These last two default to Uint8. They have checked : false to allow use // with other types. ["DataView", [dtypes_map["B"], 1, false]], diff --git a/src/core/jsproxy.c b/src/core/jsproxy.c index bb3b785b7..711b15e13 100644 --- a/src/core/jsproxy.c +++ b/src/core/jsproxy.c @@ -3353,7 +3353,7 @@ Buffer_cinit(Buffer* self, return 0; } -void +static void Buffer_dealloc(PyObject* self) { PyMem_Free(((Buffer*)self)->data); diff --git a/src/core/python2js_buffer.js b/src/core/python2js_buffer.js index 70e7d0677..45bcbb2f0 100644 --- a/src/core/python2js_buffer.js +++ b/src/core/python2js_buffer.js @@ -175,17 +175,21 @@ Module.python2js_buffer_1d_noncontiguous = function ( * @private */ Module._python2js_buffer_recursive = function (ptr, curdim, bufferData) { + const { shape, strides, ndim, converter, itemsize, suboffsets } = bufferData; // Stride and suboffset are signed, n is unsigned. - let n = DEREF_U32(bufferData.shape, curdim); - let stride = DEREF_I32(bufferData.strides, curdim); + let n = DEREF_U32(shape, curdim); + let stride = DEREF_I32(strides, curdim); let suboffset = -1; - if (bufferData.suboffsets !== 0) { - suboffset = DEREF_I32(bufferData.suboffsets, curdim); + if (ndim === 0) { + return converter(Module.python2js_buffer_1d_contiguous(ptr, itemsize, 1)); } - if (curdim === bufferData.ndim - 1) { + if (suboffsets !== 0) { + suboffset = DEREF_I32(suboffsets, curdim); + } + if (curdim === ndim - 1) { // Last dimension, use appropriate 1d converter let arraybuffer; - if (stride === bufferData.itemsize && suboffset < 0) { + if (stride === itemsize && suboffset < 0) { arraybuffer = Module.python2js_buffer_1d_contiguous(ptr, stride, n); } else { arraybuffer = Module.python2js_buffer_1d_noncontiguous( @@ -193,10 +197,10 @@ Module._python2js_buffer_recursive = function (ptr, curdim, bufferData) { stride, suboffset, n, - bufferData.itemsize, + itemsize, ); } - return bufferData.converter(arraybuffer); + return converter(arraybuffer); } let result = [];