Centralize shared utilities across benchmark backends#2040
Draft
jnke2016 wants to merge 16 commits intorapidsai:mainfrom
Draft
Centralize shared utilities across benchmark backends#2040jnke2016 wants to merge 16 commits intorapidsai:mainfrom
jnke2016 wants to merge 16 commits intorapidsai:mainfrom
Conversation
…ing across Python-native backends
…nfiguration, gather_algorithm_configs) from CppGBenchConfigLoader to base ConfigLoader class.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughIntroduces a backend utility function to load vectors from binary files with NumPy dtype inference based on file extension, and refactors shared YAML/dataset/algorithm configuration logic from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuvs_bench/cuvs_bench/backends/utils.py`:
- Around line 57-58: The code currently assigns dtype = _DTYPE_FOR_EXT.get(ext,
np.float32) which silently defaults unknown file extensions to float32; change
this to fail fast by looking up ext in _DTYPE_FOR_EXT and raising a clear
ValueError (including ext and path) when not found instead of defaulting. Update
the logic around ext and dtype (the ext variable and _DTYPE_FOR_EXT lookup) so
unknown suffixes raise an exception with a descriptive message to prevent
corrupted benchmark inputs.
- Around line 60-66: The code that reads binary matrices (reading n_rows/n_cols
and payload) must validate header and payload lengths and ensure subset_size is
non-negative and within bounds: check that f.read(4) calls returned 4 bytes
before converting to uint32 and raise a ValueError if not; validate subset_size
>= 0 and if provided clamp it to n_rows but error if it's > n_rows; compute
expected_bytes = n_rows * n_cols * np.dtype(dtype).itemsize and verify f.read
returns exactly that many bytes (or at least enough for the requested subset)
before calling np.frombuffer, raising a clear ValueError if sizes mismatch;
update the logic around n_rows, n_cols, subset_size, dtype, np.frombuffer and
f.read to use these checks so malformed files produce actionable errors rather
than cryptic crashes.
In `@python/cuvs_bench/cuvs_bench/orchestrator/config_loaders.py`:
- Around line 234-243: The code silently ignores invalid algorithm_configuration
values when they are neither files nor directories; update the block handling
algorithm_configuration (the variable algorithm_configuration that populates
algos_conf_fs) to detect the invalid path and raise a clear exception (e.g.,
raise ValueError) or log-and-raise with the provided path and its type so
callers know the override is invalid instead of silently falling back to
defaults; implement this in the same conditional after the
os.path.isdir/os.path.isfile checks where algos_conf_fs is modified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d1822e3-7adb-4f35-a054-0962f282eec1
📒 Files selected for processing (2)
python/cuvs_bench/cuvs_bench/backends/utils.pypython/cuvs_bench/cuvs_bench/orchestrator/config_loaders.py
Comment on lines
+57
to
+58
| ext = os.path.splitext(path)[1].lower() | ||
| dtype = _DTYPE_FOR_EXT.get(ext, np.float32) |
There was a problem hiding this comment.
Fail fast on unsupported vector file extensions.
Line 58 silently defaults unknown suffixes to float32, which can corrupt benchmark inputs when a filename is mistyped.
Proposed fix
def load_vectors(path: str, subset_size: Optional[int] = None) -> np.ndarray:
@@
ext = os.path.splitext(path)[1].lower()
- dtype = _DTYPE_FOR_EXT.get(ext, np.float32)
+ if ext not in _DTYPE_FOR_EXT:
+ raise ValueError(
+ f"Unsupported vector file extension '{ext}'. "
+ f"Expected one of: {', '.join(sorted(_DTYPE_FOR_EXT.keys()))}"
+ )
+ dtype = _DTYPE_FOR_EXT[ext]📝 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
| ext = os.path.splitext(path)[1].lower() | |
| dtype = _DTYPE_FOR_EXT.get(ext, np.float32) | |
| ext = os.path.splitext(path)[1].lower() | |
| if ext not in _DTYPE_FOR_EXT: | |
| raise ValueError( | |
| f"Unsupported vector file extension '{ext}'. " | |
| f"Expected one of: {', '.join(sorted(_DTYPE_FOR_EXT.keys()))}" | |
| ) | |
| dtype = _DTYPE_FOR_EXT[ext] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuvs_bench/cuvs_bench/backends/utils.py` around lines 57 - 58, The
code currently assigns dtype = _DTYPE_FOR_EXT.get(ext, np.float32) which
silently defaults unknown file extensions to float32; change this to fail fast
by looking up ext in _DTYPE_FOR_EXT and raising a clear ValueError (including
ext and path) when not found instead of defaulting. Update the logic around ext
and dtype (the ext variable and _DTYPE_FOR_EXT lookup) so unknown suffixes raise
an exception with a descriptive message to prevent corrupted benchmark inputs.
Comment on lines
+60
to
+66
| n_rows = int(np.frombuffer(f.read(4), dtype=np.uint32)[0]) | ||
| n_cols = int(np.frombuffer(f.read(4), dtype=np.uint32)[0]) | ||
| if subset_size is not None: | ||
| n_rows = min(n_rows, subset_size) | ||
| raw = f.read(n_rows * n_cols * np.dtype(dtype).itemsize) | ||
| data = np.frombuffer(raw, dtype=dtype) | ||
| return data.reshape(n_rows, n_cols) |
There was a problem hiding this comment.
Validate file header/payload size and subset_size bounds.
Lines 60-66 assume complete header/payload and accept negative subset_size, which leads to non-actionable errors on bad input.
Proposed fix
def load_vectors(path: str, subset_size: Optional[int] = None) -> np.ndarray:
@@
+ if subset_size is not None and subset_size < 0:
+ raise ValueError("subset_size must be >= 0")
+
with open(path, "rb") as f:
- n_rows = int(np.frombuffer(f.read(4), dtype=np.uint32)[0])
- n_cols = int(np.frombuffer(f.read(4), dtype=np.uint32)[0])
+ header = f.read(8)
+ if len(header) != 8:
+ raise ValueError(f"Invalid vector file header in '{path}'")
+ n_rows, n_cols = np.frombuffer(header, dtype=np.uint32, count=2)
+ n_rows, n_cols = int(n_rows), int(n_cols)
if subset_size is not None:
n_rows = min(n_rows, subset_size)
- raw = f.read(n_rows * n_cols * np.dtype(dtype).itemsize)
- data = np.frombuffer(raw, dtype=dtype)
+ expected = n_rows * n_cols * np.dtype(dtype).itemsize
+ raw = f.read(expected)
+ if len(raw) != expected:
+ raise ValueError(
+ f"Unexpected payload size in '{path}': expected {expected} bytes, got {len(raw)}"
+ )
+ data = np.frombuffer(raw, dtype=dtype, count=n_rows * n_cols)
return data.reshape(n_rows, n_cols)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuvs_bench/cuvs_bench/backends/utils.py` around lines 60 - 66, The
code that reads binary matrices (reading n_rows/n_cols and payload) must
validate header and payload lengths and ensure subset_size is non-negative and
within bounds: check that f.read(4) calls returned 4 bytes before converting to
uint32 and raise a ValueError if not; validate subset_size >= 0 and if provided
clamp it to n_rows but error if it's > n_rows; compute expected_bytes = n_rows *
n_cols * np.dtype(dtype).itemsize and verify f.read returns exactly that many
bytes (or at least enough for the requested subset) before calling
np.frombuffer, raising a clear ValueError if sizes mismatch; update the logic
around n_rows, n_cols, subset_size, dtype, np.frombuffer and f.read to use these
checks so malformed files produce actionable errors rather than cryptic crashes.
Comment on lines
+234
to
+243
| if algorithm_configuration: | ||
| if os.path.isdir(algorithm_configuration): | ||
| algos_conf_fs += [ | ||
| os.path.join(algorithm_configuration, f) | ||
| for f in os.listdir(algorithm_configuration) | ||
| if ".json" not in f | ||
| ] | ||
| elif os.path.isfile(algorithm_configuration): | ||
| algos_conf_fs.append(algorithm_configuration) | ||
| return algos_conf_fs |
There was a problem hiding this comment.
Do not silently ignore invalid algorithm_configuration paths.
Line 234 accepts an override input, but when it is neither a file nor a directory, the code currently falls back silently to defaults.
Proposed fix
if algorithm_configuration:
if os.path.isdir(algorithm_configuration):
algos_conf_fs += [
os.path.join(algorithm_configuration, f)
for f in os.listdir(algorithm_configuration)
if ".json" not in f
]
elif os.path.isfile(algorithm_configuration):
algos_conf_fs.append(algorithm_configuration)
+ else:
+ raise FileNotFoundError(
+ f"algorithm_configuration path does not exist: "
+ f"{algorithm_configuration}"
+ )
return algos_conf_fs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuvs_bench/cuvs_bench/orchestrator/config_loaders.py` around lines 234
- 243, The code silently ignores invalid algorithm_configuration values when
they are neither files nor directories; update the block handling
algorithm_configuration (the variable algorithm_configuration that populates
algos_conf_fs) to detect the invalid path and raise a clear exception (e.g.,
raise ValueError) or log-and-raise with the provided path and its type so
callers know the override is invalid instead of silently falling back to
defaults; implement this in the same conditional after the
os.path.isdir/os.path.isfile checks where algos_conf_fs is modified.
…idate header/payload size, warn on invalid algorithm config paths.
…tor loading from file paths on first access
…or load_vectors to use it.
…stry.py to match current BenchmarkBackend abstract interface
…ansparent vector loading, and base ConfigLoader inherited methods
…taset lookup, DatasetConfig construction) and delegate backend-specific logic to abstract _build_benchmark_configs().
…sts for Python-native backends.
… user-specified executable path capability.
…-n-trials, and --backend-config flags.
jrbourbeau
approved these changes
Apr 27, 2026
Member
jrbourbeau
left a comment
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks @jnke2016 for moving these to a shared place. Overall LGTM -- just left a couple minor (nonblocking) comments
| # it was passed in or loaded from disk. | ||
| self._base_vectors = base_vectors if base_vectors is not None else np.empty((0, 0)) | ||
| self._query_vectors = query_vectors if query_vectors is not None else np.empty((0, 0)) | ||
| self._groundtruth_neighbors = groundtruth_neighbors |
Member
There was a problem hiding this comment.
Why is _groundtruth_neighbors treated differently here than _base_vectors and _query_vectors?
Comment on lines
+341
to
+344
| if ".json" not in f | ||
| and "constraint" not in f | ||
| and ".py" not in f | ||
| and "__pycache__" not in f |
Member
There was a problem hiding this comment.
Are these supposed to be yaml files? If so, checking for that extension (.yaml + .yml) might be better. Maybe the situation is more complex though
…parently after backend.search() returns
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR centralizes duplicated logic identified during review of the OpenSearch and Elasticsearch backend PRs. Both backends independently reimplemented common operations (vector file loading, YAML parsing, dataset lookup, DatasetConfig construction, parameter expansion, recall computation) that should be shared across all backends. This PR moves that shared logic into the framework so that backend authors focus only on backend-specific concerns.
Motivation
With the pluggable backend architecture, each new Python-native backend needs to load binary vector files, parse dataset YAML configs, look up datasets by name, expand parameter combinations, and compute recall. Without centralized utilities, each backend reimplements these independently, leading to:
Changes
1. Shared vector loading utility (
backends/utils.py)Added
load_vectors()anddtype_from_filename()(originating fromgenerate_groundtruth/utils.py) for reading binary vector files. Supports all formats (.fbin,.f16bin,.u8bin,.i8bin,.ibin) withsubset_sizesupport and input validation (unsupported extensions, truncated headers/payloads). The C++ backend is unaffected since it passes file paths to the subprocess.2. Promoted config-loading methods to base
ConfigLoaderclassMoved
load_yaml_file,get_dataset_configuration, andgather_algorithm_configsfromCppGBenchConfigLoaderto the baseConfigLoaderclass. These were being reimplemented by both the OpenSearch and Elasticsearch config loaders. They are now inherited automatically. Also added a warning for invalidalgorithm_configurationpaths (previously silently ignored).3. Dataset refactored from dataclass to regular class with transparent vector loading
The
Datasetclass now transparently loads vectors from file paths on first access via properties. Python-native backends just accessdataset.base_vectorsand get a numpy array regardless of whether vectors were provided directly or loaded from disk. The C++ backend continues to usedataset.base_fileonly, never triggering any loading. This was done in response to reviewer feedback that backends should not need to know about file loading utilities.4. ConfigLoader template method pattern
ConfigLoader.load()is now a concrete method that handles the shared steps (loading dataset YAML, finding the dataset by name, constructingDatasetConfigwith path resolution viabuild_dataset_config()) then delegates to the abstract_build_benchmark_configs()for backend-specific logic. Backend authors implement_build_benchmark_configs()and receive theDatasetConfigalready built. They cannot skip or reimplement the shared steps. The orchestrator is unchanged.5. Shared parameter expansion and recall computation (
backends/utils.py)Added
expand_param_grid()for expanding YAML parameter specs (dict-of-lists to list-of-dicts via Cartesian product) andcompute_recall()for computing recall@k from neighbors and ground truth. Both are used by Python-native backends. The C++ backend does not use these (it handles parameter expansion interleaved with constraint validation, and computes recall in the subprocess).Remaining work
generate_groundtruth/utils.py,get_dataset/fbin_to_f16bin.py, andget_dataset/hdf5_to_fbin.pyinto a top-levelcuvs_bench/io.pymodule