Skip to content

[REVIEW] Add OpenSearch backend to cuvs-bench#2012

Open
jrbourbeau wants to merge 5 commits intorapidsai:mainfrom
jrbourbeau:cuvs-bench-opensearch2
Open

[REVIEW] Add OpenSearch backend to cuvs-bench#2012
jrbourbeau wants to merge 5 commits intorapidsai:mainfrom
jrbourbeau:cuvs-bench-opensearch2

Conversation

@jrbourbeau
Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau commented Apr 10, 2026

Would still like to refine a bit but pushing up for early feedback

cc @singhmanas1 @janakivamaraju @afourniernv

xref #1907 which does something similar but for Elastic

EDIT: Okay, this is now ready for review (cc @jnke2016)

Signed-off-by: James Bourbeau <jbourbeau@nvidia.com>
print(
f"Build failed for {config.index_name}: {build_result.error_message}"
try:
backend.initialize()
Copy link
Copy Markdown
Member Author

@jrbourbeau jrbourbeau Apr 10, 2026

Choose a reason for hiding this comment

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

Note to reviewers: This is a separate, but related change to the orchestrator. I noticed the initialize and cleanup methods weren't actually being called like their docstrings said they were suppose to. This is a big diff but most of it is just intending over the existing code into a try/except block. The functional change is calling initialize + cleanup

EDIT: FWIW supporting a context manager for backend setup / teardown seems reasonable. But that's also not something I wanted to handle in this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch. The initialize() and cleanup() hooks in BenchmarkBackend are defined in the abstract base class with docstrings describing their intended purpose, but neither method is actually called by the BenchmarkOrchestrator in _run_sweep() or _run_trial(). This was an oversight because the only backend implemented at the time was CppGoogleBenchmarkBackend, whose lifecycle is entirely self-contained within each subprocess call to build()/search(), making initialize()/cleanup() unnecessary for it.

Copy link
Copy Markdown
Contributor

@jnke2016 jnke2016 Apr 13, 2026

Choose a reason for hiding this comment

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

EDIT: FWIW supporting a context manager for backend setup / teardown seems reasonable. But that's also not something I wanted to handle in this PR.

I Agree that context manager support is reasonable. The purpose of the pluggable backend API is precisely to enable these kinds of extensions without modifying the core orchestrator. Since initialize() and cleanup() are already part of the BenchmarkBackend interface, wiring them up in the orchestrator (whether as explicit calls or a context manager) would ensure the plugin system works end-to-end for future backends without requiring further modifications to the core


[tool.pytest.ini_options]
markers = [
"integration: tests that require a live OpenSearch node (run with '-m integration')",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TODO: Make this more generic language (not just opensearch)

Signed-off-by: James Bourbeau <jbourbeau@nvidia.com>
…ch-opensearch

Signed-off-by: James Bourbeau <jbourbeau@nvidia.com>
Signed-off-by: James Bourbeau <jbourbeau@nvidia.com>
…ch-opensearch

Signed-off-by: James Bourbeau <jbourbeau@nvidia.com>
@jrbourbeau jrbourbeau changed the title [WIP] Add OpenSearch backend to cuvs-bench [REVIEW] Add OpenSearch backend to cuvs-bench Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants