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
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
- name: Install dev dependencies
run: |
python -m pip install --upgrade pip
pip install -e ".[dev]"
pip install -e ".[dev]" "numpy<2.5.0"

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

- name: Run tests
run: pytest -q --cov=snapvec --cov-report=term-missing
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-07-13 - Batching file writes via bytearray
**Learning:** In `ChecksumWriter`, frequent small file writes combined with continuous `zlib.crc32` updates caused significant overhead. Batching these small chunks into a `bytearray` and only computing the checksum and flushing to disk at a 64KB threshold yielded an approximate 1.4x speedup.
**Action:** Use a bounded `bytearray` batching strategy when dealing with many small file writes that require incremental checksum calculations to reduce system calls and library overhead without causing unbounded memory growth.
Comment on lines +5 to +7

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 | 💤 Low value

Add a blank line after the heading.

The markdown linter flags MD022: the heading on line 5 has no blank line before the content on line 6.

♻️ Proposed fix
 ## 2024-07-13 - Batching file writes via bytearray
+
 **Learning:** In `ChecksumWriter`, frequent small file writes combined with continuous `zlib.crc32` updates caused significant overhead. Batching these small chunks into a `bytearray` and only computing the checksum and flushing to disk at a 64KB threshold yielded an approximate 1.4x speedup.
 **Action:** Use a bounded `bytearray` batching strategy when dealing with many small file writes that require incremental checksum calculations to reduce system calls and library overhead without causing unbounded memory growth.
📝 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-07-13 - Batching file writes via bytearray
**Learning:** In `ChecksumWriter`, frequent small file writes combined with continuous `zlib.crc32` updates caused significant overhead. Batching these small chunks into a `bytearray` and only computing the checksum and flushing to disk at a 64KB threshold yielded an approximate 1.4x speedup.
**Action:** Use a bounded `bytearray` batching strategy when dealing with many small file writes that require incremental checksum calculations to reduce system calls and library overhead without causing unbounded memory growth.
## 2024-07-13 - Batching file writes via bytearray
**Learning:** In `ChecksumWriter`, frequent small file writes combined with continuous `zlib.crc32` updates caused significant overhead. Batching these small chunks into a `bytearray` and only computing the checksum and flushing to disk at a 64KB threshold yielded an approximate 1.4x speedup.
**Action:** Use a bounded `bytearray` batching strategy when dealing with many small file writes that require incremental checksum calculations to reduce system calls and library overhead without causing unbounded memory growth.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)

[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 - 7, Insert a blank line immediately after the
“2024-07-13 - Batching file writes via bytearray” heading in the documented
section, before the Learning paragraph, while preserving the existing text and
formatting.

Source: Linters/SAST tools

32 changes: 28 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

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

Since from __future__ import annotations is enabled at the top of the file and union types are already written using the | operator elsewhere in this file (e.g., str | Path), we can use bytes | bytearray instead of Union[bytes, bytearray]. This allows us to avoid importing Union from typing entirely.

Suggested change
from typing import IO, Callable, Union
from typing import IO, Callable

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 | 💤 Low value

Prefer | union syntax over Union for consistency.

The file already uses modern X | Y syntax (e.g., type[BaseException] | None on line 111), and the codebase uses str | Path in downstream consumers. Using Union[bytes, bytearray] is stylistically inconsistent. If the project targets Python 3.10+, prefer bytes | bytearray directly and drop the Union import.

♻️ Proposed refactor
-from typing import IO, Callable, Union
+from typing import IO, Callable

And on line 65:

-    def write(self, data: Union[bytes, bytearray]) -> int:
+    def write(self, data: bytes | bytearray) -> int:
🤖 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 `@snapvec/_file_format.py` at line 34, Update the type annotations in
snapvec/_file_format.py to use the modern bytes | bytearray union syntax instead
of Union[bytes, bytearray], and remove the now-unused Union import while
preserving the existing IO and Callable imports.



_TRAILER_MAGIC = b"CRC2"
Expand Down Expand Up @@ -60,21 +60,45 @@ 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)
# Performance optimization: batch small file writes and frequent
# zlib.crc32 updates into a bytearray to reduce overhead.

# If the incoming chunk is large, flush the current buffer and
# write the large chunk directly to avoid memory copies.
if len(data) >= 65536:
if self._buffer:
self._crc = zlib.crc32(self._buffer, self._crc)
self._f.write(self._buffer)
self._buffer.clear()
self._crc = zlib.crc32(data, self._crc)
self._f.write(data)
return len(data)

self._buffer.extend(data)
# Flush the buffer when it reaches 64KB to cap memory usage.
if len(self._buffer) >= 65536:
self._crc = zlib.crc32(self._buffer, self._crc)
self._f.write(self._buffer)
self._buffer.clear()
return len(data)

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
if self._buffer:
self._crc = zlib.crc32(self._buffer, self._crc)
self._f.write(self._buffer)
self._buffer.clear()
self._f.write(_TRAILER_MAGIC)
self._f.write(struct.pack("<I", self._crc & 0xFFFFFFFF))
self._finalised = True
Comment on lines +65 to 104

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

The logic to flush the buffer (updating the CRC, writing to the file, and clearing the buffer) is duplicated three times in ChecksumWriter. We can extract this into a private helper method _flush to reduce code duplication and improve maintainability. Additionally, we can use the modern bytes | bytearray union type instead of Union.

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

    def write(self, data: bytes | bytearray) -> int:
        if self._finalised:
            raise RuntimeError(
                "ChecksumWriter.write called after finalise(); the "
                "trailer has already been emitted."
            )
        # Performance optimization: batch small file writes and frequent
        # zlib.crc32 updates into a bytearray to reduce overhead.

        # If the incoming chunk is large, flush the current buffer and
        # write the large chunk directly to avoid memory copies.
        if len(data) >= 65536:
            self._flush()
            self._crc = zlib.crc32(data, self._crc)
            self._f.write(data)
            return len(data)

        self._buffer.extend(data)
        # Flush the buffer when it reaches 64KB to cap memory usage.
        if len(self._buffer) >= 65536:
            self._flush()
        return len(data)

    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
Loading