feat(knowledge): add OpenSearch as a first-class knowledge backend#228
Conversation
Greptile SummaryThis PR adds OpenSearch as a first-class knowledge backend alongside the existing LlamaIndex and Foundational RAG options, implementing
Confidence Score: 5/5Safe to merge. All five issues raised in the previous review round are resolved and the new code introduces no correctness regressions or data-safety concerns. The adapter core paths are implemented correctly and backed by a 1100+ line unit test suite. Remaining findings are efficiency and consistency suggestions with no impact on correctness. The _collection_info_from_index method in adapter.py and validate_backend_config in register.py are worth a second look before the feature sees heavy multi-tenant load. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as API Client
participant Ingestor as OpenSearchIngestor
participant Dask as Dask Worker
participant OS as OpenSearch / AOSS
Client->>Ingestor: submit_job(file_paths, collection)
Ingestor->>Ingestor: _ensure_index (race-safe)
alt Local mode
Ingestor->>Ingestor: _start_local_ingestion (thread)
Ingestor-->>Client: job_id (immediate)
Ingestor->>OS: bulk index chunks
else Dask mode
Ingestor->>Dask: client.submit(run_opensearch_ingestion_task)
Ingestor-->>Client: job_id (immediate)
Ingestor->>Ingestor: _monitor_dask_ingestion (thread)
Dask->>Dask: reconstruct OpenSearchIngestor
Dask->>OS: bulk index chunks (SigV4 resolved in worker)
Dask-->>Ingestor: result dict
Ingestor->>Ingestor: _apply_dask_ingestion_result
end
Client->>Ingestor: retrieve(query, collection)
Ingestor->>OS: knn_vector search + filter
OS-->>Ingestor: hits
Ingestor-->>Client: RetrievalResult (Chunks)
loop TTL cleanup (every 1h)
Ingestor->>OS: list_collections
OS-->>Ingestor: collections + updated_at
Ingestor->>OS: delete_collection (expired indexes)
end
Reviews (8): Last reviewed commit: "fix(opensearch): stop indexing internal ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Thank you @rhossi , we will review this soon |
source_path was written into every chunk's metadata and propagated by normalize() into every Chunk surfaced to API consumers and LLM context. For byte-upload paths (Dask mode, upload_file) this leaked temp paths such as /tmp/tmpXXXXXX.pdf. The field was write-only — no production code or test read it — so drop it from the indexed document. Regression test added asserting no indexed chunk and no retrieved Chunk metadata carries source_path. Flagged by Greptile (P1) on PR NVIDIA-AI-Blueprints#228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@KyleZheng1284 can you review this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an OpenSearch-backed knowledge layer, wires it into configs, docs, tests, and Helm, and adds Amazon OpenSearch Serverless deployment guidance plus live integration coverage. ChangesOpenSearch knowledge backend
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Config as configs/config_web_opensearch.yml
participant Register as sources/knowledge_layer/src/register.py
participant Backend as knowledge_layer.opensearch
participant Adapter as sources/knowledge_layer/src/opensearch/adapter.py
participant Dask as sources/knowledge_layer/src/opensearch/distributed.py
participant OpenSearch as OpenSearch / AOSS
Config->>Register: backend config fields and workflow config
Register->>Adapter: build OpenSearch adapter config
Register->>Backend: select opensearch backend
Backend->>Dask: run_opensearch_ingestion_task()
Dask->>OpenSearch: ensure index and bulk index chunks
Adapter->>OpenSearch: kNN search and document deletion
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
source_path was written into every chunk's metadata and propagated by normalize() into every Chunk surfaced to API consumers and LLM context. For byte-upload paths (Dask mode, upload_file) this leaked temp paths such as /tmp/tmpXXXXXX.pdf. The field was write-only — no production code or test read it — so drop it from the indexed document. Regression test added asserting no indexed chunk and no retrieved Chunk metadata carries source_path. Flagged by Greptile (P1) on PR NVIDIA-AI-Blueprints#228. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
rhossi
force-pushed
the
feat/opensearch-aoss
branch
from
21cdbee to
14b05f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/deployment/aws-opensearch-serverless.md`:
- Around line 424-433: The bash script unsets credential environment variables
and runs aws sso login but does not verify that the SSO login succeeded or that
credentials are properly active. Add a verification step immediately after the
aws sso login --profile cs-admin command by inserting aws sts
get-caller-identity --profile cs-admin to confirm the credentials are active and
the SSO login was successful before proceeding with the live tests.
In `@docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md`:
- Line 25: Both plan files violate markdown heading hierarchy (MD001) by jumping
from H1 directly to H3, skipping H2. In
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md at line
25 and docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md at line
29, change all headings that start with `### Task` to `## Task` to maintain
proper heading hierarchy. Ensure the document structure follows H1 (title) → H2
(tasks) → content hierarchy, which is required for Sphinx with MyST parser to
build successfully with the `-W` flag.
In `@docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md`:
- Line 269: Remove or generalize the hardcoded personal file paths embedded in
the grep shell commands in the document. Replace the absolute paths like
`/Users/fdecarvalhop/Documents/projects/aiq/.venv/lib/python3.13/site-packages/aiq_api`
with either a removal of the fallback path (keeping only the `src/` directory
search) or with a generic pattern using `python -m site --user-site` to
dynamically locate the site-packages directory. Apply this fix consistently
across all instances where similar hardcoded paths appear in grep commands to
ensure the documentation is portable and works for other engineers.
In `@sources/knowledge_layer/pyproject.toml`:
- Around line 55-62: The opensearch extra in pyproject.toml is missing the
distributed dependency, but OpenSearchIngestor._get_dask_client() in adapter.py
imports from the distributed package when ingestion_mode equals "dask". Add
"distributed" to the list of dependencies in the opensearch extra bracket along
with the other packages like opensearch-py, boto3, and openai to ensure proper
dependency resolution and prevent ImportError at runtime.
In `@sources/knowledge_layer/src/opensearch/adapter.py`:
- Around line 934-944: The code is using file_name as the primary key for
deletion and listing operations instead of file_id, which allows multiple
logical files with the same filename to be unintentionally merged or deleted
together. Refactor the OpenSearch adapter to use file_id as the primary identity
for all delete, list, and status operations: In
sources/knowledge_layer/src/opensearch/adapter.py lines 934-944, update the
query in the deletion logic to rely solely on file_id as the matching criterion
instead of also checking file_name and treating it as an identity field. In
lines 1042-1045 (the aggregation/grouping logic in list_files), change the
aggregation key from file_name to file_id to ensure each unique file is treated
separately. In lines 1349-1363 (likely another method using file_name as a key),
similarly update to use file_id as the primary key. Keep file_name as display
metadata only—it can be returned in response payloads but should never be used
to identify or filter files for deletion, listing, or status checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 34d48fcc-f525-4f4a-b45d-226573ea38d5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
configs/config_web_opensearch.ymldeploy/helm/README.mddeploy/helm/examples/aws-opensearch-serverless-values.yamldocs/source/deployment/aws-opensearch-serverless.mddocs/source/deployment/index.mddocs/source/index.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mdpyproject.tomlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/README.mdsources/knowledge_layer/pyproject.tomlsources/knowledge_layer/src/__init__.pysources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/opensearch/adapter.pysources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/run_adapter_compliance.pytests/knowledge_layer_tests/test_opensearch_adapter.pytests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_serverless_live.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/deployment/index.mddocs/source/index.mddocs/source/deployment/aws-opensearch-serverless.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
**
⚙️ CodeRabbit configuration file
**: # Contributing GuidelinesWe welcome contributions to the NVIDIA AI-Q blueprint. This repository uses a maintainer-reviewed pull request workflow with DCO sign-off, code-owner review, copy-pr-bot mirroring, and GitHub Actions validation.
Before You Start
- Search existing issues and pull requests before opening new work.
- Open an issue or discussion before large design changes, public APIs, deployment changes, or contributor workflow changes.
- Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
- Target the
developbranch unless a maintainer asks you to use a release branch.Pull Requests
- Fork the repository and create a focused branch from
develop.- Make the smallest coherent change and add or update tests for behavior changes.
- Sign off every commit with
git commit -s.- Run the relevant local validation before opening the PR.
- Open a pull request into
developand fill out the PR template with exact validation evidence.- Address review feedback until required checks and code-owner review pass.
Local Validation
Use the narrowest command that covers your change, then include the exact output or workflow link in the PR.
uv sync --group dev uv run ruff check . uv run ruff format --check . uv run pytestFor UI changes:
cd frontends/ui npm ci npm run lint npm run type-check npm run test:ci npm run buildFor deployment changes, run the relevant Helm or compose validation and describe the environment used.
CI and Bot Workflow
AI-Q uses push-triggered GitHub Actions. Pull requests are mirrored by copy-pr-bot to
pull-request/<PR number>branches after a maintainer or configured vetter comments/ok to test, and CI runs on those mirrored branches.Repository owners, organization members, and collaborators can request NVSkills validation by commenting:
/nvskills-ciMaintainers can request bot-driven merge with:
``...
Files:
docs/source/deployment/index.mdsources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/__init__.pydocs/source/index.mdsources/knowledge_layer/pyproject.tomldeploy/helm/README.mddeploy/helm/examples/aws-opensearch-serverless-values.yamlpyproject.tomlsources/knowledge_layer/README.mdtests/knowledge_layer_tests/run_adapter_compliance.pyconfigs/config_web_opensearch.ymltests/knowledge_layer_tests/test_opensearch_live.pydocs/source/deployment/aws-opensearch-serverless.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mdsources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/register.pytests/knowledge_layer_tests/test_opensearch_serverless_live.pydocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mdtests/knowledge_layer_tests/test_opensearch_adapter.pysources/knowledge_layer/src/opensearch/adapter.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
Files:
sources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/__init__.pytests/knowledge_layer_tests/run_adapter_compliance.pytests/knowledge_layer_tests/test_opensearch_live.pysources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/test_opensearch_serverless_live.pytests/knowledge_layer_tests/test_opensearch_adapter.pysources/knowledge_layer/src/opensearch/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/__init__.pysources/knowledge_layer/pyproject.tomlsources/knowledge_layer/README.mdsources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/register.pysources/knowledge_layer/src/opensearch/adapter.py
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
deploy/helm/README.mddeploy/helm/examples/aws-opensearch-serverless-values.yamlconfigs/config_web_opensearch.yml
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}
⚙️ CodeRabbit configuration file
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}: Review automation and packaging changes for least-privilege permissions, pinned versions where appropriate,
copy-pr-bot pull-request/ branch behavior, reproducible uv/npm setup, secret handling, and consistency with
the documented validation matrix.
Files:
pyproject.toml
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_serverless_live.pytests/knowledge_layer_tests/test_opensearch_adapter.py
🪛 ast-grep (0.43.0)
sources/knowledge_layer/src/register.py
[info] 275-275: Do not hardcode temporary file or directory names
Context: "/tmp/chroma_data"
Note: [CWE-377].
(hardcoded-tmp-file)
[info] 283-283: Do not hardcode temporary file or directory names
Context: "/tmp/chroma_data"
Note: [CWE-377].
(hardcoded-tmp-file)
tests/knowledge_layer_tests/test_opensearch_adapter.py
[warning] 892-892: Do not make http calls without encryption
Context: "http://nim-embed.ns-nim.svc.cluster.local:8000/v1"
Note: [CWE-319].
(requests-http)
🪛 LanguageTool
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[style] ~335-~335: To elevate your writing, try using a synonym here.
Context: ... "did the worker actually do anything?" hard to confirm. A doc note saves the next p...
(HARD_TO)
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
[style] ~216-~216: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...verless, use SigV4 with service aoss. For Amazon OpenSearch Service domains, use ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[style] ~18-~18: Consider an alternative to strengthen your wording.
Context: ...lready cross-links the example file (no further changes here). - `docs/source/deployment/index....
(CHANGES_ADJUSTMENTS)
[style] ~19-~19: Consider an alternative to strengthen your wording.
Context: ...lready lists the new doc on line 30 (no further changes here). - `configs/config_web_opensearch...
(CHANGES_ADJUSTMENTS)
[style] ~20-~20: Consider an alternative to strengthen your wording.
Context: ...ig, already env-substitution-driven (no further changes here). - `sources/knowledge_layer/KNOWL...
(CHANGES_ADJUSTMENTS)
🪛 markdownlint-cli2 (0.22.1)
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[warning] 29-29: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 166-166: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 167-167: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 167-167: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 241-241: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 242-242: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 242-242: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
[warning] 186-186: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[warning] 25-25: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 97-97: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 158-158: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 238-238: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 328-328: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 329-329: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 423-423: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 423-423: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 508-508: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 508-508: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 587-587: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 587-587: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 678-678: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 678-678: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 737-737: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 738-738: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 738-738: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (20)
deploy/helm/README.md (1)
134-172: ⚡ Quick winCross-reference the example values image pull secret name.
The README snippet on line 166 refers to
examples/aws-opensearch-serverless-values.yaml, and the example file setsimagePullSecrets[0].name=ngc-image-pull-secret(line 18 of the example file). However, earlier in the README (lines 84–86), the standard installation pattern uses a different reference. Ensure the comment on line 166 clarifies that users must update the pull secret name in the example values to match their actual secret, or create a secret namedngc-image-pull-secretas shown in the example.Suggested addition to line 167 (after the link):
Update the
imagePullSecretsname in this file to match the actual secret you created earlier (or usengc-image-pull-secretif you follow the example).docs/source/deployment/aws-opensearch-serverless.md (3)
262-272: ⚡ Quick winDocument text-only ingestion limitation in workflow config.
Lines 254–257 note that the OpenSearch backend extracts plain text only and does not honor
AIQ_EXTRACT_TABLES,AIQ_EXTRACT_IMAGES, orAIQ_EXTRACT_CHARTS. However, the workflow config example on lines 259–272 does not include a comment reiterating this constraint. Consider adding a YAML comment to line 262 (after the backend declaration) to warn users:functions: knowledge_search: _type: knowledge_retrieval backend: opensearch # Note: text-only extraction (no tables/images/charts)
349-417: ⚡ Quick winClarify timing and retry logic for AOSS visibility delay.
Line 402–407 documents that AOSS has a 5–30 second eventual-consistency delay before search reflects written documents. However, the verification step on lines 374–380 does not include retry or wait logic. The guidance says "Poll
GET /v1/documents/{job_id}/statusuntilstatusisSUCCESS" but does not mention that a successful status may not be immediately searchable.Consider either:
- Adding a note after line 381 that
SUCCESSstatus does not guarantee immediate search visibility, or- Including a short polling loop in the curl example to wait for document count > 0.
1-18: The migration note is accurate and intentionally positioned. The plan documentdocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md(Task 1, lines 25–62) explicitly instructs inserting this exact migration note to guide v1.0 users. The v2.0 CHANGELOG confirms the architectural shift to YAML-driven configuration, and./deploy.shis absent from the current codebase, confirming it was v1.0-only. No changes needed.sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md (5)
216-217: 💤 Low valueFix repetitive sentence openings in the SigV4 guidance.
Lines 216–217 contain two consecutive sentences both beginning with "For": "For Amazon OpenSearch Serverless, use SigV4 with service
aoss. For Amazon OpenSearch Service domains, use servicees." Consider rewording for clarity:Use SigV4 with service `aoss` for Amazon OpenSearch Serverless, or service `es` for Amazon OpenSearch Service domains.Source: Linters/SAST tools
185-186: 💤 Low valueAdd blank line before fenced code block.
Line 186 opens a code block immediately after line 185 (the OpenSearch (Self-hosted) heading) without a blank line in between. Markdown linters expect a blank line before fenced code blocks. Add one:
**OpenSearch (Self-hosted)** ```yamlSource: Linters/SAST tools
1063-1066: ⚡ Quick winVerify troubleshooting advice matches actual error modes.
Lines 1063–1066 list four OpenSearch troubleshooting scenarios:
- SigV4 auth failure → suggests checking AWS credentials and service type
- SSO credential precedence → suggests unsetting env vars
- Embedding dimension mismatch → suggests setting
OPENSEARCH_EMBEDDING_DIM- AOSS 403 → suggests granting IAM and data-access permissions
Cross-check each scenario against the live test error logs to confirm these are the most common root causes and that the fixes are correct. If the live tests uncovered other failure modes, add them to the table.
1037-1049: All OpenSearch environment variables are properly declared and used.All 13 OpenSearch-specific environment variables listed in lines 1037–1049 are:
- Declared in
sources/knowledge_layer/src/register.pyin theKnowledgeRetrievalConfigclass- Actually used by the OpenSearch adapter in
sources/knowledge_layer/src/opensearch/adapter.pyNo missing or unused variables detected.
402-435: Upstream issue is tracked; test coverage for the header workaround should be verified.The limitation is documented accurately and is tracked as a known gap in
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md(listed as the/v1/chat/completionsconversation_idsilent-drop task). The underlying cause—thatnat.data_models.api_server.ChatRequestdoes not declare aconversation_idfield—is correct for nat 1.7.0.The workaround (passing
conversation-idas an HTTP header) is architecturally sound and code inchat_researcher/register.pyshows proper header handling. However, no explicit integration tests were found for the/v1/chat/completionsendpoint with the header-based routing workaround. Consider adding a test to verify the curl example in the documentation works as intended.docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md (1)
1-6: Path verification complete—the relative path resolves correctly.The relative path
../../../deploy/helm/examples/aws-opensearch-serverless-values.yamlfromdocs/source/deployment/aws-opensearch-serverless.mdcorrectly resolves to the Helm values file. No action needed.sources/knowledge_layer/src/__init__.py (1)
25-25: LGTM!sources/knowledge_layer/src/opensearch/__init__.py (1)
22-25: LGTM!sources/knowledge_layer/README.md (1)
16-17: LGTM!Also applies to: 25-25
sources/knowledge_layer/src/opensearch/distributed.py (1)
28-120: LGTM!sources/knowledge_layer/src/register.py (1)
41-46: Local validation passes—code meets Python requirements.Ruff validation confirms compliance with style and formatting standards. Python syntax is valid, all type definitions (BackendType, OpenSearchAuthType, OpenSearchAwsService, OpenSearchIngestionMode, OpenSearchDaskFileTransfer) are correctly declared, and imports for the OpenSearch adapter are in place. The function signature and error handling at lines 472–557 are well-formed.
pyproject.toml (1)
98-102: LGTM!tests/knowledge_layer_tests/run_adapter_compliance.py (1)
21-27: LGTM!Also applies to: 97-98, 457-459
tests/knowledge_layer_tests/test_opensearch_live.py (1)
38-215: LGTM!tests/knowledge_layer_tests/test_opensearch_serverless_live.py (1)
43-348: LGTM!tests/knowledge_layer_tests/test_opensearch_adapter.py (1)
274-1133: Ruff validation passed; pytest unavailable in sandbox environment.Ruff check and format validation for all four modified test files passed:
test_opensearch_adapter.py: ✅ All checks passedtest_opensearch_live.py: ✅ All checks passedtest_opensearch_serverless_live.py: ✅ All checks passedrun_adapter_compliance.py: ✅ All checks passed- All files already properly formatted
Pytest cannot be executed (pytest not installed in sandbox), so behavioral test coverage cannot be validated in this environment.
rhossi
force-pushed
the
feat/opensearch-aoss
branch
from
098160c to
ebcd02b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
docs/source/deployment/aws-opensearch-serverless.md (1)
424-432:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd SSO credential verification step (addresses past review comment).
Lines 425–426 clear stale credential environment variables and run
aws sso login, but the script does not verify that the SSO login succeeded or that credentials are active before running the live tests. Add a verification step immediately after theaws sso logincommand:aws sts get-caller-identity --profile cs-adminThis confirms credentials are active and the SSO login was successful before proceeding to the pytest command.
🔧 Proposed fix
unset AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY AWS_SESSION_TOKEN AWS_CREDENTIAL_EXPIRATION aws sso login --profile cs-admin + +# Verify credentials are active +aws sts get-caller-identity --profile cs-admin AIQ_OPENSEARCH_SERVERLESS_LIVE_TESTS=1 \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/source/deployment/aws-opensearch-serverless.md` around lines 424 - 432, The bash script clears environment variables and runs aws sso login but does not verify that the SSO login succeeded before proceeding to the pytest command. Add a credential verification step immediately after the aws sso login --profile cs-admin line by inserting the command aws sts get-caller-identity --profile cs-admin. This verification step will confirm that credentials are active and the SSO authentication was successful before the tests run, preventing failures due to invalid or expired credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md`:
- Line 25: Both planning files violate markdown heading hierarchy (MD001) by
jumping from H1 directly to H3, skipping H2, which blocks strict docs builds.
Replace all occurrences of `### Task` with `## Task` in both planning files (the
current file and the other planning file mentioned) to restore proper heading
hierarchy. After making the changes, verify locally by running cd docs &&
SPHINXOPTS="-W" make html to confirm the fix resolves the hierarchy error.
In `@docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md`:
- Around line 268-269: Remove hardcoded personal file paths from the shell
commands in the documentation. At lines 268-269 (first grep command with
chat/completions and conversation_id fallback) and lines 276-277 (grep command
with nat module reference), delete the entire conditional fallback branch that
references
/Users/fdecarvalhop/Documents/projects/aiq/.venv/lib/python3.13/site-packages/aiq_api.
Keep only the primary grep command that searches in src/ directory; if the
target is not found in src/, allow the command to fail transparently rather than
falling back to personal environment paths that will not work for other
engineers.
In `@sources/knowledge_layer/src/opensearch/adapter.py`:
- Around line 552-553: The filtering of invalid paths at the anchor site
(sources/knowledge_layer/src/opensearch/adapter.py, lines 552-553) creates a
mismatch between the filtered validated_paths list and the unfiltered
original_filenames list. When indices from validated_paths are later used to
access original_filenames at the sibling sites (lines 570-573 and 1157-1159),
this causes wrong file metadata to be persisted if any earlier path was invalid.
Fix this by filtering original_filenames in parallel with validated_paths at the
anchor site using the same filtering logic, so that both lists remain aligned.
Then verify that the sibling sites (lines 570-573 and 1157-1159) are using
indices against the now-correctly-filtered original_filenames list.
- Around line 1403-1417: The synchronous `_embed_texts()` method makes blocking
OpenAI API calls that are invoked from the async `retrieve()` method, which
blocks the event loop and prevents concurrent request handling. Wrap the call to
`self._embed_texts()` in `asyncio.to_thread()` to execute the blocking operation
on a thread pool instead of blocking the event loop. Similarly, wrap the
`client.search()` call on line 1440 (in the retrieve method) with
`asyncio.to_thread()` to prevent the OpenSearch query from blocking concurrent
operations.
In `@sources/knowledge_layer/src/opensearch/distributed.py`:
- Line 111: Wrap the ingestor._update_collection_timestamp(collection_name) call
in a try-except block to make it non-fatal. Catch any exceptions that occur
during the timestamp update and log them as a warning or error without
re-raising, allowing the Dask task to complete successfully with the per-file
indexing results intact. Since the coordinator already refreshes the timestamp
when applying the worker result, the timestamp update failure should not cause
the entire task to fail.
In `@tests/knowledge_layer_tests/test_opensearch_adapter.py`:
- Around line 279-283: In addition to clearing the ingestor cache for
"opensearch" at line 279 with factory._INGESTOR_INSTANCES.pop(), also clear the
retriever cache for "opensearch" using an analogous pop call on the retriever
cache (likely factory._RETRIEVER_INSTANCES) to prevent cached retriever
instances from other tests from affecting the determinism of this test that
calls both get_ingestor and get_retriever with the same "opensearch"
configuration.
---
Duplicate comments:
In `@docs/source/deployment/aws-opensearch-serverless.md`:
- Around line 424-432: The bash script clears environment variables and runs aws
sso login but does not verify that the SSO login succeeded before proceeding to
the pytest command. Add a credential verification step immediately after the aws
sso login --profile cs-admin line by inserting the command aws sts
get-caller-identity --profile cs-admin. This verification step will confirm that
credentials are active and the SSO authentication was successful before the
tests run, preventing failures due to invalid or expired credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 29cdaef5-10d6-41ab-bce4-7edf6ad6da6b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
configs/config_web_opensearch.ymldeploy/helm/README.mddeploy/helm/examples/aws-opensearch-serverless-values.yamldocs/source/deployment/aws-opensearch-serverless.mddocs/source/deployment/index.mddocs/source/index.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mdpyproject.tomlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/README.mdsources/knowledge_layer/pyproject.tomlsources/knowledge_layer/src/__init__.pysources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/opensearch/adapter.pysources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/run_adapter_compliance.pytests/knowledge_layer_tests/test_opensearch_adapter.pytests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_serverless_live.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
deploy/helm/examples/aws-opensearch-serverless-values.yamldeploy/helm/README.mdconfigs/config_web_opensearch.yml
**
⚙️ CodeRabbit configuration file
**: # Contributing GuidelinesWe welcome contributions to the NVIDIA AI-Q blueprint. This repository uses a maintainer-reviewed pull request workflow with DCO sign-off, code-owner review, copy-pr-bot mirroring, and GitHub Actions validation.
Before You Start
- Search existing issues and pull requests before opening new work.
- Open an issue or discussion before large design changes, public APIs, deployment changes, or contributor workflow changes.
- Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
- Target the
developbranch unless a maintainer asks you to use a release branch.Pull Requests
- Fork the repository and create a focused branch from
develop.- Make the smallest coherent change and add or update tests for behavior changes.
- Sign off every commit with
git commit -s.- Run the relevant local validation before opening the PR.
- Open a pull request into
developand fill out the PR template with exact validation evidence.- Address review feedback until required checks and code-owner review pass.
Local Validation
Use the narrowest command that covers your change, then include the exact output or workflow link in the PR.
uv sync --group dev uv run ruff check . uv run ruff format --check . uv run pytestFor UI changes:
cd frontends/ui npm ci npm run lint npm run type-check npm run test:ci npm run buildFor deployment changes, run the relevant Helm or compose validation and describe the environment used.
CI and Bot Workflow
AI-Q uses push-triggered GitHub Actions. Pull requests are mirrored by copy-pr-bot to
pull-request/<PR number>branches after a maintainer or configured vetter comments/ok to test, and CI runs on those mirrored branches.Repository owners, organization members, and collaborators can request NVSkills validation by commenting:
/nvskills-ciMaintainers can request bot-driven merge with:
``...
Files:
deploy/helm/examples/aws-opensearch-serverless-values.yamldeploy/helm/README.mddocs/source/index.mdsources/knowledge_layer/src/__init__.pydocs/source/deployment/index.mdsources/knowledge_layer/src/opensearch/__init__.pypyproject.tomltests/knowledge_layer_tests/run_adapter_compliance.pysources/knowledge_layer/README.mdsources/knowledge_layer/pyproject.tomlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/opensearch/distributed.pyconfigs/config_web_opensearch.ymldocs/source/deployment/aws-opensearch-serverless.mdsources/knowledge_layer/src/register.pydocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mdtests/knowledge_layer_tests/test_opensearch_live.pydocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mdtests/knowledge_layer_tests/test_opensearch_serverless_live.pysources/knowledge_layer/src/opensearch/adapter.pytests/knowledge_layer_tests/test_opensearch_adapter.py
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/index.mddocs/source/deployment/index.mddocs/source/deployment/aws-opensearch-serverless.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
Files:
sources/knowledge_layer/src/__init__.pysources/knowledge_layer/src/opensearch/__init__.pytests/knowledge_layer_tests/run_adapter_compliance.pysources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_serverless_live.pysources/knowledge_layer/src/opensearch/adapter.pytests/knowledge_layer_tests/test_opensearch_adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/__init__.pysources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/README.mdsources/knowledge_layer/pyproject.tomlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/register.pysources/knowledge_layer/src/opensearch/adapter.py
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}
⚙️ CodeRabbit configuration file
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}: Review automation and packaging changes for least-privilege permissions, pinned versions where appropriate,
copy-pr-bot pull-request/ branch behavior, reproducible uv/npm setup, secret handling, and consistency with
the documented validation matrix.
Files:
pyproject.toml
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_serverless_live.pytests/knowledge_layer_tests/test_opensearch_adapter.py
🪛 ast-grep (0.43.0)
sources/knowledge_layer/src/register.py
[info] 275-275: Do not hardcode temporary file or directory names
Context: "/tmp/chroma_data"
Note: [CWE-377].
(hardcoded-tmp-file)
[info] 283-283: Do not hardcode temporary file or directory names
Context: "/tmp/chroma_data"
Note: [CWE-377].
(hardcoded-tmp-file)
tests/knowledge_layer_tests/test_opensearch_adapter.py
[warning] 892-892: Do not make http calls without encryption
Context: "http://nim-embed.ns-nim.svc.cluster.local:8000/v1"
Note: [CWE-319].
(requests-http)
🪛 LanguageTool
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
[style] ~216-~216: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...verless, use SigV4 with service aoss. For Amazon OpenSearch Service domains, use ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[style] ~18-~18: Consider an alternative to strengthen your wording.
Context: ...lready cross-links the example file (no further changes here). - `docs/source/deployment/index....
(CHANGES_ADJUSTMENTS)
[style] ~19-~19: Consider an alternative to strengthen your wording.
Context: ...lready lists the new doc on line 30 (no further changes here). - `configs/config_web_opensearch...
(CHANGES_ADJUSTMENTS)
[style] ~20-~20: Consider an alternative to strengthen your wording.
Context: ...ig, already env-substitution-driven (no further changes here). - `sources/knowledge_layer/KNOWL...
(CHANGES_ADJUSTMENTS)
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[style] ~335-~335: To elevate your writing, try using a synonym here.
Context: ... "did the worker actually do anything?" hard to confirm. A doc note saves the next p...
(HARD_TO)
🪛 markdownlint-cli2 (0.22.1)
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
[warning] 186-186: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[warning] 25-25: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 97-97: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 158-158: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 238-238: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 328-328: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 329-329: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 423-423: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 423-423: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 508-508: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 508-508: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 587-587: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 587-587: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 678-678: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 678-678: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 737-737: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 738-738: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 738-738: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[warning] 29-29: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 166-166: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 167-167: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 167-167: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 241-241: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 242-242: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 242-242: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md (1)
29-134: ⚡ Quick winVerify Task 1 status: code and tests appear to already exist on the branch.
The relevant code snippets show
_resolve_embedding_api_keyhelper and both ingestor and retriever_embed_textsmethods already call it, and the unit test file already containstest_ingestor_embed_raises_when_hosted_api_and_missing_keyandtest_ingestor_embed_allows_local_nim_without_keywith the exact structure this task asks to write. Confirm whether:
- This code is already committed on the branch and the plan should mark Task 1 as already done.
- The plan was drafted before the implementation and needs updating to reflect current state.
- This is a duplicate implementation and should be skipped.
If the code is already present and passing tests, update Task 1 to say "Verify the implementation exists and tests pass" rather than "Write and implement from scratch."
sources/knowledge_layer/pyproject.toml (1)
55-62: Unresolved dependency mismatch remains.
opensearchextra still does not includedistributed, whilesources/knowledge_layer/src/opensearch/adapter.pyLine 649 importsfrom distributed import Clientfor Dask ingestion. This is the same issue already reported earlier.sources/knowledge_layer/src/opensearch/adapter.py (1)
934-944: File identity is still coupled tofile_nameacross delete/list/status paths.This remains the same previously reported issue: duplicate filenames in a collection can still collapse file records and broaden deletion/status matching.
Also applies to: 1042-1045, 1121-1123, 1349-1363
sources/knowledge_layer/src/__init__.py (1)
25-25: LGTM!sources/knowledge_layer/src/opensearch/__init__.py (1)
22-25: LGTM!sources/knowledge_layer/src/register.py (1)
41-45: Provide lint/format/test validation evidence for these Python behavior changes.Please attach the exact outputs for:
uv run ruff check .uv run ruff format --check .uv run pytest(or the narrowest pytest command that fully covers these backend changes)As per coding guidelines, "Run ruff check and ruff format validation for Python code changes" and "Run pytest for all behavior changes in Python code".
Also applies to: 48-78, 111-251, 253-301, 346-387
Source: Coding guidelines
sources/knowledge_layer/README.md (1)
16-17: LGTM!Also applies to: 25-26
tests/knowledge_layer_tests/test_opensearch_adapter.py (1)
1-278: LGTM!Also applies to: 284-1133
pyproject.toml (1)
98-102: LGTM!tests/knowledge_layer_tests/run_adapter_compliance.py (1)
21-27: LGTM!Also applies to: 97-98, 457-459
tests/knowledge_layer_tests/test_opensearch_live.py (1)
1-215: LGTM!tests/knowledge_layer_tests/test_opensearch_serverless_live.py (1)
1-348: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
sources/knowledge_layer/src/opensearch/adapter.py (2)
1419-1441:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid blocking the event loop inside
async retrieve().Line 1438 and Line 1440 perform synchronous network I/O inside an async method. This blocks concurrent requests on the event loop.
Proposed fix
+import asyncio ... - query_embedding = self._embed_texts([query])[0] + query_embedding = (await asyncio.to_thread(self._embed_texts, [query]))[0] body = self._build_search_body(query_embedding, top_k or self.default_top_k, filters) - response = client.search(index=index_name, body=body, request_timeout=self.timeout) + response = await asyncio.to_thread( + client.search, + index=index_name, + body=body, + request_timeout=self.timeout, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sources/knowledge_layer/src/opensearch/adapter.py` around lines 1419 - 1441, The async retrieve method contains synchronous blocking I/O calls to the OpenSearch client: the client.indices.exists() call and the client.search() call. Replace the synchronous OpenSearch client obtained from _get_client() with an AsyncOpenSearch client, then convert both the indices.exists() and search() calls to use their async counterparts (await client.indices.exists() and await client.search()) to prevent blocking the event loop during concurrent request handling.
552-573:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
original_filenamesaligned with filtered paths.Line 552 filters invalid paths, but Line 571 indexes into the unfiltered
original_filenames. That mislabels files when an earlier path is invalid, and the mismatch propagates into Line 1156 during ingestion.Proposed fix
- validated_paths = [path for path in file_paths if os.path.exists(path)] + validated_entries: list[tuple[str, str]] = [] + for i, path in enumerate(file_paths): + if not os.path.exists(path): + continue + name = original_filenames[i] if i < len(original_filenames) else Path(path).name + validated_entries.append((path, name)) + validated_paths = [path for path, _ in validated_entries] if not validated_paths: ... file_details = [] - for i, path in enumerate(validated_paths): - file_name = original_filenames[i] if i < len(original_filenames) else Path(path).name + for i, (path, file_name) in enumerate(validated_entries): file_id = requested_file_id if requested_file_id and len(validated_paths) == 1 else str(uuid.uuid4()) ... + job_config["original_filenames"] = [name for _, name in validated_entries]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sources/knowledge_layer/src/opensearch/adapter.py` around lines 552 - 573, The issue is that when `validated_paths` filters out invalid file paths at line 552, the `original_filenames` list is not filtered in parallel, causing index misalignment when the loop at line 571 tries to retrieve the corresponding original filename for each validated path. When an earlier path is invalid and filtered out, the index `i` no longer corresponds to the correct position in the unfiltered `original_filenames` list. To fix this, create a parallel filtered list of original filenames that matches the validated paths, or build a mapping that associates each validated path with its corresponding original filename before the loop that constructs `file_details`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md`:
- Around line 80-97: The markdown code fences in both planning documents lack
proper blank-line spacing and language specifications, causing MD031/MD040
linter warnings. Fix these formatting issues by: (1) adding a blank line before
the opening code fence and after the closing code fence at each location, and
(2) specifying the appropriate language identifier (mermaid, bash, json, yaml,
markdown, python, etc.) on the opening fence. In
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md, apply
this fix at lines 80-97 (the mermaid diagram shown), 158, 238, 328-329, 423,
508, 587, 678, and 737-738. In
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md, apply the same
fixes at lines 166-167 and 241-242. After fixing, run cd docs && make clean &&
SPHINXOPTS="-W" make html to confirm all warnings are resolved.
In `@sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md`:
- Around line 1037-1049: The environment variables table in the
KNOWLEDGE-LAYER-SETUP.md documentation is missing several OpenSearch
configuration options that are actually supported and used in the codebase
(specifically register.py). Add the missing variables to the table with their
descriptions and "opensearch" component designation, including:
OPENSEARCH_VERIFY_CERTS, OPENSEARCH_VECTOR_FIELD, OPENSEARCH_TEXT_FIELD,
OPENSEARCH_EMBEDDING_DIM, OPENSEARCH_ENGINE, OPENSEARCH_SPACE_TYPE,
OPENSEARCH_M, OPENSEARCH_EF_CONSTRUCTION, OPENSEARCH_EF_SEARCH,
OPENSEARCH_TIMEOUT, OPENSEARCH_MAX_RETRIES, OPENSEARCH_CHUNK_SIZE,
OPENSEARCH_CHUNK_OVERLAP, OPENSEARCH_BULK_BATCH_SIZE,
OPENSEARCH_EMBEDDING_BATCH_SIZE, OPENSEARCH_AOSS_DELETE_MAX_BATCHES, and
OPENSEARCH_AOSS_DELETE_BACKOFF_SECONDS. Alternatively, if the table is
intentionally curated to show only essential options, add a clarifying note
indicating that advanced configuration options are available in register.py.
In `@sources/knowledge_layer/src/opensearch/adapter.py`:
- Around line 1-25: The OpenSearch adapter has two critical bugs that must be
fixed. First, in the file validation logic around the area where validated_paths
is filtered and original_filenames is indexed (around lines 552-573), the
enumeration index i is being used directly on original_filenames while
validated_paths is being filtered separately, causing an index mismatch when
files are excluded. Fix this by ensuring the index stays aligned with the
validated paths—either by filtering original_filenames in parallel with
validated_paths, or by using the correct index from the filtered set. Second,
the retrieve() async method (around lines 1419-1441) calls synchronous functions
_embed_texts() and client.search() directly, which blocks the event loop and
degrades concurrent request handling. Move these blocking calls to thread pools
using asyncio.to_thread() or similar async utilities, or reimplement them as
async methods to properly handle concurrent requests.
In `@sources/knowledge_layer/src/opensearch/distributed.py`:
- Around line 112-120: The current logic incorrectly marks the task as failed
when no files are processed because the all-failed check compares failed_count
== len(file_results) (both 0 for an empty batch). Add an explicit check for
empty file_results before the status determination: if file_results is empty,
return a result with status "completed" (since no ingestion was needed rather
than failed). Move this empty-batch check before the current failed_count ==
len(file_results) condition so that only batches with actual files that all
failed are marked as failed. Update the status field in the return dictionary to
use this revised logic while keeping other fields consistent.
---
Duplicate comments:
In `@sources/knowledge_layer/src/opensearch/adapter.py`:
- Around line 1419-1441: The async retrieve method contains synchronous blocking
I/O calls to the OpenSearch client: the client.indices.exists() call and the
client.search() call. Replace the synchronous OpenSearch client obtained from
_get_client() with an AsyncOpenSearch client, then convert both the
indices.exists() and search() calls to use their async counterparts (await
client.indices.exists() and await client.search()) to prevent blocking the event
loop during concurrent request handling.
- Around line 552-573: The issue is that when `validated_paths` filters out
invalid file paths at line 552, the `original_filenames` list is not filtered in
parallel, causing index misalignment when the loop at line 571 tries to retrieve
the corresponding original filename for each validated path. When an earlier
path is invalid and filtered out, the index `i` no longer corresponds to the
correct position in the unfiltered `original_filenames` list. To fix this,
create a parallel filtered list of original filenames that matches the validated
paths, or build a mapping that associates each validated path with its
corresponding original filename before the loop that constructs `file_details`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ddc6e660-28a8-4152-a371-c9221ea82a73
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
configs/config_web_opensearch.ymldeploy/helm/README.mddeploy/helm/examples/aws-opensearch-serverless-values.yamldocs/source/deployment/aws-opensearch-serverless.mddocs/source/deployment/index.mddocs/source/index.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mdpyproject.tomlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/README.mdsources/knowledge_layer/pyproject.tomlsources/knowledge_layer/src/__init__.pysources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/opensearch/adapter.pysources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/run_adapter_compliance.pytests/knowledge_layer_tests/test_opensearch_adapter.pytests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_serverless_live.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/index.mddocs/source/deployment/index.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mddocs/source/deployment/aws-opensearch-serverless.md
**
⚙️ CodeRabbit configuration file
**: # Contributing GuidelinesWe welcome contributions to the NVIDIA AI-Q blueprint. This repository uses a maintainer-reviewed pull request workflow with DCO sign-off, code-owner review, copy-pr-bot mirroring, and GitHub Actions validation.
Before You Start
- Search existing issues and pull requests before opening new work.
- Open an issue or discussion before large design changes, public APIs, deployment changes, or contributor workflow changes.
- Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
- Target the
developbranch unless a maintainer asks you to use a release branch.Pull Requests
- Fork the repository and create a focused branch from
develop.- Make the smallest coherent change and add or update tests for behavior changes.
- Sign off every commit with
git commit -s.- Run the relevant local validation before opening the PR.
- Open a pull request into
developand fill out the PR template with exact validation evidence.- Address review feedback until required checks and code-owner review pass.
Local Validation
Use the narrowest command that covers your change, then include the exact output or workflow link in the PR.
uv sync --group dev uv run ruff check . uv run ruff format --check . uv run pytestFor UI changes:
cd frontends/ui npm ci npm run lint npm run type-check npm run test:ci npm run buildFor deployment changes, run the relevant Helm or compose validation and describe the environment used.
CI and Bot Workflow
AI-Q uses push-triggered GitHub Actions. Pull requests are mirrored by copy-pr-bot to
pull-request/<PR number>branches after a maintainer or configured vetter comments/ok to test, and CI runs on those mirrored branches.Repository owners, organization members, and collaborators can request NVSkills validation by commenting:
/nvskills-ciMaintainers can request bot-driven merge with:
``...
Files:
docs/source/index.mddeploy/helm/README.mdsources/knowledge_layer/src/opensearch/__init__.pydocs/source/deployment/index.mdsources/knowledge_layer/src/__init__.pysources/knowledge_layer/pyproject.tomldeploy/helm/examples/aws-opensearch-serverless-values.yamlsources/knowledge_layer/README.mdpyproject.tomlsources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdtests/knowledge_layer_tests/run_adapter_compliance.pydocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mddocs/source/deployment/aws-opensearch-serverless.mdsources/knowledge_layer/src/register.pytests/knowledge_layer_tests/test_opensearch_serverless_live.pyconfigs/config_web_opensearch.ymltests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_adapter.pysources/knowledge_layer/src/opensearch/adapter.py
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
deploy/helm/README.mddeploy/helm/examples/aws-opensearch-serverless-values.yamlconfigs/config_web_opensearch.yml
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
Files:
sources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/__init__.pysources/knowledge_layer/src/opensearch/distributed.pytests/knowledge_layer_tests/run_adapter_compliance.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/test_opensearch_serverless_live.pytests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_adapter.pysources/knowledge_layer/src/opensearch/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/__init__.pysources/knowledge_layer/pyproject.tomlsources/knowledge_layer/README.mdsources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/register.pysources/knowledge_layer/src/opensearch/adapter.py
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}
⚙️ CodeRabbit configuration file
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}: Review automation and packaging changes for least-privilege permissions, pinned versions where appropriate,
copy-pr-bot pull-request/ branch behavior, reproducible uv/npm setup, secret handling, and consistency with
the documented validation matrix.
Files:
pyproject.toml
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_opensearch_serverless_live.pytests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_adapter.py
🪛 ast-grep (0.43.0)
sources/knowledge_layer/src/register.py
[info] 275-275: Do not hardcode temporary file or directory names
Context: "/tmp/chroma_data"
Note: [CWE-377].
(hardcoded-tmp-file)
[info] 283-283: Do not hardcode temporary file or directory names
Context: "/tmp/chroma_data"
Note: [CWE-377].
(hardcoded-tmp-file)
tests/knowledge_layer_tests/test_opensearch_adapter.py
[warning] 892-892: Do not make http calls without encryption
Context: "http://nim-embed.ns-nim.svc.cluster.local:8000/v1"
Note: [CWE-319].
(requests-http)
🪛 LanguageTool
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
[style] ~216-~216: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...verless, use SigV4 with service aoss. For Amazon OpenSearch Service domains, use ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[style] ~18-~18: Consider an alternative to strengthen your wording.
Context: ...lready cross-links the example file (no further changes here). - `docs/source/deployment/index....
(CHANGES_ADJUSTMENTS)
[style] ~19-~19: Consider an alternative to strengthen your wording.
Context: ...lready lists the new doc on line 30 (no further changes here). - `configs/config_web_opensearch...
(CHANGES_ADJUSTMENTS)
[style] ~20-~20: Consider an alternative to strengthen your wording.
Context: ...ig, already env-substitution-driven (no further changes here). - `sources/knowledge_layer/KNOWL...
(CHANGES_ADJUSTMENTS)
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[style] ~334-~334: To elevate your writing, try using a synonym here.
Context: ... "did the worker actually do anything?" hard to confirm. A doc note saves the next p...
(HARD_TO)
🪛 markdownlint-cli2 (0.22.1)
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
[warning] 186-186: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[warning] 97-97: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 158-158: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 238-238: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 328-328: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 329-329: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 423-423: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 423-423: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 508-508: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 508-508: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 587-587: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 587-587: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 678-678: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 678-678: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 737-737: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 738-738: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 738-738: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[warning] 166-166: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 167-167: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 167-167: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 241-241: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 242-242: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 242-242: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (24)
sources/knowledge_layer/src/opensearch/distributed.py (5)
111-111: Duplicate: make collection timestamp refresh non-fatal.Line 111 can still throw after successful indexing and fail the whole Dask task instead of returning the structured per-file results.
1-38: LGTM!
40-46: LGTM!
50-64: LGTM!
65-109: LGTM!docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md (1)
29-135: ⚡ Quick winTask 1 may be redundant:
_resolve_embedding_api_keyappears to already exist.The relevant_code_snippets section shows
_resolve_embedding_api_key(embed_base_url: str)already defined atsources/knowledge_layer/src/opensearch/adapter.py:262–271, raisingRuntimeErrorwhen hosted API lacksNVIDIA_API_KEY. Task 1 Steps 1–3 propose writing this function, but it may already be on the branch.If the function exists, Task 1 should skip the function definition (lines 93–103) and focus only on:
- Adding unit tests (already included in Steps 1–2).
- Verifying/integrating the helper into both
_embed_textscallers (ingestor + retriever).- Confirming no regressions.
Verify against the current branch state before executing Task 1; if the helper is already there, streamline the task to avoid redundant work.
Verify the branch state:
grep -n "def _resolve_embedding_api_key" sources/knowledge_layer/src/opensearch/adapter.pyIf present, update Task 1 instructions to skip the function definition step.
docs/source/deployment/aws-opensearch-serverless.md (2)
1-479: LGTM! The guide is comprehensive, well-structured, and commands are accurate. The architecture diagram clearly explains Pod Identity and SigV4 credential flow. All AWS CLI commands (security policies, collection creation, IAM role setup, Pod Identity association) follow correct syntax. The verification steps are concrete and the troubleshooting table maps symptoms to causes effectively. The SSO credential cleanup section (line 425–427) includes the verification step flagged in the previous review.
297-297: All relative paths resolve correctly and target files exist.Verification confirms:
docs/source/deployment/aws-opensearch-serverless.mdline 297:../../../deploy/helm/examples/aws-opensearch-serverless-values.yaml→deploy/helm/examples/aws-opensearch-serverless-values.yaml✓sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdline 241:../../docs/source/deployment/aws-opensearch-serverless.md→docs/source/deployment/aws-opensearch-serverless.md✓Both target files exist and relative paths are correct for Sphinx builds from repository root.
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md (3)
185-200: ⚡ Quick winAdd blank line before code block (MD031).
Line 186 has a code fence immediately after bold text without a blank line in between. Add one blank line before the opening triple backticks:
**OpenSearch (Self-hosted)** + ```yamlSource: Linters/SAST tools
215-224: 💤 Low valueReduce repetition of "For" at sentence start.
Lines 216–217 open successive sentences with "For":
- "For Amazon OpenSearch Serverless, use…"
- "For Amazon OpenSearch Service domains, use…"
Consider restructuring for variety:
For Amazon OpenSearch Serverless, use SigV4 with service `aoss`. For Amazon OpenSearch Service domains, use -service `es`. +service `es` instead.or:
-For Amazon OpenSearch Serverless, use SigV4 with service `aoss`. For Amazon OpenSearch Service domains, use -service `es`. + +Use SigV4 with service `aoss` for Amazon OpenSearch Serverless, or service `es` for Amazon OpenSearch Service domains.Source: Linters/SAST tools
402-435: 💤 Low valueClarify the conversation-id limitation and upstream tracking.
The "How collection routing works" section (lines 402–435) clearly explains the
/v1/chat/completionsconversation-id body-field limitation and provides a curl workaround with theconversation-idHTTP header. The workaround is practical and the section correctly notes this is a gap in the upstreamnat/aiq_apirepository, not in this repo.One minor suggestion: Consider adding a GitHub issue link or tracking issue number if available, so users know the status of the permanent fix.
docs/source/deployment/index.md (1)
30-30: LGTM! The link to the new Amazon OpenSearch Serverless guide is correctly positioned in the "Deployment Guides" section and uses consistent link formatting.docs/source/index.md (1)
103-103: LGTM! The toctree entry for "Amazon OpenSearch Serverless" is correctly formatted and placed after "Kubernetes" in the Deployment section, maintaining consistent alphabetical-ish ordering.sources/knowledge_layer/pyproject.toml (1)
21-21: LGTM!Also applies to: 55-66
sources/knowledge_layer/src/__init__.py (1)
25-25: LGTM!sources/knowledge_layer/src/opensearch/__init__.py (1)
22-25: LGTM!sources/knowledge_layer/src/register.py (1)
41-45: LGTM!Also applies to: 48-78, 111-251, 271-300, 346-387, 479-480
sources/knowledge_layer/README.md (1)
16-17: LGTM!Also applies to: 25-25
tests/knowledge_layer_tests/test_opensearch_adapter.py (2)
279-283: Reset both factory caches in this setup path.Line 279 clears only
_INGESTOR_INSTANCES, but this test also asserts retriever wiring at Line 282; a stale_RETRIEVER_INSTANCESentry can make this order-dependent.
39-272: LGTM!Also applies to: 290-1133
pyproject.toml (1)
98-102: Provide validation evidence for these Python/test changes.Please attach the local validation outputs for:
uv run ruff check .uv run ruff format --check .uv run pytest(or an explicitly scoped pytest command that covers these new/changed tests).As per coding guidelines,
**/*.pychanges must run Ruff check/format validation and**/*test*.pybehavior changes must run pytest.Source: Coding guidelines
tests/knowledge_layer_tests/run_adapter_compliance.py (1)
21-27: LGTM!Also applies to: 97-98, 457-459
tests/knowledge_layer_tests/test_opensearch_live.py (1)
38-215: LGTM!tests/knowledge_layer_tests/test_opensearch_serverless_live.py (1)
57-348: LGTM!
rhossi
force-pushed
the
feat/opensearch-aoss
branch
from
cf3b045 to
971a3ba
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md (1)
1-898:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCode fence language specifiers are still missing; will cause Task 12 strict build to fail.
The static analysis tool reports MD040 (missing language specifier) warnings at lines 168, 255, 353, 453, 544, 630, 731, and 797. A past review comment indicated these were addressed in commits ebcd02b and 971a3ba, but the linter still flags violations. Task 12 explicitly requires a strict Sphinx build with
SPHINXOPTS="-W --keep-going -n", which treats warnings as errors — this plan will fail at its final gate if these are not fixed.Action: For each flagged line, add a language identifier to the opening code fence:
- If the fence opens with bare
```, change it to```bash(or appropriate language:json,yaml,markdown,python, etc.)- If already correctly specified (e.g.,
```bash), the linter may be reporting a false positive or the file has been edited since the linter ran.Run locally to verify:
cd docs && SPHINXOPTS="-W" make clean && SPHINXOPTS="-W --keep-going" make html 2>&1 | grep -i "md040\|fenced"Fix all reported issues, then re-run to confirm zero warnings before committing Task 12.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md` around lines 1 - 898, The code fences in the OpenSearch EKS deployment plan documentation file are missing language identifiers, which will cause the strict Sphinx build in Task 12 to fail with MD040 warnings treated as errors. For each of the flagged lines (168, 255, 353, 453, 544, 630, 731, and 797) in `docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md`, replace bare triple-backticks (` ``` `) with a language-specified fence (` ```bash ` for shell commands, ` ```json ` for JSON, ` ```yaml ` for YAML, etc.). After making all fixes, run the linter verification command to confirm zero warnings remain, then commit the changes.Source: Linters/SAST tools
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md (1)
1-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCode fence language specifiers missing at lines 177 and 260; blocks strict Sphinx build.
Static analysis reports MD040 (missing language specifier) at lines 177 and 260. These correspond to MyST admonition fences in Task 2 (text-only callout) and Task 4 (visibility delay note). While MyST's
{note}directive doesn't strictly require a code-block language, the linter flags bare triple-backtick fences without language identifiers.Fix: Ensure all fenced code blocks (including admonition bodies) have a language specifier:
```bashfor shell commands```pythonfor Python code```jsonfor JSON examples```markdownor```textfor plain prose in fencesRun locally:
cd docs && SPHINXOPTS="-W" make clean && SPHINXOPTS="-W --keep-going" make html 2>&1 | grep -i "md040\|fenced"Fix all issues before Task 8's final quality gate, which includes
SPHINXOPTS="-W"strict build.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md` around lines 1 - 505, The markdown linter flags missing language specifiers on code fences at lines 177 and 260 in the plan document, which blocks the strict Sphinx build (SPHINXOPTS="-W"). Both issues occur in Task 2 Step 2 and Task 4 Step 1 where MyST admonition blocks are fenced with bare triple-backticks. Add language specifiers to all code fences: use `bash` for shell commands, `python` for Python code, `json` for JSON, `markdown` or `text` for prose. For the MyST note directives (the fenced blocks starting with ``` followed by {note}), either convert them to proper MyST syntax without outer fence markers (use the ````{note}```` format directly) or add a language specifier like ` ```markdown ` if keeping them as fenced blocks. Verify the fix by running a strict Sphinx build locally with `cd docs && SPHINXOPTS="-W" make clean && SPHINXOPTS="-W --keep-going" make html` and confirm no MD040 linter errors appear before Task 8's final quality gate.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md`:
- Around line 1-898: The code fences in the OpenSearch EKS deployment plan
documentation file are missing language identifiers, which will cause the strict
Sphinx build in Task 12 to fail with MD040 warnings treated as errors. For each
of the flagged lines (168, 255, 353, 453, 544, 630, 731, and 797) in
`docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md`,
replace bare triple-backticks (` ``` `) with a language-specified fence (`
```bash ` for shell commands, ` ```json ` for JSON, ` ```yaml ` for YAML, etc.).
After making all fixes, run the linter verification command to confirm zero
warnings remain, then commit the changes.
In `@docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md`:
- Around line 1-505: The markdown linter flags missing language specifiers on
code fences at lines 177 and 260 in the plan document, which blocks the strict
Sphinx build (SPHINXOPTS="-W"). Both issues occur in Task 2 Step 2 and Task 4
Step 1 where MyST admonition blocks are fenced with bare triple-backticks. Add
language specifiers to all code fences: use `bash` for shell commands, `python`
for Python code, `json` for JSON, `markdown` or `text` for prose. For the MyST
note directives (the fenced blocks starting with ``` followed by {note}), either
convert them to proper MyST syntax without outer fence markers (use the
````{note}```` format directly) or add a language specifier like ` ```markdown `
if keeping them as fenced blocks. Verify the fix by running a strict Sphinx
build locally with `cd docs && SPHINXOPTS="-W" make clean && SPHINXOPTS="-W
--keep-going" make html` and confirm no MD040 linter errors appear before Task
8's final quality gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f9cce398-5a87-4696-b275-24beb3850886
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.secrets.baselinedocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mdsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
**
⚙️ CodeRabbit configuration file
**: # Contributing GuidelinesWe welcome contributions to the NVIDIA AI-Q blueprint. This repository uses a maintainer-reviewed pull request workflow with DCO sign-off, code-owner review, copy-pr-bot mirroring, and GitHub Actions validation.
Before You Start
- Search existing issues and pull requests before opening new work.
- Open an issue or discussion before large design changes, public APIs, deployment changes, or contributor workflow changes.
- Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
- Target the
developbranch unless a maintainer asks you to use a release branch.Pull Requests
- Fork the repository and create a focused branch from
develop.- Make the smallest coherent change and add or update tests for behavior changes.
- Sign off every commit with
git commit -s.- Run the relevant local validation before opening the PR.
- Open a pull request into
developand fill out the PR template with exact validation evidence.- Address review feedback until required checks and code-owner review pass.
Local Validation
Use the narrowest command that covers your change, then include the exact output or workflow link in the PR.
uv sync --group dev uv run ruff check . uv run ruff format --check . uv run pytestFor UI changes:
cd frontends/ui npm ci npm run lint npm run type-check npm run test:ci npm run buildFor deployment changes, run the relevant Helm or compose validation and describe the environment used.
CI and Bot Workflow
AI-Q uses push-triggered GitHub Actions. Pull requests are mirrored by copy-pr-bot to
pull-request/<PR number>branches after a maintainer or configured vetter comments/ok to test, and CI runs on those mirrored branches.Repository owners, organization members, and collaborators can request NVSkills validation by commenting:
/nvskills-ciMaintainers can request bot-driven merge with:
``...
Files:
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mdsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
🪛 markdownlint-cli2 (0.22.1)
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[warning] 177-177: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[warning] 168-168: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 255-255: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 353-353: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 453-453: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 544-544: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 630-630: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 731-731: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 797-797: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md (1)
1050-1060: Previous review gaps addressed; environment variables table is now complete.The past review flagged missing OpenSearch tuning parameters in the env vars table. Lines 1050–1059 now document
OPENSEARCH_VERIFY_CERTS,OPENSEARCH_EMBEDDING_DIM,OPENSEARCH_CHUNK_SIZE,OPENSEARCH_CHUNK_OVERLAP,OPENSEARCH_TIMEOUT,OPENSEARCH_BULK_BATCH_SIZE, andOPENSEARCH_EMBEDDING_BATCH_SIZE. The note at line 1059 correctly points readers toregister.pyfor advanced options, completing the documentation surface.docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md (4)
29-114: ⚡ Quick winTask 1 implementation is sound; aligns with adapter code and test strategy.
Task 1 correctly identifies the NVIDIA_API_KEY gap and frames a fail-fast solution. The test code (lines 42–78) properly covers both paths: hosted API (should raise) and self-hosted NIM (should pass). The helper function
_resolve_embedding_api_keyat lines 95–105 matches the actual adapter implementation (confirmed insources/knowledge_layer/src/opensearch/adapter.py:262–271). The reuse of this helper in both ingestor and retriever_embed_textsmethods is correct per the provided context snippets.
279-351: ⚡ Quick winTask 5 triage-first approach is correct; research steps are actionable.
Task 5 appropriately frames the
/v1/chat/completionsconversation_idsilent-drop as a triage-first issue, with clear Steps 1–3 to locate the handler, determine the current routing mechanism, and decide between two acceptable outcomes (honor the field or remove it). The task correctly notes that if Step 1 reveals this is outside the OpenSearch story, the fix belongs in a separate PR. This prevents scope creep while keeping the issue documented.
388-430: ⚡ Quick winTask 7 DCO sign-off rebase is correctly specified; idempotent approach is sound.
The rebase strategy (
git rebase --signoff develop) is correct and idempotent — already-signed commits are left unchanged, and only unsigned commits receive the trailer. Verification steps at 1–3 properly validate the result before declaring readiness. No changes needed.
433-494: Task 8 quality gate is comprehensive; includes strict docs build, adapter tests, and YAML parsing.Task 8 correctly chains the final validation: strict Sphinx build (
-W --keep-going -n), adapter unit tests, reference YAML parsing, and Helm example parsing. This gate will catch any regressions introduced by earlier tasks and confirms the branch is PR-ready. Sequence and commands are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sources/knowledge_layer/src/opensearch/adapter.py (1)
1135-1140: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider wrapping
_health_check_client()inasyncio.to_threadfor consistency.Both
health_checkmethods (ingestor line 1135, retriever line 1540) call the synchronous_health_check_client()directly from async context. While less critical thanretrieve(), this can still block the event loop during cluster connectivity issues.Suggested fix
async def health_check(self) -> bool: try: - return self._health_check_client() + return await asyncio.to_thread(self._health_check_client) except Exception as e: logger.warning("OpenSearch health check failed: %s", e) return FalseApply same pattern to
OpenSearchRetriever.health_check()at line 1540.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sources/knowledge_layer/src/opensearch/adapter.py` around lines 1135 - 1140, The `health_check` method is an async function that directly calls the synchronous `_health_check_client()` method, which can block the event loop during connectivity issues. Wrap the `_health_check_client()` call with `asyncio.to_thread()` to offload the synchronous operation to a thread pool. This pattern needs to be applied in two places: the `health_check` method in the ingestor class (shown in the diff) and the `health_check` method in the `OpenSearchRetriever` class at line 1540. Ensure both implementations use the same `asyncio.to_thread` pattern for consistency.tests/knowledge_layer_tests/test_opensearch_adapter.py (1)
1071-1089:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen multi-key pagination coverage in this test fixture.
Line 1075 advances pages using only
file_id, while this test setsfile_id == file_name. That can mask bugs where cursor ordering ignores one composite key. Use distinct values and tuple-based ordering/assertions.Suggested tightening patch
- all_files = [f"f{i:02d}.txt" for i in range(7)] + all_files = [(f"id{i:02d}", f"f{i:02d}.txt") for i in range(7)] @@ - after_value = (after or {}).get("file_id") - remaining = [name for name in all_files if after_value is None or name > after_value] + after_key = after or {} + after_value = ( + after_key.get("file_id"), + after_key.get("file_name"), + ) if after_key else None + remaining = [item for item in all_files if after_value is None or item > after_value] @@ - { - "key": {"file_id": name, "file_name": name}, + { + "key": {"file_id": file_id, "file_name": file_name}, "doc_count": 1, - "doc": {"hits": {"hits": [{"_source": {"file_id": name, "file_name": name}}]}}, + "doc": {"hits": {"hits": [{"_source": {"file_id": file_id, "file_name": file_name}}]}}, "content_types": {"buckets": []}, } - for name in page + for file_id, file_name in page ] @@ - agg["after_key"] = {"file_id": page[-1], "file_name": page[-1]} + agg["after_key"] = {"file_id": page[-1][0], "file_name": page[-1][1]} @@ - assert sorted(f.file_name for f in files) == all_files + assert sorted((f.file_id, f.file_name) for f in files) == sorted(all_files) @@ - assert search_calls[1] == {"file_id": "f02.txt", "file_name": "f02.txt"} - assert search_calls[2] == {"file_id": "f05.txt", "file_name": "f05.txt"} + assert search_calls[1] == {"file_id": "id02", "file_name": "f02.txt"} + assert search_calls[2] == {"file_id": "id05", "file_name": "f05.txt"}Also applies to: 1104-1105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/knowledge_layer_tests/test_opensearch_adapter.py` around lines 1071 - 1089, The test fixture uses identical values for file_id and file_name, which can mask bugs in multi-key pagination where cursor ordering might ignore one of the composite keys. Modify the fake_search function to use distinct values for file_id and file_name, and update the pagination logic that currently only uses after_value from file_id to instead use tuple-based ordering that considers both file_id and file_name keys together. This ensures that the pagination cursor comparison on line 1075 and the after_key construction on line 1089 properly validate that both composite keys are being used correctly in the cursor ordering.
♻️ Duplicate comments (1)
sources/knowledge_layer/src/opensearch/distributed.py (1)
111-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCollection timestamp refresh remains fatal in the worker path.
This was flagged in a prior review and remains unaddressed. If
_update_collection_timestampraises after successful indexing, the entire Dask task fails and the structured per-file results are lost. The coordinator already refreshes the timestamp when applying results, so this call should be non-fatal.- ingestor._update_collection_timestamp(collection_name) + try: + ingestor._update_collection_timestamp(collection_name) + except Exception: + logger.warning( + "Failed to refresh collection timestamp for %s; preserving ingestion results", + collection_name, + exc_info=True, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sources/knowledge_layer/src/opensearch/distributed.py` at line 111, The call to ingestor._update_collection_timestamp(collection_name) will cause the entire Dask task to fail if it raises an exception, even though indexing has already succeeded and the coordinator handles timestamp refresh separately. Wrap this method call in a try-except block that catches any exceptions, logs them for debugging purposes, and continues execution without re-raising. This ensures that per-file results are preserved and the task completes successfully even if the timestamp update fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sources/knowledge_layer/src/opensearch/adapter.py`:
- Around line 553-559: The validated_paths list correctly filters and pairs
filenames with paths in submit_job, but the original_filenames in job_config
passed to _run_ingestion at line 1162 is never updated to match the filtered
paths, reintroducing the index mismatch. After constructing the validated list
at lines 553-559, extract the filtered original_filenames from the second
element of each tuple in validated and update job_config["original_filenames"]
with this filtered list before it is passed to _run_ingestion. This ensures that
when _run_ingestion executes around line 1162, it indexes into the correctly
filtered original_filenames that aligns with the validated_paths rather than the
original unfiltered list.
---
Outside diff comments:
In `@sources/knowledge_layer/src/opensearch/adapter.py`:
- Around line 1135-1140: The `health_check` method is an async function that
directly calls the synchronous `_health_check_client()` method, which can block
the event loop during connectivity issues. Wrap the `_health_check_client()`
call with `asyncio.to_thread()` to offload the synchronous operation to a thread
pool. This pattern needs to be applied in two places: the `health_check` method
in the ingestor class (shown in the diff) and the `health_check` method in the
`OpenSearchRetriever` class at line 1540. Ensure both implementations use the
same `asyncio.to_thread` pattern for consistency.
In `@tests/knowledge_layer_tests/test_opensearch_adapter.py`:
- Around line 1071-1089: The test fixture uses identical values for file_id and
file_name, which can mask bugs in multi-key pagination where cursor ordering
might ignore one of the composite keys. Modify the fake_search function to use
distinct values for file_id and file_name, and update the pagination logic that
currently only uses after_value from file_id to instead use tuple-based ordering
that considers both file_id and file_name keys together. This ensures that the
pagination cursor comparison on line 1075 and the after_key construction on line
1089 properly validate that both composite keys are being used correctly in the
cursor ordering.
---
Duplicate comments:
In `@sources/knowledge_layer/src/opensearch/distributed.py`:
- Line 111: The call to ingestor._update_collection_timestamp(collection_name)
will cause the entire Dask task to fail if it raises an exception, even though
indexing has already succeeded and the coordinator handles timestamp refresh
separately. Wrap this method call in a try-except block that catches any
exceptions, logs them for debugging purposes, and continues execution without
re-raising. This ensures that per-file results are preserved and the task
completes successfully even if the timestamp update fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 768cbf30-3dfd-4c4f-9d88-b8fbe0c5e1bd
📒 Files selected for processing (3)
sources/knowledge_layer/src/opensearch/adapter.pysources/knowledge_layer/src/opensearch/distributed.pytests/knowledge_layer_tests/test_opensearch_adapter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
Files:
sources/knowledge_layer/src/opensearch/distributed.pytests/knowledge_layer_tests/test_opensearch_adapter.pysources/knowledge_layer/src/opensearch/adapter.py
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/opensearch/adapter.py
**
⚙️ CodeRabbit configuration file
**: # Contributing GuidelinesWe welcome contributions to the NVIDIA AI-Q blueprint. This repository uses a maintainer-reviewed pull request workflow with DCO sign-off, code-owner review, copy-pr-bot mirroring, and GitHub Actions validation.
Before You Start
- Search existing issues and pull requests before opening new work.
- Open an issue or discussion before large design changes, public APIs, deployment changes, or contributor workflow changes.
- Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
- Target the
developbranch unless a maintainer asks you to use a release branch.Pull Requests
- Fork the repository and create a focused branch from
develop.- Make the smallest coherent change and add or update tests for behavior changes.
- Sign off every commit with
git commit -s.- Run the relevant local validation before opening the PR.
- Open a pull request into
developand fill out the PR template with exact validation evidence.- Address review feedback until required checks and code-owner review pass.
Local Validation
Use the narrowest command that covers your change, then include the exact output or workflow link in the PR.
uv sync --group dev uv run ruff check . uv run ruff format --check . uv run pytestFor UI changes:
cd frontends/ui npm ci npm run lint npm run type-check npm run test:ci npm run buildFor deployment changes, run the relevant Helm or compose validation and describe the environment used.
CI and Bot Workflow
AI-Q uses push-triggered GitHub Actions. Pull requests are mirrored by copy-pr-bot to
pull-request/<PR number>branches after a maintainer or configured vetter comments/ok to test, and CI runs on those mirrored branches.Repository owners, organization members, and collaborators can request NVSkills validation by commenting:
/nvskills-ciMaintainers can request bot-driven merge with:
``...
Files:
sources/knowledge_layer/src/opensearch/distributed.pytests/knowledge_layer_tests/test_opensearch_adapter.pysources/knowledge_layer/src/opensearch/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_opensearch_adapter.py
🔇 Additional comments (5)
sources/knowledge_layer/src/opensearch/adapter.py (2)
27-27: LGTM!
1435-1446: LGTM!sources/knowledge_layer/src/opensearch/distributed.py (1)
113-120: LGTM!tests/knowledge_layer_tests/test_opensearch_adapter.py (2)
105-147: LGTM!Also applies to: 163-168, 986-1007
105-105: Provide validation evidence for this changed test module.Per coding guidelines (
**/*.pyand**/*test*.py), attach output from:uv run ruff check tests/knowledge_layer_tests/test_opensearch_adapter.py uv run ruff format --check tests/knowledge_layer_tests/test_opensearch_adapter.py uv run pytest tests/knowledge_layer_tests/test_opensearch_adapter.py
rhossi
force-pushed
the
feat/opensearch-aoss
branch
from
8b6671c to
2ad792e
Compare
…AOSS support Ships OpenSearchRetriever and OpenSearchIngestor as built-in BaseRetriever and BaseIngestor implementations selectable via knowledge_retrieval YAML (backend: opensearch). Supports auth modes none, basic, and sigv4 with the aoss/es service split, AOSS-aware defaults (allow_document_ids, bulk_refresh), HNSW knn_vector mappings, TTL-driven session collection cleanup, and Dask ingestion that constructs the OpenSearch client inside each worker so SigV4 credentials resolve in the worker process (Pod Identity, SSO, env profiles). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
Unit tests cover index lifecycle, ingestion (local and Dask paths), retrieval, deletion, and TTL cleanup. Two opt-in live suites validate against a real OpenSearch endpoint: a generic suite for self-hosted/basic/sigv4 setups and a dedicated AOSS suite that exercises SigV4 health checks, eventual visibility, and aoss-specific delete batching. Adapter compliance harness extended to include the opensearch backend. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
Reference YAML wires the chat_deepresearcher_agent stack to the OpenSearch knowledge backend with env-substitution defaults so the same file works for self-hosted clusters and Amazon OpenSearch Serverless without edits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
Adds OpenSearch to the available backends matrix, switching examples for self-hosted (none/basic) and AOSS (sigv4 with aoss/es), Dask ingestion mode guidance, three live-test recipes, and OpenSearch-specific environment variables. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
Initial Amazon OpenSearch Serverless deployment guide covering workflow config, EKS Pod Identity association command, helm values override, local SSO live-test recipe, and a troubleshooting table. Helm README adds an OpenSearch section with image override and SigV4 env wiring. The example helm values file provides a starting point for AOSS deployments. Follow-up plan deepens this guide into an end-to-end EKS walkthrough. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
Twelve-task plan that turns the initial AOSS deployment guide into a self-contained end-to-end EKS walkthrough: migration callout, architecture diagram, prerequisites, AOSS collection creation, IAM trust and permissions, data access policy, Pod Identity association, image pull secret, embedding endpoint, verification, teardown, cross-link, and strict docs build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com>
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/examples/aws-opensearch-serverless-values.yaml (1)
19-33:⚠️ Potential issue | 🟠 MajorWire
TAVILY_API_KEYinto the OpenSearch web-mode examples.
configs/config_web_opensearch.ymlenablestavily_web_searchtools by default (lines 129–136), but the example values file omitsTAVILY_API_KEYfromsecretEnv. Users deploying with this config will hit web search failures at runtime unless they manually discover and add the secret separately.🔧 Proposed fix
secretEnv: NVIDIA_API_KEY: NVIDIA_API_KEY + TAVILY_API_KEY: TAVILY_API_KEYAlso add
secretEnvblock to the README and deployment guide snippets and document theTAVILY_API_KEYrequirement alongsideNVIDIA_API_KEY.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/examples/aws-opensearch-serverless-values.yaml` around lines 19 - 33, The `configs/config_web_opensearch.yml` configuration enables `tavily_web_search` tools by default, but the example values files and documentation snippets omit the required `TAVILY_API_KEY` secret, causing runtime failures for users. In deploy/helm/examples/aws-opensearch-serverless-values.yaml (lines 19-33), add `TAVILY_API_KEY: TAVILY_API_KEY` to the `secretEnv` block alongside the existing `NVIDIA_API_KEY` entry. Apply the same change to the `secretEnv` block shown in deploy/helm/README.md (lines 147-163) and docs/source/deployment/aws-opensearch-serverless.md (lines 302-316) to ensure all deployment guide snippets and documentation examples consistently include the `TAVILY_API_KEY` requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md`:
- Around line 705-718: The section heading "4. Confirm the index appears in
AOSS" promises to verify index creation, but the `aws opensearchserverless
list-collections` command only confirms the collection exists, not the index.
Either add an additional command that performs an index-level query (such as
listing indexes within the collection or querying the index directly) to
actually verify the `aiq-smoke` index was created, or rename the section heading
to accurately reflect that it only verifies collection existence rather than
index creation.
- Around line 102-104: Multiple fenced code blocks in the markdown files are
opened but never closed, causing subsequent prose to render as code. In
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md, remove
the dangling ` ```text` fence openers at lines 102-104, 168-170, 255-257,
353-355, 453-455, 544-546, 630-632, 731-733, and 797-799. In
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md, replace the
stray ` ```markdown` fence openers at lines 177-179 and 260-262 with plain
closing fences (` ``` `) to properly terminate the preceding code blocks.
In `@docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md`:
- Around line 69-75: The monkeypatch is targeting the wrong module path. The
adapter imports OpenAI using a local import statement (from openai import
OpenAI) at the module level, so patching "openai.OpenAI" will not intercept the
client instantiation when _embed_texts is called. Change the monkeypatch target
to patch OpenAI in the adapter's local namespace by using the full module path
where OpenAI is imported (sources.knowledge_layer.src.opensearch.adapter.OpenAI)
instead of the original openai module path, so that when the adapter
instantiates the client, it will use the _FakeOpenAI class instead of the real
one.
---
Outside diff comments:
In `@deploy/helm/examples/aws-opensearch-serverless-values.yaml`:
- Around line 19-33: The `configs/config_web_opensearch.yml` configuration
enables `tavily_web_search` tools by default, but the example values files and
documentation snippets omit the required `TAVILY_API_KEY` secret, causing
runtime failures for users. In
deploy/helm/examples/aws-opensearch-serverless-values.yaml (lines 19-33), add
`TAVILY_API_KEY: TAVILY_API_KEY` to the `secretEnv` block alongside the existing
`NVIDIA_API_KEY` entry. Apply the same change to the `secretEnv` block shown in
deploy/helm/README.md (lines 147-163) and
docs/source/deployment/aws-opensearch-serverless.md (lines 302-316) to ensure
all deployment guide snippets and documentation examples consistently include
the `TAVILY_API_KEY` requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 57c67737-b489-4635-a8ae-003f85e32d8d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.secrets.baselineconfigs/config_web_opensearch.ymldeploy/helm/README.mddeploy/helm/examples/aws-opensearch-serverless-values.yamldocs/source/deployment/aws-opensearch-serverless.mddocs/source/deployment/index.mddocs/source/index.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mdpyproject.tomlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/README.mdsources/knowledge_layer/pyproject.tomlsources/knowledge_layer/src/__init__.pysources/knowledge_layer/src/opensearch/__init__.pysources/knowledge_layer/src/opensearch/adapter.pysources/knowledge_layer/src/opensearch/distributed.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/run_adapter_compliance.pytests/knowledge_layer_tests/test_opensearch_adapter.pytests/knowledge_layer_tests/test_opensearch_live.pytests/knowledge_layer_tests/test_opensearch_serverless_live.py
💤 Files with no reviewable changes (12)
- sources/knowledge_layer/pyproject.toml
- sources/knowledge_layer/src/init.py
- sources/knowledge_layer/src/opensearch/init.py
- tests/knowledge_layer_tests/run_adapter_compliance.py
- tests/knowledge_layer_tests/test_opensearch_serverless_live.py
- tests/knowledge_layer_tests/test_opensearch_live.py
- sources/knowledge_layer/README.md
- sources/knowledge_layer/src/register.py
- sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
- sources/knowledge_layer/src/opensearch/distributed.py
- tests/knowledge_layer_tests/test_opensearch_adapter.py
- sources/knowledge_layer/src/opensearch/adapter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/index.mddocs/source/deployment/index.mddocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mddocs/source/deployment/aws-opensearch-serverless.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
**
⚙️ CodeRabbit configuration file
**: # Contributing GuidelinesWe welcome contributions to the NVIDIA AI-Q blueprint. This repository uses a maintainer-reviewed pull request workflow with DCO sign-off, code-owner review, copy-pr-bot mirroring, and GitHub Actions validation.
Before You Start
- Search existing issues and pull requests before opening new work.
- Open an issue or discussion before large design changes, public APIs, deployment changes, or contributor workflow changes.
- Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
- Target the
developbranch unless a maintainer asks you to use a release branch.Pull Requests
- Fork the repository and create a focused branch from
develop.- Make the smallest coherent change and add or update tests for behavior changes.
- Sign off every commit with
git commit -s.- Run the relevant local validation before opening the PR.
- Open a pull request into
developand fill out the PR template with exact validation evidence.- Address review feedback until required checks and code-owner review pass.
Local Validation
Use the narrowest command that covers your change, then include the exact output or workflow link in the PR.
uv sync --group dev uv run ruff check . uv run ruff format --check . uv run pytestFor UI changes:
cd frontends/ui npm ci npm run lint npm run type-check npm run test:ci npm run buildFor deployment changes, run the relevant Helm or compose validation and describe the environment used.
CI and Bot Workflow
AI-Q uses push-triggered GitHub Actions. Pull requests are mirrored by copy-pr-bot to
pull-request/<PR number>branches after a maintainer or configured vetter comments/ok to test, and CI runs on those mirrored branches.Repository owners, organization members, and collaborators can request NVSkills validation by commenting:
/nvskills-ciMaintainers can request bot-driven merge with:
``...
Files:
docs/source/index.mddeploy/helm/README.mdpyproject.tomlconfigs/config_web_opensearch.ymldocs/source/deployment/index.mddeploy/helm/examples/aws-opensearch-serverless-values.yamldocs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.mddocs/source/deployment/aws-opensearch-serverless.mddocs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
deploy/helm/README.mdconfigs/config_web_opensearch.ymldeploy/helm/examples/aws-opensearch-serverless-values.yaml
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}
⚙️ CodeRabbit configuration file
{.github/**,ci/**,.pre-commit-config.yaml,pyproject.toml,uv.lock}: Review automation and packaging changes for least-privilege permissions, pinned versions where appropriate,
copy-pr-bot pull-request/ branch behavior, reproducible uv/npm setup, secret handling, and consistency with
the documented validation matrix.
Files:
pyproject.toml
🪛 LanguageTool
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md
[style] ~360-~360: To elevate your writing, try using a synonym here.
Context: ... "did the worker actually do anything?" hard to confirm. A doc note saves the next p...
(HARD_TO)
docs/superpowers/plans/2026-05-03-opensearch-eks-reference-deployment.md
[style] ~18-~18: Consider an alternative to strengthen your wording.
Context: ...lready cross-links the example file (no further changes here). - `docs/source/deployment/index....
(CHANGES_ADJUSTMENTS)
[style] ~19-~19: Consider an alternative to strengthen your wording.
Context: ...lready lists the new doc on line 30 (no further changes here). - `configs/config_web_opensearch...
(CHANGES_ADJUSTMENTS)
[style] ~20-~20: Consider an alternative to strengthen your wording.
Context: ...ig, already env-substitution-driven (no further changes here). - `sources/knowledge_layer/KNOWL...
(CHANGES_ADJUSTMENTS)
🔇 Additional comments (3)
.secrets.baseline (1)
136-136: LGTM!Also applies to: 181-181, 244-244, 293-293
pyproject.toml (1)
23-24: LGTM!Also applies to: 34-38, 41-41, 46-46, 98-102, 213-218
docs/superpowers/plans/2026-05-05-opensearch-gaps-and-risks.md (1)
157-161: The note's claims are supported by the code. The OpenSearch adapter includes_read_docx_file()and_read_pptx_file()functions that usedocx2txtandpython-pptxrespectively, with file-suffix routing at lines 241–245 insources/knowledge_layer/src/opensearch/adapter.py. The note accurately reflects the current loader coverage for all three claimed formats.
KyleZheng1284
left a comment
KyleZheng1284
left a comment
There was a problem hiding this comment.
The OpenSearch integration is well-scoped and the core ingestion/retrieval path is promising. I’m requesting changes for four issues that affect security and data correctness: basic-auth credentials entering Dask task arguments, collision-prone logical collection naming, caller metadata overwriting adapter-owned fields, and AOSS deletion reporting success after a partial delete. These should be addressable without redesigning the integration.
…s, metadata, AOSS deletes Resolves the four CHANGES_REQUESTED items plus the model-config request on PR NVIDIA-AI-Blueprints#228: - Keep basic-auth credentials out of Dask task arguments: _worker_config now strips username/password, workers resolve OPENSEARCH_USERNAME/OPENSEARCH_PASSWORD from their own environment, and distributed basic-auth ingestion fails fast when they are absent. opensearch_password is now a SecretStr so it redacts in model reprs/dumps. - Make logical-to-physical index mapping collision-safe: append a stable disambiguator derived from the original collection name so distinct logical names map to distinct indexes, and reject index reuse when _meta.collection_name does not match the requested collection. - Prevent caller metadata from overwriting adapter-owned _meta fields (backend, collection_name, embedding config, lifecycle timestamps) by rejecting reserved keys. - Fix AOSS deletion reporting partial success on a stale search view: enumerate the full matching ID set via stable search_after pagination before deleting, and fail rather than return a partial count when enumeration cannot complete. - Align config_web_opensearch.yml models to nemotron-3-super-120b-a12b, matching configs/config_web_frag.yml on develop. Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/ok to test |
@KyleZheng1284, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 4adec45 |
KyleZheng1284
left a comment
KyleZheng1284
left a comment
There was a problem hiding this comment.
I revalidated the latest head after the review fixes. The focused OpenSearch checks pass (36 passed), and the full knowledge-layer suite reports 87 passed, 6 skipped. Four skips are live OpenSearch coverage: two self-hosted tests and two AOSS tests. Because 4adec45 changes the physical index naming and AOSS deletion algorithm, please rerun AIQ_OPENSEARCH_SERVERLESS_LIVE_TESTS=1 uv run pytest tests/knowledge_layer_tests/test_opensearch_serverless_live.py -q against the latest head and add the result to the PR before approval.
The deep_research_agent block in config_web_opensearch.yml carried an unsupported max_loops field, so `nat validate` failed with `functions.deep_research_agent.max_loops: Extra inputs are not permitted` (DeepResearchAgentConfig sets extra="forbid"). Rebuild the block from the canonical configs/config_web_frag.yml on develop: drop max_loops and add enable_citation_verification, source_router_llm, and writer_llm. `nat validate --config_file configs/config_web_opensearch.yml` now passes. Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sources/knowledge_layer/src/opensearch/adapter.py`:
- Around line 757-780: The Dask ingestion path in `_worker_config` silently
overrides `generate_summary` to false, so requests that ask for summaries are
ignored without any notice. Add a submit-time warning or a clear doc note in the
`OpenSearchAdapter` / `run_opensearch_ingestion_task` path explaining that
distributed ingestion does not produce summaries, and make sure the behavior is
documented where `generate_summary` is forced off so callers do not expect
`_apply_dask_ingestion_result` to receive a summary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c5e34e30-8bb5-41cb-a787-7322b63f4930
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.secrets.baselineconfigs/config_web_opensearch.ymlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/opensearch/adapter.pysources/knowledge_layer/src/register.pytests/knowledge_layer_tests/test_opensearch_adapter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
configs/config_web_opensearch.ymlsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/register.pysources/knowledge_layer/src/opensearch/adapter.pytests/knowledge_layer_tests/test_opensearch_adapter.py
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
configs/config_web_opensearch.yml
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/register.pysources/knowledge_layer/src/opensearch/adapter.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
sources/knowledge_layer/src/register.pysources/knowledge_layer/src/opensearch/adapter.pytests/knowledge_layer_tests/test_opensearch_adapter.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/knowledge_layer/src/register.pysources/knowledge_layer/src/opensearch/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_opensearch_adapter.py
🔇 Additional comments (12)
tests/knowledge_layer_tests/test_opensearch_adapter.py (5)
216-249: LGTM!
387-441: LGTM!
451-535: LGTM!
789-824: LGTM!
1037-1074: LGTM!sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md (1)
32-50: LGTM!Also applies to: 185-325, 405-439, 1040-1078
.secrets.baseline (2)
244-244: 🔒 Security & PrivacyVerify the baseline entry still matches the source file.
line_numbermoved from 383 to 567 for the same secret hash. Since.secrets.baselineis the input todetect-secretsin CI, please confirm the secret still exists at the new location and that the old line is no longer present; otherwise this can leave a stale allowlist entry in place.
293-293: LGTM!sources/knowledge_layer/src/register.py (1)
125-128: LGTM!Also applies to: 291-301, 355-355
configs/config_web_opensearch.yml (1)
53-71: LGTM!Also applies to: 150-179
sources/knowledge_layer/src/opensearch/adapter.py (2)
101-107: LGTM!Also applies to: 486-497, 921-926, 1058-1112
336-346: 🗄️ Data Integrity & IntegrationNo issue: the trimming step preserves uniqueness
_trim_index_name(...)drops the tail but appends a new 12-char UUID5 suffix from the full index name, so the earlier 8-char collection disambiguator is not the only collision guard.> Likely an incorrect or invalid review comment.
|
@KyleZheng1284 Re: the AOSS serverless live rerun on the latest head (be31fe9). Ran the serverless live suite against a live Amazon OpenSearch Serverless (SigV4/ Covered: Config validation evidence (per the inline comment): |
The secretEnv placeholder `NVIDIA_API_KEY: NVIDIA_API_KEY` in the AOSS helm example tripped the detect-secrets pre-commit hook under `--all-files` (how CI runs it), failing the Lint and Hooks job. The value is a literal env-var-name placeholder, not a credential; mark it with an allowlist pragma. Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
KyleZheng1284
left a comment
KyleZheng1284
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier findings—the core fixes and updated AOSS live results look good. We’re evaluating this for AI-Q 2.2, so I did one final developer-facing pass of the AWS setup docs. I found three copy/paste issues below that should be fixed so the guide works on a fresh cluster.
…otice Addresses the developer-facing findings from the latest review pass: - AWS AOSS setup docs: the verify/smoke-test commands referenced a non-existent service and label (`svc/aiq-agent`, `app.kubernetes.io/name=aiq-agent`). The chart creates `svc/aiq-backend` with `app=aiq-backend`; update the pod, log, and port-forward commands accordingly. - Smoke test made end-to-end correct: the chat request now sends `conversation-id: smoke` (the backend maps conversation-id to the collection name), poll states use the lowercase API values (`completed`/`processing`), and the expected physical index is `aiq-smoke-<hash>` (naming disambiguator). - Helm example: apply the NGC pull secret to the frontend app too, so a fresh cluster does not leave the frontend in ImagePullBackOff. - Distributed ingestion + summaries: `_worker_config` now logs a warning when a job requests `generate_summary` under Dask (summaries are only produced in local mode), with a matching note in KNOWLEDGE-LAYER-SETUP.md and a test. Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/deployment/aws-opensearch-serverless.md`:
- Around line 382-387: The troubleshooting note for the OpenSearch Serverless
upload flow points at the wrong pods and filters out the relevant error, so
update the log command in the document to inspect the Dask worker pods instead
of the backend pods. Use the existing “Dask worker logs” note as the anchor, and
adjust the kubectl selector in that section so it targets the worker workload
and preserves the SigV4 auth error output without piping through a grep that can
hide it.
In `@sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md`:
- Around line 504-507: The blockquote in KNOWLEDGE-LAYER-SETUP.md contains an
empty quoted line that will trigger markdownlint MD028; remove the blank quoted
line so the note stays as a continuous quote. Update the note text around the
summaries/local ingestion guidance, and keep the blockquote formatting clean in
that section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ce6b372c-6373-451a-a247-eb75dec3be0a
📒 Files selected for processing (6)
.secrets.baselinedeploy/helm/examples/aws-opensearch-serverless-values.yamldocs/source/deployment/aws-opensearch-serverless.mdsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/opensearch/adapter.pytests/knowledge_layer_tests/test_opensearch_adapter.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
deploy/helm/examples/aws-opensearch-serverless-values.yamldocs/source/deployment/aws-opensearch-serverless.mdsources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/opensearch/adapter.pytests/knowledge_layer_tests/test_opensearch_adapter.py
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
deploy/helm/examples/aws-opensearch-serverless-values.yaml
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update the docs under docs/source/ when behavior, configuration, or workflows change
Files:
docs/source/deployment/aws-opensearch-serverless.md
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/deployment/aws-opensearch-serverless.md
{src/aiq_agent/knowledge/**,sources/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/knowledge/**,sources/**}: Review data-source and knowledge-layer changes for optional dependency boundaries, external API error handling,
retry/rate-limit behavior, deterministic tests, and registration consistency. New source packages should include
package metadata, plugin registration when applicable, and source-level tests.
Files:
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.mdsources/knowledge_layer/src/opensearch/adapter.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
sources/knowledge_layer/src/opensearch/adapter.pytests/knowledge_layer_tests/test_opensearch_adapter.py
sources/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
New tools and data sources must be NAT functions registered with
@register_functiondecorator
Files:
sources/knowledge_layer/src/opensearch/adapter.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/knowledge_layer_tests/test_opensearch_adapter.py
🪛 markdownlint-cli2 (0.22.1)
sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md
[warning] 507-507: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (5)
deploy/helm/examples/aws-opensearch-serverless-values.yaml (1)
33-37: LGTM!sources/knowledge_layer/src/opensearch/adapter.py (1)
764-779: LGTM!sources/knowledge_layer/KNOWLEDGE-LAYER-SETUP.md (1)
1044-1067: LGTM!tests/knowledge_layer_tests/test_opensearch_adapter.py (1)
19-19: LGTM!Also applies to: 809-819
.secrets.baseline (1)
244-244: LGTM!Also applies to: 293-293
Addresses the final review pass: - AOSS smoke test troubleshooting: the Dask scheduler and workers run embedded in the backend pod (deploy/entrypoint.py starts them on tcp://localhost), so the backend selector is correct; drop the `grep -i opensearch` filter that could hide a SigV4/credential auth error whose message doesn't contain "opensearch". - KNOWLEDGE-LAYER-SETUP.md: convert the distributed-summary note from a blockquote to a plain paragraph so it is no longer adjacent to the following blockquote, clearing markdownlint MD028 (blank line inside blockquote). Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
docs/superpowers/plans/ contained development planning scratch (writing-plans artifacts), not product documentation. The files were never wired into the docs toctree/build and carried hardcoded personal absolute paths, which violates the repo rule against committing environment-specific paths. Remove them from the PR. Signed-off-by: Felipe Garcia <fdecarvalhop@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
|
/ok to test 0f791f4 |
3 participants
Summary
Adds OpenSearch as a built-in knowledge backend in AIQ 2.0, alongside the
existing LlamaIndex (ChromaDB) and Foundational RAG (Milvus) options.
Selectable through the standard
backend: opensearchfield in workflow YAML;no fork, no custom base image.
OpenSearch is broadly available — self-hosted, and as a managed offering on
every major cloud (Amazon OpenSearch Service, Amazon OpenSearch Serverless,
others). Until now, customers running OpenSearch had to write their own
BaseRetriever/BaseIngestoradapter, rebuild base images to include it,and handle their environment's authentication themselves — including inside
the Dask workers during ingestion. This PR removes that burden and
establishes a plugin pattern that other native backends can follow.
What is included
OpenSearchRetriever+OpenSearchIngestorextendingBaseRetriever/BaseIngestor, registered through the knowledge-layer factory, exposed asa NAT function (
backend: opensearchin YAML).none(development clusters),basic(self-hostedwith username/password),
sigv4(Amazon OpenSearch Service viaaws_service: es, Amazon OpenSearch Serverless viaaws_service: aoss).Pluggable enough for additional auth schemes to slot in as new Literal
values without adapter rewrites.
constructs its own OpenSearch client inside each worker, so credentials
resolve from the worker's environment — works with EKS Pod Identity,
IRSA, SSO, env profiles, and instance profiles. No signer state is
serialized across the cluster.
indexes (
<prefix>-s_<uuid>) inside a single persistent OpenSearchcluster, with 24-hour TTL cleanup driven by
_meta.updated_at. Letsmultiple sessions share one OpenSearch resource without cross-session
leakage.
deploy/helm/examples/aws-opensearch-serverless-values.yamlshows theimage,imagePullSecrets, andsecretEnvshape for a managed-serverlessEKS deployment. The chart's existing schema accepts the override directly —
no chart fork required to point at a custom image or environment.
configs/config_web_opensearch.yml(env-substitution driven; the same file works for self-hosted, managed,
and serverless OpenSearch backends).
docs/source/deployment/aws-opensearch-serverless.md— end-to-endwalkthrough for one concrete cloud (Amazon OpenSearch Serverless on EKS
with Pod Identity), covering architecture, prerequisites, OpenSearch
collection setup, IAM trust policy, data access policy, Pod Identity
association, image pull secret, embedding endpoint, verification, and
cleanup. The same pattern adapts to other cloud or self-hosted OpenSearch
deployments.
Test plan
All paths validated against a real Amazon OpenSearch Serverless collection
(SigV4 via assumed role) prior to PR submission:
tests/knowledge_layer_tests/test_opensearch_adapter.py) — 19 passed.tests/knowledge_layer_tests/test_opensearch_serverless_live.py) —2 passed in 160s. Covers SigV4 health, collection lifecycle, vector
ingest, k-NN retrieval, filtered k-NN, and AOSS-aware delete batching.
nat serveagainst a liveOpenSearch endpoint.
POST /v1/collections/<name>/documents→ embed→ bulk index → status
completed. Verified via direct doc-countquery.
_bulkissued fromthe worker process (verified absent from backend log). Authentication
resolved inside the worker.
wait. Doc count drops to 0 in the target index; other indexes
unaffected.
pypdf→ multi-batchembedding → bulk index in 5.3s. Page-level citations preserved through
retrieval.
<prefix>-s_<uuid>indexappears in OpenSearch alongside API-driven collections.
SPHINXOPTS="-W --keep-going -n")— zero warnings.
CONTRIBUTING.md compliance
Signed-off-by:trailers per the DCO requirement(
git rebase --signoffacross the branch).Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores