Skip to content

Commit

Permalink
Fix handling of conversion of 0d buffers (pyodide#5092)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hoodmane committed Oct 18, 2024
1 parent 0453d82 commit 7fd66c6
Show file tree
Hide file tree
Showing 9 changed files with 246 additions and 16 deletions.
12 changes: 6 additions & 6 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,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:
Expand All @@ -730,7 +730,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:
Expand All @@ -739,7 +739,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/ pyodide-build/pyodide_build/tests
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/ pyodide-build/pyodide_build/tests packages/buffer-test/
requires:
- build-core-nodylink
filters:
Expand All @@ -748,7 +748,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:
Expand All @@ -757,7 +757,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:
Expand All @@ -766,7 +766,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/ pyodide-build/pyodide_build/tests
test-params: --runtime=node-no-host src packages/micropip packages/fpcast-test packages/sharedlib-test-py/ packages/cpp-exceptions-test/ pyodide-build/pyodide_build/tests packages/buffer-test/
requires:
- build-core
filters:
Expand Down
6 changes: 6 additions & 0 deletions docs/project/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ myst:
- {{ Fix }} `pyimport("a.b")` won't fail when `a` is removed by `del sys.modules["a"]`
{pr}`4993`

- {{ 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`

## Version 0.26.2

_July 26, 2024_
Expand Down
98 changes: 98 additions & 0 deletions packages/buffer-test/buffer-test/buffer-test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
#define PY_SSIZE_T_CLEAN
#include <Python.h>

// 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;
}
16 changes: 16 additions & 0 deletions packages/buffer-test/buffer-test/setup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from setuptools import Extension, setup

setup(
name="buffer-test",
version="0.1.1",
author="Hood Chatham",
author_email="[email protected]",
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"])],
)
10 changes: 10 additions & 0 deletions packages/buffer-test/meta.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package:
name: buffer-test
version: "0.1.1"
tag:
- core
- pyodide.test
top-level:
- buffer_test
source:
path: buffer-test
94 changes: 94 additions & 0 deletions packages/buffer-test/test_buffer.py
Original file line number Diff line number Diff line change
@@ -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
4 changes: 3 additions & 1 deletion src/core/jslib.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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]],
Expand Down
2 changes: 1 addition & 1 deletion src/core/jsproxy.c
Original file line number Diff line number Diff line change
Expand Up @@ -3283,7 +3283,7 @@ Buffer_cinit(Buffer* self,
return 0;
}

void
static void
Buffer_dealloc(PyObject* self)
{
PyMem_Free(((Buffer*)self)->data);
Expand Down
20 changes: 12 additions & 8 deletions src/core/python2js_buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,28 +175,32 @@ 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(
ptr,
stride,
suboffset,
n,
bufferData.itemsize,
itemsize,
);
}
return bufferData.converter(arraybuffer);
return converter(arraybuffer);
}

let result = [];
Expand Down

0 comments on commit 7fd66c6

Please sign in to comment.