-
Notifications
You must be signed in to change notification settings - Fork 344
add agentic for query service subcommand #2572
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
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 |
|---|---|---|
|
|
@@ -92,3 +92,4 @@ class ServiceQueryRequest: | |
| query: str | ||
| retrieval: QueryRetrievalOptions = field(default_factory=QueryRetrievalOptions) | ||
| service: QueryServiceOptions = field(default_factory=QueryServiceOptions) | ||
| agentic: bool = False | ||
|
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 new user-facing Rule Used: User-facing configuration must use Pydantic models... (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/options.py
Line: 95
Comment:
**Validate the agentic request setting**
The new user-facing `agentic` setting is added to an unvalidated dataclass without descriptive field metadata, so unsupported option combinations are not rejected at request construction and instead fail later at the remote service boundary.
**Rule Used:** User-facing configuration must use Pydantic models... ([source](https://github.com/nvidia/nemo-retriever/blob/f41b14e714a1883d2d8640ff2c23991b7ed9ca6d/nemo_retriever/.greptile/config.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ def query_documents(request: ServiceQueryRequest) -> list[RetrievalHit]: | |
| base_url=request.service.service_url, | ||
| api_token=request.service.service_api_token, | ||
| ) | ||
| raw_result_sets = client.query(request.query, top_k=retrieval_top_k) | ||
| raw_result_sets = client.query(request.query, top_k=retrieval_top_k, agentic=request.agentic) | ||
|
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.
When an agentic service query has a valid final Knowledge Base Used: Prompt To Fix With AIThis is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/service.py
Line: 30
Comment:
**Candidate pool exceeds agentic depth**
When an agentic service query has a valid final `top_k` but a `candidate_k` above the configured `agentic.backend_top_k`, this call sends `candidate_k` as the endpoint's `top_k`, causing the service to reject the request with HTTP 422.
**Knowledge Base Used:**
- [Query workflow orchestration](https://app.greptile.com/nvidia-public-github/-/custom-context/knowledge-base/nvidia/nemo-retriever/-/docs/query-workflow-orchestration.md)
- [Retriever service](https://app.greptile.com/nvidia-public-github/-/custom-context/knowledge-base/nvidia/nemo-retriever/-/docs/retriever-service.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| raw_hits = raw_result_sets[0] | ||
| return shape_query_hits( | ||
| raw_hits, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -553,14 +553,15 @@ def query( | |
| *, | ||
| top_k: int, | ||
| collection_name: str | None = None, | ||
| agentic: bool = False, | ||
| ) -> list[list[dict[str, Any]]] | list[QueryHit]: | ||
| """Search ingested documents through ``POST /v1/query``. | ||
|
|
||
| Note: | ||
| ``top_k`` is required here but defaults to 10 on :meth:`aquery`. | ||
| That asymmetry is part of the released signature; do not unify it. | ||
| """ | ||
|
Comment on lines
+556
to
563
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 public Rule Used: Every public class and function in nemo_retriever ... (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/service/client.py
Line: 556-563
Comment:
**Document the agentic parameter**
The public `query()` and `aquery()` methods add `agentic` without documenting its behavior or single-query restriction, leaving library users unable to discover this changed request contract from the API documentation.
**Rule Used:** Every public class and function in nemo_retriever ... ([source](https://github.com/nvidia/nemo-retriever/blob/f41b14e714a1883d2d8640ff2c23991b7ed9ca6d/nemo_retriever/.greptile/config.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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! |
||
| return self._run(self.aquery(query, top_k=top_k, collection_name=collection_name)) | ||
| return self._run(self.aquery(query, top_k=top_k, collection_name=collection_name, agentic=agentic)) | ||
|
|
||
| def _query_hit(self, hit: dict[str, Any]) -> QueryHit: | ||
| return self._model( | ||
|
|
@@ -580,12 +581,15 @@ async def aquery( | |
| *, | ||
| top_k: int = 10, | ||
| collection_name: str | None = None, | ||
| agentic: bool = False, | ||
| ) -> list[list[dict[str, Any]]] | list[QueryHit]: | ||
| """Asynchronously search through ``POST /v1/query``.""" | ||
|
|
||
| payload: dict[str, Any] = {"query": query, "top_k": int(top_k)} | ||
| if collection_name: | ||
| payload["collection_name"] = collection_name | ||
| if agentic: | ||
|
Comment on lines
+584
to
+591
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.
When Prompt To Fix With AIThis is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/service/client.py
Line: 584-591
Comment:
**Reject batched agentic queries**
When `query()` or `aquery()` receives a list of query strings with `agentic=True`, the client serializes both values even though the service requires a single string for agentic mode, causing the request to fail service validation instead of returning results.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| payload["agentic"] = True | ||
| body = await self._arequest("POST", "/v1/query", json=payload) | ||
| try: | ||
| parsed = QueryResponse.model_validate(body).hits_by_query( | ||
|
|
||
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.
When
--agenticis combined with--format evidence, the command runs the service's agentic workflow but still passesstrategies=["semantic"]to output shaping, causingcoverage.strategies_usedto report incorrect provenance.Prompt To Fix With AI