Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Buffer Protocol #8

Merged
merged 40 commits into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
94714e9
init work for buffer protocol
delta003 Sep 1, 2023
1da4948
buffers module
delta003 Sep 1, 2023
4120a6f
This commit will be squashed.
delta003 Sep 1, 2023
8960e60
inplace, don't decref
delta003 Sep 1, 2023
632af3c
This commit will be squashed.
delta003 Sep 1, 2023
8ac93a3
This commit will be squashed.
delta003 Sep 1, 2023
b44f30d
better doc
delta003 Sep 1, 2023
1ba0eb2
some progress on the array to buffer
delta003 Sep 1, 2023
3f14aed
iterate in zig
delta003 Sep 1, 2023
c55dc78
merge develop
delta003 Sep 4, 2023
2fcebc4
merge develop
delta003 Sep 4, 2023
fb866b8
Use pytest.build.zig in debug example
delta003 Sep 4, 2023
0c80134
Merge branch 'mb/use-pytest-build' of github.com:fulcrum-so/ziggy-pyd…
delta003 Sep 4, 2023
b4820f8
This commit will be squashed.
delta003 Sep 4, 2023
39950fb
This commit will be squashed.
delta003 Sep 4, 2023
1dc7506
__buffer__, __release_buffer__
delta003 Sep 4, 2023
ba2ce8b
__buffer__
delta003 Sep 4, 2023
fc32d01
Merge branch 'develop' of github.com:fulcrum-so/ziggy-pydust into mb/…
delta003 Sep 4, 2023
96c5fe7
revert lock
delta003 Sep 4, 2023
34c0a85
This commit will be squashed.
delta003 Sep 4, 2023
11a91e2
merge, refactor
delta003 Sep 5, 2023
3b4355b
release buffer
delta003 Sep 5, 2023
52cbd40
optional ref
delta003 Sep 5, 2023
0fd2326
add some useful methods
delta003 Sep 5, 2023
6a90e96
union with allocator oom
delta003 Sep 5, 2023
4b0e423
raise buffer
delta003 Sep 5, 2023
142dc8e
This commit will be squashed.
delta003 Sep 5, 2023
449604d
add sum, handle errors
delta003 Sep 5, 2023
8a221b0
This commit will be squashed.
delta003 Sep 5, 2023
77e1cf1
This commit will be squashed.
delta003 Sep 5, 2023
123e06e
This commit will be squashed.
delta003 Sep 5, 2023
56b1ec6
flags
delta003 Sep 5, 2023
ea73657
Merge branch 'develop' of github.com:fulcrum-so/ziggy-pydust into mb/…
delta003 Sep 5, 2023
6e0af6d
remove self obj
delta003 Sep 5, 2023
c4bf1e2
comments
delta003 Sep 5, 2023
b1377d8
more improvements
delta003 Sep 5, 2023
f2680b3
Merge branch 'develop' of github.com:fulcrum-so/ziggy-pydust into mb/…
delta003 Sep 5, 2023
60e6881
comments
delta003 Sep 5, 2023
f7d69e2
better error message
delta003 Sep 5, 2023
54f4817
types
delta003 Sep 5, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions example/buffers.zig
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
const std = @import("std");
const py = @import("pydust");

pub const ConstantBuffer = py.class("ConstantBuffer", struct {
pub const __doc__ = "A class implementing a buffer protocol";
const Self = @This();

values: []i64,
shape: []isize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the shape is only ever 1-dimensional, this can just be pylength: isize right? And leave a comment that it's an isize to be compatible with Python API.

Then just take the address of it, possibly needing a ptr cast.

format: [:0]const u8 = "l", // i64

pub fn __init__(self: *Self, args: *const extern struct { elem: py.PyLong, size: py.PyLong }) !void {
self.values = try py.allocator.alloc(i64, try args.size.as(u64));
@memset(self.values, try args.elem.as(i64));
self.shape = try py.allocator.alloc(isize, 1);
self.shape[0] = @intCast(self.values.len);
}

pub fn __buffer__(self: *const Self, view: *py.PyBuffer, flags: c_int) !void {
// For more details on request types, see https://docs.python.org/3/c-api/buffer.html#buffer-request-types
if (flags & py.PyBuffer.WRITABLE != 0) {
return py.BufferError.raise("buffer is not writable");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that the caller didn't request a writable buffer? I'm sure you can still give them a writable one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a caller requested writable, this will fail. I was going to show an example of using flags - reject a request for a writable buffer.

}

const pyObj = try py.self(@constCast(self));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robert3005 this looks right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should constCast inside py.self instead:

pub fn self(selfInstance: anytype) !types.PyObject {
    const selfState = @fieldParentPtr(pytypes.State(@typeInfo(@TypeOf(selfInstance)).Pointer.child), "state", selfInstance);
    return .{ .py = @constCast(&selfState.obj) };  <--- here
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@constcast is weird here. But I don't think we've extracted out the logic to correctly construct the self argument for functions that are not class methods.

view.initFromSlice(i64, self.values, self.shape, pyObj);

// We need to incref the self object because it's being used by the view.
pyObj.incref();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like initFromSlice should do this?

}

pub fn __release_buffer__(self: *const Self, view: *py.PyBuffer) void {
py.allocator.free(self.values);
py.allocator.free(self.shape);
// It might be necessary to clean up the view here. Depends on the implementation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of more complex / custom buffer implementation. That's what I was trying to say

_ = view;
}
});

// A function that accepts an object implementing the buffer protocol.
pub fn sum(args: *const extern struct { buf: py.PyObject }) !i64 {
// ND is required by asSlice.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use PyBUF_SIMPLE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can but I thought it's useful to get the slice of bytes of specific type, even if you don't get back the right shape. Happy to switch for this example.

var view = try py.PyBuffer.of(args.buf, py.PyBuffer.ND);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should actually be a function on PyObject? It aligns with C API of PyObject_GetBuffer and it's maybe clearer what's happening?

So it would be:

var buffer: py.PyBuffer = undefined;
args.buf.getBuffer(&buffer)

defer view.release();

var bufferSum: i64 = 0;
for (view.asSlice(i64)) |value| bufferSum += value;
return bufferSum;
}

comptime {
py.module(@This());
}
4 changes: 2 additions & 2 deletions pydust/src/errors.zig
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
const std = @import("std");
const Allocator = @import("std").mem.Allocator;

pub const PyError = error{
// Propagate an error raised from another Python function call.
// This is the equivalent of returning PyNULL and allowing the already set error info to remain.
Propagate,
Raised,
} || std.mem.Allocator.Error;
} || Allocator.Error;
2 changes: 2 additions & 0 deletions pydust/src/functions.zig
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ const reservedNames = .{
"__init__",
"__len__",
"__del__",
"__buffer__",
"__release_buffer__",
};

/// Parse the arguments of a Zig function into a Pydust function siganture.
Expand Down
27 changes: 27 additions & 0 deletions pydust/src/pytypes.zig
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,20 @@ fn Slots(comptime name: [:0]const u8, comptime definition: type, comptime Instan
}};
}

if (@hasDecl(definition, "__buffer__")) {
slots_ = slots_ ++ .{ffi.PyType_Slot{
.slot = ffi.Py_bf_getbuffer,
.pfunc = @ptrCast(@constCast(&bf_getbuffer)),
}};
}

if (@hasDecl(definition, "__release_buffer__")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should fold this inside the it block above and compileError if you have one and not the other?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an optional

The __release_buffer__ method should be called when a caller no longer needs the buffer returned by __buffer__. It corresponds to the bf_releasebuffer C slot. This is an optional part of the buffer protocol.

slots_ = slots_ ++ .{ffi.PyType_Slot{
.slot = ffi.Py_bf_releasebuffer,
.pfunc = @ptrCast(@constCast(&bf_releasebuffer)),
}};
}

slots_ = slots_ ++ .{ffi.PyType_Slot{
.slot = ffi.Py_tp_methods,
.pfunc = @ptrCast(@constCast(&methods.pydefs)),
Expand Down Expand Up @@ -136,6 +150,19 @@ fn Slots(comptime name: [:0]const u8, comptime definition: type, comptime Instan

ffi.PyErr_Restore(error_type, error_value, error_tb);
}

fn bf_getbuffer(self: *ffi.PyObject, view: *ffi.Py_buffer, flags: c_int) callconv(.C) c_int {
// In case of any error, the view.obj field must be set to NULL.
view.obj = null;

const instance: *Instance = @ptrCast(self);
return tramp.errVoid(definition.__buffer__(&instance.state, @ptrCast(view), flags));
}

fn bf_releasebuffer(self: *ffi.PyObject, view: *ffi.Py_buffer) callconv(.C) void {
const instance: *Instance = @ptrCast(self);
return definition.__release_buffer__(&instance.state, @ptrCast(view));
}
};
}

Expand Down
1 change: 1 addition & 0 deletions pydust/src/types.zig
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pub usingnamespace @import("types/bool.zig");
pub usingnamespace @import("types/buffer.zig");
pub usingnamespace @import("types/dict.zig");
pub usingnamespace @import("types/error.zig");
pub usingnamespace @import("types/float.zig");
Expand Down
137 changes: 137 additions & 0 deletions pydust/src/types/buffer.zig
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
const std = @import("std");
const py = @import("../pydust.zig");
const ffi = py.ffi;
const PyError = @import("../errors.zig").PyError;

/// Wrapper for Python Py_buffer.
/// See: https://docs.python.org/3/c-api/buffer.html
pub const PyBuffer = extern struct {
const Self = @This();

pub const SIMPLE: c_int = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe wrap these in a pub const Flags = struct { ... } just so it's obvious what they are. I was initially confused by py.PyBuffer.ND

pub const WRITABLE: c_int = 0x0001;
pub const FORMAT: c_int = 0x0004;
pub const ND: c_int = 0x0008;
pub const STRIDES: c_int = 0x0010 | ND;
pub const C_CONTIGUOUS: c_int = 0x0020 | STRIDES;
pub const F_CONTIGUOUS: c_int = 0x0040 | STRIDES;
pub const ANY_CONTIGUOUS: c_int = 0x0080 | STRIDES;
pub const INDIRECT: c_int = 0x0100 | STRIDES;
pub const CONTIG: c_int = STRIDES | WRITABLE;
pub const CONTIG_RO: c_int = ND;
pub const STRIDED: c_int = STRIDES | WRITABLE;
pub const STRIDED_RO: c_int = STRIDES;
pub const RECORDS: c_int = STRIDES | FORMAT | WRITABLE;
pub const RECORDS_RO: c_int = STRIDES | FORMAT;
pub const FULL: c_int = STRIDES | FORMAT | WRITABLE | ND;
pub const FULL_RO: c_int = STRIDES | FORMAT | ND;

buf: ?[*]u8,

// Use pyObj to get the PyObject.
// This must be an optional pointer so we can set null value.
obj: ?*ffi.PyObject,

// product(shape) * itemsize.
// For contiguous arrays, this is the length of the underlying memory block.
// For non-contiguous arrays, it is the length that the logical structure would
// have if it were copied to a contiguous representation.
len: isize,
itemsize: isize,
readonly: c_int,

// If ndim == 0, the memory location pointed to by buf is interpreted as a scalar of size itemsize.
// In that case, both shape and strides are NULL.
ndim: c_int,
format: [*:0]const u8,

shape: ?[*]isize = null,
// If strides is NULL, the array is interpreted as a standard n-dimensional C-array.
// Otherwise, the consumer must access an n-dimensional array as follows:
// ptr = (char *)buf + indices[0] * strides[0] + ... + indices[n-1] * strides[n-1];
strides: ?[*]isize = null,
// If all suboffsets are negative (i.e. no de-referencing is needed),
// then this field must be NULL (the default value).
suboffsets: ?[*]isize = null,
internal: ?*anyopaque = null,

pub fn release(self: *Self) void {
ffi.PyBuffer_Release(@ptrCast(self));
}

pub fn pyObj(self: *Self) py.PyObject {
return .{ .py = self.obj orelse unreachable };
}

// Flag is a combination of ffi.PyBUF_* flags.
// See: https://docs.python.org/3/c-api/buffer.html#buffer-request-types
pub fn of(obj: py.PyObject, flag: c_int) !PyBuffer {
if (ffi.PyObject_CheckBuffer(obj.py) != 1) {
return py.BufferError.raise("object does not support buffer interface");
}

var out: Self = undefined;
if (ffi.PyObject_GetBuffer(obj.py, @ptrCast(&out), flag) != 0) {
// Error is already raised.
return PyError.Propagate;
}
return out;
}

pub fn initFromSlice(self: *Self, comptime value_type: type, values: []value_type, shape: []isize, obj: py.PyObject) void {
self.* = .{
.buf = std.mem.sliceAsBytes(values).ptr,
.obj = obj.py,
.len = @intCast(values.len * @sizeOf(value_type)),
.itemsize = @sizeOf(value_type),
.readonly = 1,
.ndim = 1,
.format = getFormat(value_type).ptr,
.shape = shape.ptr,
};
}

// asSlice returns buf property as Zig slice. The view must have been created with ND flag.
pub fn asSlice(self: *const Self, comptime value_type: type) []value_type {
return @alignCast(std.mem.bytesAsSlice(value_type, self.buf.?[0..@intCast(self.len)]));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a start #29


fn getFormat(comptime value_type: type) [:0]const u8 {
switch (@typeInfo(value_type)) {
.Int => |i| {
switch (i.signedness) {
.unsigned => switch (i.bits) {
8 => return "B",
16 => return "H",
32 => return "I",
64 => return "L",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you pushed the return inside the switch, you can just have else => {} and then raise a single compileError at the end of this function

else => {
@compileError("Unsupported buffer value type" ++ @typeName(value_type));
},
},
.signed => switch (i.bits) {
8 => return "b",
16 => return "h",
32 => return "i",
64 => return "l",
else => {
@compileError("Unsupported buffer value type" ++ @typeName(value_type));
},
},
}
},
.Float => |f| {
switch (f.bits) {
32 => return "f",
64 => return "d",
else => {
@compileError("Unsupported buffer value type" ++ @typeName(value_type));
},
}
},
else => {
@compileError("Unsupported buffer value type" ++ @typeName(value_type));
},
}
}
};
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,7 @@ root = "example/functions.zig"
[[tool.pydust.ext_module]]
name = "example.classes"
root = "example/classes.zig"

[[tool.pydust.ext_module]]
name = "example.buffers"
root = "example/buffers.zig"
17 changes: 17 additions & 0 deletions test/test_buffers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
from array import array

from example import buffers


def test_view():
buffer = buffers.ConstantBuffer(1, 10)
view = memoryview(buffer)
for i in range(10):
assert view[i] == 1
view.release()


def test_sum():
# array implements a buffer protocol
arr = array("l", [1, 2, 3, 4, 5])
assert buffers.sum(arr) == 15