feat: add retrieval eval - #418
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a retrieval benchmark evaluation feature to the dingo-python package, adding a new RetrievalExecutor to evaluate search APIs against MTEB benchmarks, an abstract SearchClient interface with an agentic search backend, CLI and SDK support, and comprehensive unit tests. The code review feedback focuses on critical improvements for concurrency, robustness, and performance. Specifically, the reviewer recommends making the rate limiter thread-safe using a threading lock, wrapping search client calls and MTEB evaluations in try-except blocks to prevent crashes, avoiding inefficient dictionary conversions in the search loop, and refining configuration defaults (such as allowing api_url to be None and ensuring doc_id is stored as a string) to prevent runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| self.sub_queries = int(sub_queries) if sub_queries is not None else None | ||
| self._last_request_time = 0.0 |
There was a problem hiding this comment.
Initialize a threading lock to ensure thread-safe access to the shared rate limiter state.
| self.sub_queries = int(sub_queries) if sub_queries is not None else None | |
| self._last_request_time = 0.0 | |
| self.sub_queries = int(sub_queries) if sub_queries is not None else None | |
| self._last_request_time = 0.0 | |
| self._lock = threading.Lock() |
| for row in corpus: | ||
| doc_id = row["id"] | ||
| self._corpus_ids.add(doc_id) |
There was a problem hiding this comment.
Ensure that doc_id is always stored as a string. If the corpus contains integer IDs, matching them against string candidates in resolve_hit will fail because Python treats "123" == 123 as False.
| for row in corpus: | |
| doc_id = row["id"] | |
| self._corpus_ids.add(doc_id) | |
| for row in corpus: | |
| doc_id = str(row["id"]) | |
| self._corpus_ids.add(doc_id) |
| resolved_id, src = resolve_hit( | ||
| hit, dict(self._title_to_ids), self._corpus_ids | ||
| ) |
There was a problem hiding this comment.
Converting self._title_to_ids (which contains the entire corpus) to a standard dict on every single hit inside the loop is extremely inefficient and will cause massive CPU and memory overhead. Since defaultdict supports .get(), pass it directly.
| resolved_id, src = resolve_hit( | |
| hit, dict(self._title_to_ids), self._corpus_ids | |
| ) | |
| resolved_id, src = resolve_hit( | |
| hit, self._title_to_ids, self._corpus_ids | |
| ) |
| class RetrievalArgs(BaseModel): | ||
| backend: str = "agentic" | ||
| api_url: str = "" | ||
| api_token: Optional[str] = None |
There was a problem hiding this comment.
By defaulting api_url to an empty string "", any configuration that does not explicitly provide api_url will pass "" to the search client, overriding the client's internal default URL (e.g., "http://127.0.0.1:8080"). Changing api_url to Optional[str] = None allows the client to fall back to its default URL when no custom URL is configured.
| class RetrievalArgs(BaseModel): | |
| backend: str = "agentic" | |
| api_url: str = "" | |
| api_token: Optional[str] = None | |
| class RetrievalArgs(BaseModel): | |
| backend: str = "agentic" | |
| api_url: Optional[str] = None | |
| api_token: Optional[str] = None |
| from __future__ import annotations | ||
| import logging | ||
| import os | ||
| import time | ||
| from typing import Any |
There was a problem hiding this comment.
Import the threading module to support thread-safe rate limiting across concurrent evaluation threads.
| from __future__ import annotations | |
| import logging | |
| import os | |
| import time | |
| from typing import Any | |
| from __future__ import annotations | |
| import logging | |
| import os | |
| import threading | |
| import time | |
| from typing import Any |
| def __init__( | ||
| self, | ||
| api_url: str = "http://127.0.0.1:8080", | ||
| timeout: float = 30.0, | ||
| max_retries: int = 3, | ||
| retry_backoff: float = 0.5, | ||
| rate_limit: float = 0.0, | ||
| retrieval_mode: str = "hybrid", | ||
| sub_queries: int | None = None, | ||
| api_token: str | None = None, | ||
| **_kwargs: Any, | ||
| ) -> None: | ||
| self.base_url = api_url.rstrip("/") |
There was a problem hiding this comment.
Change api_url to accept None and safely fall back to the default URL if it is None or empty. This prevents AttributeError or invalid URL schema errors when api_url is not provided in the configuration.
| def __init__( | |
| self, | |
| api_url: str = "http://127.0.0.1:8080", | |
| timeout: float = 30.0, | |
| max_retries: int = 3, | |
| retry_backoff: float = 0.5, | |
| rate_limit: float = 0.0, | |
| retrieval_mode: str = "hybrid", | |
| sub_queries: int | None = None, | |
| api_token: str | None = None, | |
| **_kwargs: Any, | |
| ) -> None: | |
| self.base_url = api_url.rstrip("/") | |
| def __init__( | |
| self, | |
| api_url: str | None = None, | |
| timeout: float = 30.0, | |
| max_retries: int = 3, | |
| retry_backoff: float = 0.5, | |
| rate_limit: float = 0.0, | |
| retrieval_mode: str = "hybrid", | |
| sub_queries: int | None = None, | |
| api_token: str | None = None, | |
| **_kwargs: Any, | |
| ) -> None: | |
| self.base_url = (api_url or "http://127.0.0.1:8080").rstrip("/") |
| results = mteb.evaluate( | ||
| model, | ||
| tasks=tasks, | ||
| overwrite_strategy="always", | ||
| ) | ||
|
|
||
| task_metrics = self._extract_metrics(results) | ||
| all_results[task_name] = task_metrics |
There was a problem hiding this comment.
Wrap mteb.evaluate in a try-except block. When evaluating multiple tasks (e.g., "SciFact,SCIDOCS"), a failure in one task should not crash the entire executor and discard the results of other successful tasks.
| results = mteb.evaluate( | |
| model, | |
| tasks=tasks, | |
| overwrite_strategy="always", | |
| ) | |
| task_metrics = self._extract_metrics(results) | |
| all_results[task_name] = task_metrics | |
| try: | |
| results = mteb.evaluate( | |
| model, | |
| tasks=tasks, | |
| overwrite_strategy="always", | |
| ) | |
| task_metrics = self._extract_metrics(results) | |
| all_results[task_name] = task_metrics | |
| except Exception as e: | |
| logger.error(f"Failed to evaluate task {task_name}: {e}", exc_info=True) |
No description provided.