Skip to content

Commit b213b54

Browse files
d-v-bd-v-b
authored
fix: make benchmark page-cache clearing opt-in, never prompt for sudo (#4200)
* fix: byte-order handling for structured dtypes in the bytes codec (#220) * fix: byte-order handling for structured dtypes in the bytes codec The bytes codec neither byte-swapped structured-dtype fields to its configured endian on encode (numpy reports byteorder '|' for void dtypes, so the top-level byteorder comparison never detected a mismatch) nor honored its endian when decoding, silently corrupting any structured data whose field byte order differed from the stored one (e.g. virtual references to external big-endian data). Encode now detects byte-order mismatches by comparing full dtypes via newbyteorder, and decode reinterprets raw bytes in the stored byte order before converting to the data type's declared byte order, so the stored layout (codec state) and the in-memory layout (array data type) are independent. Closes #4141 Assisted-by: ClaudeCode:claude-fable-5 * test: fold structured byte-order cases into existing bytes codec tests Extend test_endian's parametrization with structured dtypes and test_bytes_codec_sync_roundtrip with endian/dtype parametrization plus stored-layout and decoded-dtype assertions, instead of adding parallel test functions for the same properties. Assisted-by: ClaudeCode:claude-fable-5 * refactor: rename stored_dtype to view_dtype in BytesCodec decode The variable is the dtype used to view the raw chunk bytes (byte order from the codec's endian configuration), not a property of the stored data or of the returned buffer, which always carries the array's declared dtype. Assisted-by: ClaudeCode:claude-fable-5 * docs: note that the decode-side byte-order conversion copies the chunk Assisted-by: ClaudeCode:claude-fable-5 * fix: make benchmark page-cache clearing opt-in, never prompt for sudo clear_cache() in tests/benchmarks/test_e2e.py ran sudo unconditionally in every benchmark's setup, so a plain `pytest` run blocked on a password prompt (#4199). It is now a no-op unless ZARR_BENCHMARK_CLEAR_CACHE is set, uses `sudo -n` so it can never block interactively, and the broken Darwin invocation ("&&" passed as an argument to sync) is fixed. The benchmark CI jobs set the variable to keep clearing caches. Closes #4199 Assisted-by: ClaudeCode:claude-fable-5
1 parent aa2b8e2 commit b213b54

4 files changed

Lines changed: 25 additions & 3 deletions

File tree

‎.github/workflows/codspeed.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ jobs:
3333
version: '1.16.5'
3434
- name: Run the benchmarks
3535
uses: CodSpeedHQ/action@f99becdce5e5d51fd556489ebef684f4ecfd6286 # v4.18.5
36+
env:
37+
ZARR_BENCHMARK_CLEAR_CACHE: '1'
3638
with:
3739
mode: walltime
3840
run: hatch run test.py3.12-minimal:pytest tests/benchmarks --codspeed

‎.github/workflows/test.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,8 @@ jobs:
178178
- name: Install Hatch
179179
run: python -m pip install hatch==1.16.5
180180
- name: Run Benchmarks
181+
env:
182+
ZARR_BENCHMARK_CLEAR_CACHE: '1'
181183
run: |
182184
hatch env run --env "test.py3.13-minimal" run-benchmark
183185

‎changes/4199.bugfix.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
The end-to-end benchmarks no longer invoke `sudo` to drop the OS page cache during a regular `pytest` run. Cache clearing is now opt-in via the `ZARR_BENCHMARK_CLEAR_CACHE` environment variable, which the benchmark CI jobs set.

‎tests/benchmarks/test_e2e.py‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@
44

55
from __future__ import annotations
66

7+
import os
78
import platform
89
import subprocess
10+
import warnings
911
from functools import lru_cache
1012
from operator import getitem, setitem
1113
from typing import TYPE_CHECKING, Any, Literal
@@ -21,12 +23,27 @@
2123

2224

2325
def clear_cache() -> None:
26+
"""Drop the OS page cache between benchmark rounds.
27+
28+
Requires passwordless sudo, so it is opt-in: set `ZARR_BENCHMARK_CLEAR_CACHE=1`
29+
to enable it (as the benchmark CI jobs do). By default this is a no-op, so a
30+
plain `pytest` run never prompts for a sudo password (see issue #4199).
31+
`sudo -n` guarantees we fail instead of blocking on a password prompt even
32+
when the variable is set.
33+
"""
34+
if os.environ.get("ZARR_BENCHMARK_CLEAR_CACHE", "") not in ("1", "true"):
35+
return
2436
if platform.system() == "Darwin":
25-
subprocess.call(["sync", "&&", "sudo", "purge"])
37+
subprocess.call(["sync"])
38+
subprocess.call(["sudo", "-n", "purge"])
2639
elif platform.system() == "Linux":
27-
subprocess.call(["sudo", "sh", "-c", "sync; echo 3 > /proc/sys/vm/drop_caches"])
40+
subprocess.call(["sudo", "-n", "sh", "-c", "sync; echo 3 > /proc/sys/vm/drop_caches"])
2841
else:
29-
raise Exception("Unsupported platform") # noqa: TRY002
42+
warnings.warn(
43+
f"ZARR_BENCHMARK_CLEAR_CACHE is set but cache clearing is not supported on "
44+
f"{platform.system()}; skipping.",
45+
stacklevel=2,
46+
)
3047

3148

3249
if TYPE_CHECKING:

0 commit comments

Comments
 (0)