Support per-percentage ground truth datasets via filter_percentage param

[Likhoram]

Description

Adds optional key_suffix to HDF5 dataset reads. When filter_percentage is set (e.g. "10pct"), the neighbors and radial threshold datasets are read from suffixed keys (neighbors_10pct, faiss_max_distance_10pct), allowing one dataset file to hold precomputed answer sets for multiple filter percentages.

Testing

• Added a unit test covering suffixed key resolution (neighbors_10pct,faiss_max_distance_10pct) and unchanged unsuffixed reads
• Ran the full filtered radial curve (18 configurations) against a live cluster; exact-search runs return recall 1.0

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 33653c4)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 33653c4

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Close HDF5 file before raising error The h5py file handle is not closed on the error path, leaking a file descriptor when the key is missing. Open the file with a context manager or explicitly close it before raising ConfigurationError. osbenchmark/utils/dataset.py [111-117] def _load(self): if self.data is None: file = h5py.File(self.dataset_path) if self.context not in file: + available = sorted(file.keys()) + file.close() raise ConfigurationError( f"Dataset key '{self.context}' does not exist in {self.dataset_path}. " - f"Available keys: {sorted(file.keys())}. If filter_percentage is set, " + f"Available keys: {available}. If filter_percentage is set, " f"ensure the dataset was generated with that percentage.") self.data = cast(h5py.Dataset, file[self.context]) Suggestion importance[1-10]: 4 __ Why: Valid observation about a potential file descriptor leak on the error path, though the impact is minor since raising ConfigurationError in tests/CI would typically terminate execution and the OS would reclaim the descriptor.	Low

---

Previous suggestions

Suggestions up to commit 4f4781c

Category	Suggestion	Impact
General	Sanitize key suffix before concatenation Sanitize key_suffix before concatenating to avoid accidental double-underscores or leading separators when callers pass a value like "10pct" or "10pct". Stripping surrounding underscores/whitespace ensures the resulting HDF5 key exactly matches the expected {context}_{suffix} format and prevents silent key-not-found failures. osbenchmark/utils/dataset.py [101-105] def __init__(self, dataset_path: str, context: Context, key_suffix: str = None): self.dataset_path = dataset_path self.context = self.parse_context(context) if key_suffix: - self.context = f"{self.context}_{key_suffix}" + self.context = f"{self.context}_{key_suffix.strip().strip('_')}" Suggestion importance[1-10]: 3 __ Why: Minor defensive improvement; callers control the key_suffix value and the current test uses a clean suffix. It's a nice-to-have but not critical.	Low

Suggestions up to commit 9127cb4

Category	Suggestion	Impact
General	Validate suffixed HDF5 key exists Appending the suffix unconditionally when key_suffix is provided will break for contexts whose parsed key already needs special handling, and silently produces missing keys later during _load. Validate that the resulting suffixed key exists in the HDF5 file (or at least document/handle the KeyError) to fail fast with a clear error instead of a cryptic h5py lookup failure. osbenchmark/utils/dataset.py [101-105] def __init__(self, dataset_path: str, context: Context, key_suffix: str = None): self.dataset_path = dataset_path self.context = self.parse_context(context) if key_suffix: self.context = f"{self.context}_{key_suffix}" + self.current = self.BEGINNING + self.data = None Suggestion importance[1-10]: 2 __ Why: The suggestion asks to validate the suffixed key, but the improved_code is essentially identical to the existing_code (just adds the two existing subsequent lines). No actual validation is added, making the suggestion ineffective.	Low

Suggestions up to commit aff487b

Category	Suggestion	Impact
Possible issue	Guard against invalid suffixed HDF5 keys Appending the suffix unconditionally can produce invalid HDF5 keys for radial threshold contexts (e.g., max_distance_neighbors becoming max_distance_neighbors_10pct), which likely do not exist in per-percentage datasets. Validate that the resulting key exists in the HDF5 file, or restrict suffixing to the neighbors context only, to avoid runtime KeyError when opening the dataset. osbenchmark/utils/dataset.py [101-105] def __init__(self, dataset_path: str, context: Context, key_suffix: str = None): self.dataset_path = dataset_path self.context = self.parse_context(context) if key_suffix: self.context = f"{self.context}_{key_suffix}" + # Caller is responsible for ensuring the suffixed key exists in the HDF5 file. Suggestion importance[1-10]: 4 __ Why: The concern about suffix being applied to radial threshold contexts is valid, since the PR also passes filter_percentage to threshold datasets. However, the improved_code merely adds a comment without any actual validation or guard, providing minimal real improvement.	Low

[Copilot]

Pull request overview

This PR extends dataset loading to support per-percentage (“10pct”, etc.) ground-truth datasets stored in a single HDF5 file by suffixing the dataset key names when filter_percentage is provided.

Changes:

• Add filter_percentage workload param and pass it through when opening neighbors/threshold datasets.
• Extend get_data_set() / HDF5DataSet to accept an optional HDF5 key suffix and append it to the resolved context key.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
osbenchmark/workload/params.py	Adds filter_percentage param and threads it into neighbor/threshold dataset creation.
osbenchmark/utils/dataset.py	Adds optional key_suffix support for HDF5 dataset key selection via context_<suffix>.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Persistent review updated to latest commit 9127cb4

[kotwanikunal]

Why do we still have references of radial_engine? I thought you removed the redundant param?

[Likhoram]

That's from the old base, the branch predates #1099 where radial engine was removed. The diff showed pre-removal code as context. Rebased onto latest main, the old param is gone now.

[kotwanikunal]

Can we add error handling here? Check if the key exists? If not fail the workload?

[github-actions]

Persistent review updated to latest commit 4f4781c

[github-actions]

Persistent review updated to latest commit 33653c4