-
Notifications
You must be signed in to change notification settings - Fork 339
Dev/mahikaw/agentic enhancements #2274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
55ff375
fe5d1fa
a3ae980
26d60d6
3520ab3
fed8ce0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -139,6 +139,11 @@ class AgenticRetrievalConfig: | |||||
| # Drives the ReAct target, the RRF/selection cut, and the per-hop fetch depth | ||||||
| # (which is raised to at least this). Defaults to 10. | ||||||
| top_k: int = AGENTIC_TARGET_TOP_K | ||||||
| # Per-hop retrieval shaping, forwarded to ``Retriever.query`` on every agent | ||||||
| # retrieval hop (mirrors the dense path's --candidate-k/--page-dedup/--content-types). | ||||||
| candidate_k: Optional[int] = None | ||||||
| page_dedup: bool = False | ||||||
| content_types: str | Sequence[str] | None = None | ||||||
|
|
||||||
| def __post_init__(self) -> None: | ||||||
| if self.llm_model is None or not str(self.llm_model).strip(): | ||||||
|
|
@@ -192,16 +197,22 @@ def __init__( | |||||
| }, | ||||||
| ) | ||||||
| self._lock = threading.Lock() | ||||||
| # Full hits keyed by doc_id, captured at the retrieval boundary (before the | ||||||
| # agent loop reduces them to doc_id/text/score) so the final output can | ||||||
| # re-hydrate the metadata the agent drops. Reset at the start of retrieve(). | ||||||
| self._hit_cache: dict[str, dict[str, Any]] = {} | ||||||
|
|
||||||
| def retrieve(self, query_ids: Sequence[str], query_texts: Sequence[str]) -> pd.DataFrame: | ||||||
| """Return selected ranked documents for each query. | ||||||
|
|
||||||
| The output schema matches ``SelectionAgentOperator``: ``query_id``, | ||||||
| ``doc_id``, ``rank``, and ``message``. | ||||||
| Columns: ``query_id``, ``doc_id``, ``rank``, ``message``, ``result_source``, | ||||||
| and ``hit`` — the full ``RetrievalHit`` (text/metadata/source/page/…) captured | ||||||
| at retrieval time and re-hydrated by ``doc_id`` (``{}`` when unavailable). | ||||||
| """ | ||||||
|
|
||||||
| if len(query_ids) != len(query_texts): | ||||||
| raise ValueError("query_ids and query_texts must have the same length.") | ||||||
| self._hit_cache.clear() | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/agentic.py
Line: 215
Comment:
**`_hit_cache.clear()` called without holding `self._lock`**
`self._hit_cache` is shared across concurrent `_retrieve_for_agent` workers (which hold `self._lock` when writing). If `retrieve()` is ever called on the same `AgenticRetriever` instance from two threads simultaneously, the `clear()` here races with `setdefault()` calls from workers of the first invocation. The current usage pattern (new instance per call in `agentic_query_documents`) is safe, but the absence of a lock on `clear()` leaves the class undocumented as "not safe for concurrent `retrieve()` calls" — a footgun for future users who hold onto an `AgenticRetriever` and call it concurrently.
How can I resolve this? If you propose a fix, please make it concise. |
||||||
|
|
||||||
| from nemo_retriever.operators.graph_ops.react_agent_operator import ReActAgentOperator | ||||||
| from nemo_retriever.operators.graph_ops.rrf_aggregator_operator import RRFAggregatorOperator | ||||||
|
|
@@ -254,7 +265,11 @@ def retrieve(self, query_ids: Sequence[str], query_texts: Sequence[str]) -> pd.D | |||||
| [str(query_text) for query_text in query_texts], | ||||||
| top_k=target_top_k, | ||||||
| ) | ||||||
| return _raw_hits_to_agentic_result([str(query_id) for query_id in query_ids], raw_hits) | ||||||
| result = _raw_hits_to_agentic_result([str(query_id) for query_id in query_ids], raw_hits) | ||||||
| # Re-hydrate the metadata dropped at the agent boundary by joining the | ||||||
| # selected doc_ids back to the full hits captured in _retrieve_for_agent. | ||||||
| result["hit"] = [self._hit_cache.get(str(doc_id), {}) for doc_id in result["doc_id"]] | ||||||
| return result | ||||||
|
|
||||||
| def _retrieve_for_agent(self, query_text: str, top_k: int) -> list[dict[str, Any]]: | ||||||
| """Retriever callback used by ``ReActAgentOperator``. | ||||||
|
|
@@ -266,8 +281,17 @@ def _retrieve_for_agent(self, query_text: str, top_k: int) -> list[dict[str, Any | |||||
| which still run concurrently under ``num_concurrent > 1``. | ||||||
| """ | ||||||
|
|
||||||
| # candidate_k must be >= the hop's top_k, which grows as the agent paginates, | ||||||
| # so floor it at top_k when the caller requested a wider pool. | ||||||
| candidate_k = max(int(self._cfg.candidate_k), int(top_k)) if self._cfg.candidate_k else None | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/agentic.py
Line: 286
Comment:
**`candidate_k=0` silently treated as `None`**
`if self._cfg.candidate_k else None` is a falsy check. `candidate_k=0` is falsy in Python, so it would be silently ignored and treated as `None` rather than triggering the floor calculation. An integer check is safer here and makes the intent explicit.
```suggestion
candidate_k = max(int(self._cfg.candidate_k), int(top_k)) if self._cfg.candidate_k is not None else None
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||
| with self._lock: | ||||||
| hits = self._retriever.query(str(query_text), top_k=int(top_k)) | ||||||
| hits = self._retriever.query( | ||||||
| str(query_text), | ||||||
| top_k=int(top_k), | ||||||
| candidate_k=candidate_k, | ||||||
| page_dedup=bool(self._cfg.page_dedup), | ||||||
| content_types=self._cfg.content_types, | ||||||
| ) | ||||||
|
|
||||||
| docs: list[dict[str, Any]] = [] | ||||||
| doc_id_field = getattr(self, "_doc_id_field", None) | ||||||
|
|
@@ -280,6 +304,10 @@ def _retrieve_for_agent(self, query_text: str, top_k: int) -> list[dict[str, Any | |||||
| ) | ||||||
| if not doc_id: | ||||||
| continue | ||||||
| # Capture the full hit (minus the embedding) for output re-hydration; | ||||||
| # first occurrence per doc_id wins. Locked: shared across ReAct workers. | ||||||
| with self._lock: | ||||||
| self._hit_cache.setdefault(doc_id, {k: v for k, v in hit_dict.items() if k != "vector"}) | ||||||
| text = str(hit_dict.get("text", "")) | ||||||
| if int(self._cfg.text_truncation) > 0: | ||||||
| text = text[: int(self._cfg.text_truncation)] | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,17 +144,16 @@ def query_documents(request: QueryRequest) -> list[RetrievalHit]: | |
|
|
||
|
|
||
| def agentic_query_documents(request: QueryRequest) -> list[dict[str, Any]]: | ||
| """Run agentic (ReAct) retrieval for a single query and return the agent's | ||
| ranked document IDs. | ||
|
|
||
| Unlike the dense ``query_documents`` path (which returns enriched hits with | ||
| text), the agent operates at the document-ID granularity of the configured | ||
| index, so the result is the ranked ``doc_id`` list the agent selected, | ||
| annotated with the source that produced it (``final_results`` / ``rrf`` / | ||
| ``selection_agent``). The LanceDB ``uri``/``table_name``, embedding config, | ||
| and (when ``--rerank`` is enabled) reranker config are passed straight | ||
| through to the wrapped ``Retriever`` that backs the agent's ``retrieve`` | ||
| tool. Reranking therefore applies per agent retrieval hop. | ||
| """Run agentic (ReAct) retrieval for a single query and return ranked hits. | ||
|
|
||
| Each result carries the full ``RetrievalHit`` metadata (``text``, ``source``, | ||
| ``page_number``, ``pdf_page``, ``metadata``, …) — re-hydrated by ``doc_id`` from | ||
| the hits captured at retrieval time — plus the agentic annotations ``doc_id``, | ||
| ``rank``, and ``result_source`` (``final_results`` / ``rrf`` / ``selection_agent``). | ||
| This matches the metadata the dense ``query_documents`` path returns. The LanceDB | ||
| ``uri``/``table_name``, embedding config, and (when ``--rerank`` is enabled) | ||
| reranker config are passed through to the wrapped ``Retriever`` that backs the | ||
| agent's ``retrieve`` tool; reranking applies per agent retrieval hop. | ||
| """ | ||
| from nemo_retriever.query.agentic import AgenticRetrievalConfig, AgenticRetriever | ||
|
|
||
|
|
@@ -166,6 +165,9 @@ def agentic_query_documents(request: QueryRequest) -> list[dict[str, Any]]: | |
| "vdb_op": "lancedb", | ||
| "vdb_kwargs": vdb_kwargs, | ||
| "top_k": int(request.retrieval.top_k), | ||
| "candidate_k": request.retrieval.candidate_k, | ||
| "page_dedup": bool(request.retrieval.page_dedup), | ||
| "content_types": request.retrieval.content_types, | ||
|
Comment on lines
+168
to
+170
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The non-agentic path in Prompt To Fix With AIThis is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/workflow.py
Line: 168-170
Comment:
**`content_types` sequence not normalized for agentic path**
The non-agentic path in `resolve_query_plan()` explicitly converts a `Sequence[str]` to a comma-joined string before passing it to `Retriever.query`. The agentic path passes `request.retrieval.content_types` raw to `AgenticRetrievalConfig`, which then forwards it directly to `self._retriever.query(content_types=...)`. If a caller passes `content_types=["text", "image"]` programmatically, the agentic path will hand a `list` to `Retriever.query` while the dense path would produce `"text,image"`, meaning the same option behaves differently across the two paths and may fail at runtime in the agentic case.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| "embedding_endpoint": request.embed.embed_invoke_url, | ||
| "embedding_api_key": api_key or "", | ||
| "llm_model": request.agentic.llm_model, | ||
|
|
@@ -193,13 +195,16 @@ def agentic_query_documents(request: QueryRequest) -> list[dict[str, Any]]: | |
| result = result.sort_values("rank") | ||
| ranked: list[dict[str, Any]] = [] | ||
| for _, row in result.iterrows(): | ||
| ranked.append( | ||
| hit = row.get("hit") if "hit" in result.columns else None | ||
| enriched: dict[str, Any] = dict(hit) if isinstance(hit, dict) else {} | ||
| enriched.update( | ||
| { | ||
| "rank": int(row.get("rank", len(ranked) + 1)), | ||
| "doc_id": str(row.get("doc_id", "")), | ||
| "rank": int(row.get("rank", len(ranked) + 1)), | ||
| "result_source": str(row.get("result_source", "")), | ||
| } | ||
| ) | ||
| ranked.append(enriched) | ||
| if len(ranked) >= request.retrieval.top_k: | ||
| break | ||
| return ranked | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging.basicConfig(force=True)without explicitstreamparameterforce=Truereplaces all existing handlers on the root logger. When this CLI runs under Typer'sCliRunner(which defaults tomix_stderr=True), log lines emitted to stderr appear inresult.outputand break anyjson.loads(result.output)assertions. Passingstream=sys.stderrexplicitly makes the intent clear and is safer ifsys.stderris swapped by the test runner.Prompt To Fix With AI