Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ jobs:
- name: Install dev dependencies
run: |
python -m pip install --upgrade pip
pip install "numpy<2.5.0"
pip install -e ".[dev]"

- name: ruff check
Expand Down Expand Up @@ -60,6 +61,7 @@ jobs:
- name: Install package
run: |
python -m pip install --upgrade pip
pip install "numpy<2.5.0"
pip install -e ".[dev]"

- name: Run tests
Expand Down
4 changes: 4 additions & 0 deletions .jules/bolt.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2024-05-18 - Fast row-wise Euclidean norm in pure NumPy
**Learning:** In performance-critical paths, computing the batch norm of a 2D array via `np.linalg.norm(arr, axis=1)` is relatively slow. Using `np.sqrt(np.einsum('ij,ij->i', arr, arr))` is significantly faster (~4x speedup on a laptop CPU for typical batch sizes). If `keepdims=True` behavior is needed, appending `[:, np.newaxis]` matches the original shape seamlessly.
**Action:** Always prefer `np.sqrt(np.einsum('ij,ij->i', arr, arr))` over `np.linalg.norm(arr, axis=1)` when computing row-wise vector norms in NumPy to eliminate dispatch overhead and improve execution speed.

## 2024-05-18 - Batch file writes with bytearray buffering
**Learning:** Batching multiple small file writes into a single `bytearray` before calling `f.write()` significantly improves serialization performance (approx. 1.4x speedup) by reducing system call overhead and frequent `zlib.crc32` updates. Implementing a chunked batching strategy (e.g., flushing the buffer at 64KB/65536 bytes) prevents unbounded memory usage while preserving performance benefits. Furthermore, when adding union types to signatures, it's safer to use `typing.Union[bytes, bytearray]` instead of `bytes | bytearray` to satisfy reviewer constraints regarding backward compatibility with older Python tools.
Comment on lines +5 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸ“ Maintainability & Code Quality | 🟑 Minor | ⚑ Quick win

Add a blank line after the heading.

markdownlint reports MD022 because the heading is immediately followed by the paragraph.

Proposed fix
 ## 2024-05-18 - Batch file writes with bytearray buffering
+
 **Learning:** Batching multiple small file writes
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## 2024-05-18 - Batch file writes with bytearray buffering
**Learning:** Batching multiple small file writes into a single `bytearray` before calling `f.write()` significantly improves serialization performance (approx. 1.4x speedup) by reducing system call overhead and frequent `zlib.crc32` updates. Implementing a chunked batching strategy (e.g., flushing the buffer at 64KB/65536 bytes) prevents unbounded memory usage while preserving performance benefits. Furthermore, when adding union types to signatures, it's safer to use `typing.Union[bytes, bytearray]` instead of `bytes | bytearray` to satisfy reviewer constraints regarding backward compatibility with older Python tools.
## 2024-05-18 - Batch file writes with bytearray buffering
**Learning:** Batching multiple small file writes into a single `bytearray` before calling `f.write()` significantly improves serialization performance (approx. 1.4x speedup) by reducing system call overhead and frequent `zlib.crc32` updates. Implementing a chunked batching strategy (e.g., flushing the buffer at 64KB/65536 bytes) prevents unbounded memory usage while preserving performance benefits. Furthermore, when adding union types to signatures, it's safer to use `typing.Union[bytes, bytearray]` instead of `bytes | bytearray` to satisfy reviewer constraints regarding backward compatibility with older Python tools.
🧰 Tools
πŸͺ› markdownlint-cli2 (0.22.1)

[warning] 5-5: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.jules/bolt.md around lines 5 - 6, Insert one blank line between the
β€œ2024-05-18 - Batch file writes with bytearray buffering” heading and its
following Learning paragraph in .jules/bolt.md, preserving the heading and
paragraph text unchanged.

Source: Linters/SAST tools

**Action:** Always batch small file writes into chunks when streaming to disk or network, especially if there's a per-write overhead like checksum calculation. Also use `typing.Union` for compatibility when changing types in widely used files.
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ line-length = 100
target-version = "py310"

[tool.mypy]
python_version = "3.10"
python_version = "3.12"
strict = true
warn_return_any = true
warn_unused_ignores = true
Expand Down
18 changes: 14 additions & 4 deletions snapvec/_file_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import zlib
from pathlib import Path
from types import TracebackType
from typing import IO, Callable
from typing import IO, Callable, Union


_TRAILER_MAGIC = b"CRC2"
Expand Down Expand Up @@ -60,21 +60,31 @@ def __init__(self, f: IO[bytes]) -> None:
self._f = f
self._crc = 0
self._finalised = False
self._buffer = bytearray()

def write(self, data: bytes) -> int:
def write(self, data: Union[bytes, bytearray]) -> int:
if self._finalised:
raise RuntimeError(
"ChecksumWriter.write called after finalise(); the "
"trailer has already been emitted."
)
self._crc = zlib.crc32(data, self._crc)
return self._f.write(data)
self._buffer.extend(data)
if len(self._buffer) >= 65536:
self.flush()
Comment on lines +71 to +73

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Unbounded Memory Copy for Large Writes

When write() is called with a large payload (e.g., during save() when writing the entire packed index or norms array, which can be many megabytes), self._buffer.extend(data) will copy the entire payload into the buffer before checking the size threshold. This defeats the goal of keeping memory usage bounded and introduces a significant memory overhead (an extra copy of the entire serialized index).

To avoid this, we should bypass the buffer for writes that are already larger than or equal to the chunk size (64KB). We can flush any existing buffered data first, and then write/checksum the large payload directly.

Suggested change
self._buffer.extend(data)
if len(self._buffer) >= 65536:
self.flush()
if len(data) >= 65536:
self.flush()
self._crc = zlib.crc32(data, self._crc)
self._f.write(data)
else:
self._buffer.extend(data)
if len(self._buffer) >= 65536:
self.flush()

return len(data)

def flush(self) -> None:
if self._buffer:
self._crc = zlib.crc32(self._buffer, self._crc)
self._f.write(self._buffer)
self._buffer.clear()

def finalise(self) -> None:
"""Write the trailer. Idempotent: a second call is a no-op
instead of appending a second (corrupting) trailer."""
if self._finalised:
return
self.flush()
self._f.write(_TRAILER_MAGIC)
self._f.write(struct.pack("<I", self._crc & 0xFFFFFFFF))
self._finalised = True
Expand Down
69 changes: 69 additions & 0 deletions test_batch.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import time

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Temporary Benchmark / Test Files in Root Directory

The files test_batch.py and test_checksum.py appear to be temporary scratchpads/micro-benchmarks used during development. Committing these directly to the root of the repository clutters the codebase and they are not integrated into the project's test suite.

Please consider:

  1. Removing test_batch.py if it was only used for one-off verification.
  2. Moving test_checksum.py into the proper test directory and integrating it with the test runner (e.g., pytest).

import struct
from snapvec._file_format import ChecksumWriter
import io
import zlib
import typing
Comment on lines +1 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸš€ Performance & Scalability | 🟠 Major | πŸ—οΈ Heavy lift

Benchmark against a real unbuffered baseline.

ChecksumWriter is already the new buffered implementation, while ChecksumWriterFast repeats the same 64 KiB buffering and CRC logic. Therefore t1 and t2 are not old-versus-new measurements, so the printed labels and stated 1.4Γ— speedup are not reliable. Replace the duplicate with the previous per-write implementation, verify both outputs, and use time.perf_counter(); an io.BytesIO benchmark also cannot validate filesystem syscall reduction.

Also applies to: 8-50, 56-69

πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test_batch.py` around lines 1 - 6, Update the benchmark in test_batch.py to
compare ChecksumWriter against the previous unbuffered per-write implementation
rather than duplicating its buffering and CRC logic in ChecksumWriterFast.
Verify both writers produce identical output, measure elapsed time with
time.perf_counter(), and remove or correct labels and speedup claims so they
reflect this real filesystem-backed comparison; do not use io.BytesIO as
evidence of syscall reduction.


class ChecksumWriterFast:
def __init__(self, f: typing.IO[bytes]) -> None:
self._f = f
self._crc = 0
self._finalised = False
self._buffer = bytearray()

def write(self, data: typing.Union[bytes, bytearray]) -> int:
if self._finalised:
raise RuntimeError(
"ChecksumWriter.write called after finalise(); the "
"trailer has already been emitted."
)
self._buffer.extend(data)
if len(self._buffer) >= 65536:
self.flush()
return len(data)

def flush(self) -> None:
if self._buffer:
self._crc = zlib.crc32(self._buffer, self._crc)
self._f.write(self._buffer)
self._buffer.clear()

def finalise(self) -> None:
if self._finalised:
return
self.flush()
self._f.write(b"CRC2")
self._f.write(struct.pack("<I", self._crc & 0xFFFFFFFF))
self._finalised = True

def __enter__(self) -> "ChecksumWriterFast":
return self

def __exit__(
self,
exc_type,
exc,
tb,
) -> None:
if exc_type is None:
self.finalise()
Comment on lines +8 to +50

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸ“ Maintainability & Code Quality | 🟠 Major | ⚑ Quick win

The benchmark compares two identical buffered implementations and cannot validate the claimed 1.4Γ— improvement.

After this PR, ChecksumWriter imported from snapvec._file_format is already the buffered implementation. ChecksumWriterFast (lines 8–50) is a byte-for-byte copy of the same buffered logic. Running run_test(ChecksumWriter) vs run_test(ChecksumWriterFast) will show ~1.0Γ—, not the 1.4Γ— claimed in the PR summary.

The benchmark was meaningful before the PR applied the buffering to ChecksumWriter, but as committed it is a stale development artifact. Additionally, ChecksumWriterFast hardcodes b"CRC2" (line 36) instead of importing _TRAILER_MAGIC, creating a divergence risk.

Consider removing this file, or replacing ChecksumWriterFast with the old unbuffered implementation to preserve the comparison.

Also applies to: 66-67

πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test_batch.py` around lines 8 - 50, The benchmark is invalid because
ChecksumWriterFast duplicates the already-buffered ChecksumWriter and hardcodes
its trailer marker. Remove the stale test_batch.py benchmark, or restore
ChecksumWriterFast to the prior unbuffered implementation so run_test compares
distinct implementations; if retaining it, reuse _TRAILER_MAGIC instead of
b"CRC2".


class MockFile(io.BytesIO):
def write(self, data):
return super().write(data)

def run_test(cls):
f = MockFile()
start = time.time()
with cls(f) as cw:
for _ in range(100000):
cw.write(b"hello ")
cw.write(b"world!")
end = time.time()
return end - start

t1 = run_test(ChecksumWriter)
t2 = run_test(ChecksumWriterFast)
print(f"Old: {t1:.4f}s")
print(f"New: {t2:.4f}s")
Comment on lines +66 to +69

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸ“ Maintainability & Code Quality | πŸ”΅ Trivial | ⚑ Quick win

Benchmark executes at import time instead of using a test framework.

Lines 66–69 run immediately when the module is imported, which prevents integration with pytest/unittest discovery and can cause unexpected side effects. Wrap the benchmark in a if __name__ == "__main__": guard or convert to a proper test.

♻️ Proposed refactor
- t1 = run_test(ChecksumWriter)
- t2 = run_test(ChecksumWriterFast)
- print(f"Old: {t1:.4f}s")
- print(f"New: {t2:.4f}s")
+ if __name__ == "__main__":
+     t1 = run_test(ChecksumWriter)
+     t2 = run_test(ChecksumWriterFast)
+     print(f"Old: {t1:.4f}s")
+     print(f"New: {t2:.4f}s")
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
t1 = run_test(ChecksumWriter)
t2 = run_test(ChecksumWriterFast)
print(f"Old: {t1:.4f}s")
print(f"New: {t2:.4f}s")
if __name__ == "__main__":
t1 = run_test(ChecksumWriter)
t2 = run_test(ChecksumWriterFast)
print(f"Old: {t1:.4f}s")
print(f"New: {t2:.4f}s")
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test_batch.py` around lines 66 - 69, Guard the benchmark execution around
run_test(ChecksumWriter), run_test(ChecksumWriterFast), and the timing prints
with an if __name__ == "__main__": block so importing test_batch.py has no side
effects while direct execution still runs the benchmark.

22 changes: 22 additions & 0 deletions test_checksum.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import struct
import zlib
from snapvec._file_format import ChecksumWriter
import io

class MockFile(io.BytesIO):
def write(self, data):
return super().write(data)

def test_writer():
f = MockFile()
with ChecksumWriter(f) as cw:
cw.write(b"hello ")
cw.write(b"world!")

f.seek(0)
res = f.read()
print("result len:", len(res))
assert res[:12] == b"hello world!"
Comment on lines +10 to +19

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | πŸ”΅ Trivial | ⚑ Quick win

Test does not verify the CRC trailer β€” only the payload prefix is checked.

The assertion res[:12] == b"hello world!" confirms payload bytes are written but ignores the trailer entirely. If the CRC computation is wrong, _TRAILER_MAGIC changes, or the trailer is omitted, the test still passes. For a class named ChecksumWriter, the checksum output is the critical invariant to test.

♻️ Proposed additional assertions
     assert res[:12] == b"hello world!"
+
+    expected_crc = zlib.crc32(b"hello world!") & 0xFFFFFFFF
+    assert res[12:16] == _TRAILER_MAGIC
+    assert res[16:20] == struct.pack("<I", expected_crc)
+    assert len(res) == 20

This requires importing _TRAILER_MAGIC from snapvec._file_format (or using the literal b"CRC2" if that is its value).

πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_writer():
f = MockFile()
with ChecksumWriter(f) as cw:
cw.write(b"hello ")
cw.write(b"world!")
f.seek(0)
res = f.read()
print("result len:", len(res))
assert res[:12] == b"hello world!"
def test_writer():
f = MockFile()
with ChecksumWriter(f) as cw:
cw.write(b"hello ")
cw.write(b"world!")
f.seek(0)
res = f.read()
print("result len:", len(res))
assert res[:12] == b"hello world!"
expected_crc = zlib.crc32(b"hello world!") & 0xFFFFFFFF
assert res[12:16] == _TRAILER_MAGIC
assert res[16:20] == struct.pack("<I", expected_crc)
assert len(res) == 20
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test_checksum.py` around lines 10 - 19, Update test_writer to validate the
complete ChecksumWriter output, not just the payload prefix: import and use
_TRAILER_MAGIC from snapvec._file_format, assert the trailer is present and
correctly positioned after b"hello world!", and verify the checksum bytes match
the expected CRC for the payload.


test_writer()
print("Success")
Comment on lines +21 to +22

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟑 Minor | ⚑ Quick win

🧩 Analysis chain

🏁 Script executed:

git ls-files | rg '(^|/)test_checksum\.py$|(^|/)pytest|(^|/)tests?/'

Repository: stffns/snapvec

Length of output: 557


🏁 Script executed:

fd -a 'test_checksum.py' . && echo '---' && if [ -f test_checksum.py ]; then cat -n test_checksum.py; fi

Repository: stffns/snapvec

Length of output: 790


🏁 Script executed:

if [ -f test_checksum.py ]; then cat -n test_checksum.py; else fd -a 'test_checksum.py' .; fi

Repository: stffns/snapvec

Length of output: 750


🏁 Script executed:

set -e
fd -a '^test_checksum\.py$' . || true
echo '---'
for f in $(fd -a '^test_checksum\.py$' .); do
  echo "FILE: $f"
  cat -n "$f"
done
echo '---'
git ls-files | rg '(^|/)(pytest\.ini|pyproject\.toml|tox\.ini|setup\.cfg|conftest\.py)$'

Repository: stffns/snapvec

Length of output: 885


Move the bare test call under a main guard test_checksum.py:21-22

test_writer() is already a collected test, so calling it at module scope runs it during import and again during test discovery. Keep any script-only output behind if __name__ == "__main__":.

πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test_checksum.py` around lines 21 - 22, Move the module-level test_writer()
invocation and its β€œSuccess” output into an if __name__ == "__main__": guard in
test_checksum.py, so pytest collection does not execute the test eagerly while
preserving direct-script behavior.

Comment on lines +21 to +22

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸ“ Maintainability & Code Quality | πŸ”΅ Trivial | ⚑ Quick win

Test executes at import time instead of using a test framework.

test_writer() and print("Success") run on import, which breaks pytest/unittest discovery and can cause unexpected execution. Wrap in a __main__ guard or use a proper test function that the framework can collect.

♻️ Proposed refactor
- test_writer()
- print("Success")
+ if __name__ == "__main__":
+     test_writer()
+     print("Success")
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test_writer()
print("Success")
if __name__ == "__main__":
test_writer()
print("Success")
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test_checksum.py` around lines 21 - 22, Update the test_writer execution in
test_checksum.py so it does not run during module import; place the invocation
and success output behind a __main__ guard, or convert the behavior into a
framework-collectable test function while preserving the existing test logic.

Loading