Conversation
…put/upsert Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
| py::ssize_t expected_stride = info.itemsize; | ||
| for (py::ssize_t i = info.ndim - 1; i >= 0; --i) { | ||
| if (info.strides[i] != expected_stride) { | ||
| throw std::runtime_error("buffer must be C-contiguous"); | ||
| } | ||
| expected_stride *= info.shape[i]; | ||
| } |
There was a problem hiding this comment.
Could the stride walk be replaced with PyBuffer_IsContiguous? pybind11 (v2.13 in extern/) exposes the underlying Py_buffer via info.view():
| py::ssize_t expected_stride = info.itemsize; | |
| for (py::ssize_t i = info.ndim - 1; i >= 0; --i) { | |
| if (info.strides[i] != expected_stride) { | |
| throw std::runtime_error("buffer must be C-contiguous"); | |
| } | |
| expected_stride *= info.shape[i]; | |
| } | |
| if (!PyBuffer_IsContiguous(info.view(), 'C')) { | |
| throw std::runtime_error("buffer must be C-contiguous"); | |
| } |
The buffer protocol doesn't require canonical strides for dimensions of extent 0 or 1, so the hand-rolled check can reject buffers that are in fact C-contiguous. NumPy normalizes strides on export, but other exporters aren't obliged to. PyBuffer_IsContiguous handles those cases, and a NULL strides, the same way CPython itself does. The helper also gets shorter. It still throws RuntimeError, so the new tests pass unchanged.
There was a problem hiding this comment.
Done, switched to PyBuffer_IsContiguous(info.view(), 'C'). You were right that the manual walk is wrong on its own terms too: the buffer protocol leaves strides for extent-0 and extent-1 dimensions unspecified, so the old loop could reject buffers that are contiguous in memory. The comment above the helper now says why.
| const auto &info = infos.back(); | ||
| spans.emplace_back(static_cast<const char *>(info.ptr), | ||
| static_cast<size_t>(info.size)); | ||
| contiguous_buffer_bytes(info)); |
There was a problem hiding this comment.
The same silent-wrong-bytes problem is still present in upsert_parts and put_parts (L2903–2909 and L3163–3169 in this revision; GitHub won't let me anchor there since those lines aren't in the diff). Their guard checks ndim == 1 && itemsize == 1 but not strides, so a strided 1-D view like memoryview(b"abcdefgh")[::2] passes and gets stored as 4 contiguous bytes starting at ptr.
Since the helper is already here, keeping the existing guard and swapping the length is enough:
spans.emplace_back(static_cast<const char *>(info.ptr),
contiguous_buffer_bytes(info));With itemsize == 1 the byte count doesn't change; the only new behavior is the contiguity check. It would also be worth extending test_non_contiguous_buffer_rejected:
with self.assertRaises(RuntimeError):
self.store.put_parts("test_non_contiguous_parts", view)
with self.assertRaises(RuntimeError):
self.store.upsert_parts("test_non_contiguous_parts", view)I think this belongs in the same PR rather than a follow-up: it's the same bug class, #4299 points at these guards as the reference behavior, and the PR description currently calls them "unchanged".
There was a problem hiding this comment.
Done. Both put_parts and upsert_parts now also require PyBuffer_IsContiguous(info.view(), 'C'), so a strided 1-D view like memoryview(b"abcdefgh")[::2] is rejected instead of being stored as the wrong bytes. buf.request(false) passes PyBUF_STRIDES | PyBUF_FORMAT, so the strides in info.view() are real and the contiguity check actually sees them.
Added test_parts_strided_view_rejected next to test_non_contiguous_buffer_rejected covering both entry points.
Two review follow-ups: Use PyBuffer_IsContiguous instead of the hand-rolled stride walk. The buffer protocol leaves strides for extent-0/1 dimensions unspecified, so a manual walk can reject buffers that are contiguous in memory. put_parts/upsert_parts only checked ndim/itemsize, so a strided 1-D view (memoryview(b"abcdefgh")[::2]) passed and got stored as the wrong bytes. Add the contiguity check to both, plus a regression test. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
The buffer protocol leaves strides for extent-0/1 dimensions unspecified, and PyBuffer_IsContiguous only checks strides where the extent is greater than 1. A (1, N) broadcast view with stride 0 on the size-1 dim is C-contiguous in memory, so put must accept it and store the full byte range; the hand-rolled stride walk this series replaced would have rejected it. Skipped when numpy is missing since run_tests.sh executes this file before numpy is installed. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
|
Both points are addressed, and I have now added the acceptance side of the first one. The stride walk is gone: New test Re-ran the full |
This branch has not been deployed
2 participants
Description
Fixes #4299.
put,upsert,put_batchandupsert_batchaccept any Python buffer-protocol object, but they usedbuffer_info.sizeas the byte length. In pybind11,sizeis the element count, so for anything withitemsize > 1(numpy typed arrays,array.array("f"), typed memoryviews) only a fraction of the data was stored while the call returned 0. Strides were ignored too, so a non-contiguous view stored the wrong bytes.This change:
contiguous_buffer_bytes()helper that returnssize * itemsizeafter verifying the buffer is C-contiguous (stride walk from the last dimension).put_parts/upsert_partsguards (those still require 1-D bytes-like parts, unchanged).Module
mooncake-integration)mooncake-wheel)Type of Change
How Has This Been Tested?
Test commands:
Built the store module twice in the repo's dev container (
mc-dev:local, aarch64): once from this branch, once withstore_py.cpprestored frommain, then ran the wheel store suite againstmooncake_master --enable_http_metadata_server=true:Test results:
New regression tests in
test_distributed_object_store.py:test_typed_buffer_roundtrip:array("f", ...)and batcharray("d")/array("i")store and returnsize * itemsizebytes. On themainmodule this FAILS (stored size is truncated to the element count); on the branch module it passes.test_non_contiguous_buffer_rejected: a strided view raisesRuntimeError. On themainmodule no error is raised (wrong bytes stored silently); on the branch module it raises.Red/green was verified by swapping only the built module: same tests fail on the
mainbuild (2 failures) and pass on the branch build (2/2 OK). Fulltest_distributed_object_store.pyon the branch build: 18 tests, OK.Checklist
./scripts/code_format.shAI Assistance Disclosure
Kimi Code helped with the investigation, implementation and tests. I reviewed every changed line and ran the validation above.