-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
…ust into mb/buffers
pydust/src/types/buffer.zig
Outdated
return getWithFlag(obj, ffi.PyBUF_FULL_RO); | ||
} | ||
|
||
pub fn getWithFlag(obj: py.PyObject, flag: c_int) !Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-pub I think
pydust/src/types/buffer.zig
Outdated
var out: Self = undefined; | ||
if (ffi.PyObject_GetBuffer(obj.py, @ptrCast(&out), flag) != 0) { | ||
// TODO(marko): This should be an error once we figure out how to do it | ||
@panic("unable to get buffer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just return PyError.Propagate since the protocol says the exporter must have raised a BufferError
}}; | ||
} | ||
|
||
if (@hasDecl(definition, "__release_buffer__")) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
pydust/src/pytypes.zig
Outdated
@@ -119,6 +133,18 @@ fn Slots(comptime name: [:0]const u8, comptime definition: type, comptime Instan | |||
|
|||
ffi.PyErr_Restore(error_type, error_value, error_tb); | |||
} | |||
|
|||
pub fn bf_getbuffer(self: *ffi.PyObject, view: *ffi.Py_buffer, flags: c_int) callconv(.C) c_int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to be pub
example/buffers.zig
Outdated
|
||
// Accept a buffer protocol object. | ||
pub fn sum(args: *const extern struct { buf: py.PyObject }) !py.PyLong { | ||
var view = try py.PyBuffer.of(args.buf, py.ffi.PyBUF_C_CONTIGUOUS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this less primitive in the future. The returned PyBuffer could have a lot more useful methods
example/buffers.zig
Outdated
@memset(self.values, elem); | ||
} | ||
|
||
// TODO(marko): Get obj from self. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is blocking I think
return py.BufferError.raise("buffer is not writable"); | ||
} | ||
|
||
const pyObj = try py.self(@constCast(self)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robert3005 this looks right?
There was a problem hiding this comment.
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
}
pydust/src/types/buffer.zig
Outdated
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)])); | ||
} |
There was a problem hiding this comment.
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
example/buffers.zig
Outdated
const Self = @This(); | ||
|
||
values: []i64, | ||
shape: []isize, |
There was a problem hiding this comment.
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.
example/buffers.zig
Outdated
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"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
return py.BufferError.raise("buffer is not writable"); | ||
} | ||
|
||
const pyObj = try py.self(@constCast(self)); |
There was a problem hiding this comment.
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.
example/buffers.zig
Outdated
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(); |
There was a problem hiding this comment.
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?
example/buffers.zig
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
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
|
||
// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
example/buffers.zig
Outdated
// 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. | ||
var view = try py.PyBuffer.of(args.buf, py.PyBuffer.ND); |
There was a problem hiding this comment.
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)
pydust/src/types/buffer.zig
Outdated
pub const PyBuffer = extern struct { | ||
const Self = @This(); | ||
|
||
pub const SIMPLE: c_int = 0; |
There was a problem hiding this comment.
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
8 => return "B", | ||
16 => return "H", | ||
32 => return "I", | ||
64 => return "L", |
There was a problem hiding this comment.
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
https://docs.python.org/3/c-api/buffer.html
and a few other useful link https://peps.python.org/pep-3118/ & https://peps.python.org/pep-0688 & https://jakevdp.github.io/blog/2014/05/05/introduction-to-the-python-buffer-protocol/