Conversation
AnuradhaKaruppiah
left a comment
AnuradhaKaruppiah
left a comment
There was a problem hiding this comment.
Thanks for the contribution
There was a problem hiding this comment.
move configs and data directories within src/nvingest_mcp_rag and add symlinks to them at the top level
Reference:
https://github.com/NVIDIA/NeMo-Agent-Toolkit/tree/develop/examples/A2A/math_assistant_a2a
|
@CodeRabbit review full |
|
@AnuradhaKaruppiah: I'll conduct a full review of this PR which adds the NV-Ingest MCP Example. I'll analyze the new example implementation including the document ingestion tools, semantic search capabilities, and the three operation modes (Direct Agent, MCP Server, and MCP Client). ✅ Actions performedFull review triggered. |
|
/ok to test 741b315 |
|
@KyleZheng1284 You need to sign off your commits. See the instructions here - https://github.com/NVIDIA/NeMo-Agent-Toolkit-Examples/pull/11/checks |
AnuradhaKaruppiah
added
non-breaking
WalkthroughIntroduces the NV-Ingest MCP RAG example with documentation, three configuration modes (Direct Agent, MCP Server, MCP Client), a Python package with document ingestion and semantic search functions, and project metadata. The example enables end-to-end document extraction, vectorization, and retrieval-augmented generation workflows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Agent as ReAct Agent
participant NVIngest as NV-Ingest Service
participant Embedding as Embedding Service
participant Milvus as Milvus VDB
participant MinIO as MinIO Storage
User->>Agent: Document ingestion with VDB upload request
Agent->>NVIngest: Extract text, tables, charts from document
NVIngest-->>Agent: Content chunks
Agent->>Embedding: Embed chunks
Embedding-->>Agent: Embeddings vectors
Agent->>Milvus: Create/validate collection
Agent->>MinIO: Store document metadata
Agent->>Milvus: Upload embeddings & metadata
Milvus-->>Agent: Upload confirmation + chunk count
Agent-->>User: Ingestion complete + statistics
sequenceDiagram
actor User
participant Agent as ReAct Agent
participant Embedding as Embedding Service
participant Milvus as Milvus VDB
participant LLM as LLM
User->>Agent: Semantic search query
Agent->>Embedding: Embed query text
Embedding-->>Agent: Query embedding vector
Agent->>Milvus: Validate collection exists
Agent->>Milvus: Top-k similarity search
Milvus-->>Agent: Retrieved document chunks
Agent->>LLM: Reason over retrieved context
LLM-->>Agent: Generated response
Agent-->>User: Search results + LLM reasoning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
examples/nvingest_mcp/configs/nvingest_mcp_server.yml (1)
61-62: Hardcoded MinIO credentials are acceptable for examples but should be parameterized for production.The default
minioadmincredentials are fine for local development/examples. Consider adding a comment noting these should be changed for production use, or use environment variable substitution similar toNVIDIA_API_KEY.- minio_access_key: minioadmin - minio_secret_key: minioadmin + minio_access_key: ${MINIO_ACCESS_KEY:-minioadmin} + minio_secret_key: ${MINIO_SECRET_KEY:-minioadmin}examples/nvingest_mcp/src/nvingest_mcp_rag/register.py (3)
78-79: Prefix unusedbuilderparameter with underscore to satisfy linter.The
builderparameter is required by the NAT framework signature but unused. Prefix with underscore to indicate intentional non-use.@register_function(config_type=DocumentIngestConfig) -async def nvingest_document_ingest_function(config: DocumentIngestConfig, builder: Builder): +async def nvingest_document_ingest_function(config: DocumentIngestConfig, _builder: Builder):Same applies to Lines 246 and 415.
162-164: Use!sconversion flag instead of explicitstr()call.Replace
{str(e)}with{e!s}for more idiomatic f-string formatting.- return f"Error ingesting document: {str(e)}" + return f"Error ingesting document: {e!s}"Same applies to Lines 374 and 501.
457-479: Consider closing the MilvusClient connection after use.The
MilvusClientis created but never explicitly closed, which could lead to connection leaks under sustained load.# Connect to Milvus and search logger.info(f"Connecting to Milvus at {config.milvus_uri}...") client = MilvusClient(uri=config.milvus_uri) + try: # Check if collection exists collections = client.list_collections() # ... rest of search logic ... + finally: + client.close()Alternatively, if
MilvusClientsupports context manager protocol, usewith MilvusClient(...) as client:.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
examples/nvingest_mcp/data/embedded_table.pdfis excluded by!**/*.pdfexamples/nvingest_mcp/data/multimodal_test.pdfis excluded by!**/*.pdfexamples/nvingest_mcp/data/test-page-form.pdfis excluded by!**/*.pdf
📒 Files selected for processing (8)
examples/nvingest_mcp/README.md(1 hunks)examples/nvingest_mcp/configs/nvingest_agent_direct.yml(1 hunks)examples/nvingest_mcp/configs/nvingest_mcp_client.yml(1 hunks)examples/nvingest_mcp/configs/nvingest_mcp_server.yml(1 hunks)examples/nvingest_mcp/pyproject.toml(1 hunks)examples/nvingest_mcp/src/.gitignore(1 hunks)examples/nvingest_mcp/src/nvingest_mcp_rag/__init__.py(1 hunks)examples/nvingest_mcp/src/nvingest_mcp_rag/register.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
examples/nvingest_mcp/src/nvingest_mcp_rag/register.py
79-79: Unused function argument: builder
(ARG001)
164-164: Use explicit conversion flag
Replace with conversion flag
(RUF010)
246-246: Unused function argument: builder
(ARG001)
367-370: Consider moving this statement to an else block
(TRY300)
374-374: Use explicit conversion flag
Replace with conversion flag
(RUF010)
415-415: Unused function argument: builder
(ARG001)
501-501: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (8)
examples/nvingest_mcp/src/.gitignore (1)
1-2: Good practice for Python packaging.The
.gitignorepattern correctly excludes setuptools-generated egg-info metadata, which is standard for Python projects.examples/nvingest_mcp/src/nvingest_mcp_rag/__init__.py (1)
1-19: LGTM!Standard package initializer with appropriate license header and module docstring.
examples/nvingest_mcp/README.md (1)
18-318: Well-structured documentation with clear usage instructions.The README effectively covers:
- Prerequisites with specific service ports
- Three operation modes with clear use-case differentiation
- Important networking note (Line 244-246) explaining Docker internal vs host embedding URLs
- Troubleshooting health checks
examples/nvingest_mcp/configs/nvingest_mcp_client.yml (1)
31-61: LGTM!Clean MCP client configuration with:
- Proper environment variable substitution for API key
- Appropriate LLM settings (temperature 0.0 for deterministic behavior)
- Resilience settings with retry parsing enabled
examples/nvingest_mcp/configs/nvingest_agent_direct.yml (1)
30-91: Configuration is consistent with MCP server config.The direct agent configuration properly mirrors the server configuration with the same extraction options, VDB settings, and workflow parameters. The embedding URL difference (Docker internal for VDB upload vs localhost for search) aligns with the documented networking setup.
examples/nvingest_mcp/src/nvingest_mcp_rag/register.py (3)
40-173: Well-structured function registration with clear configuration separation.The
DocumentIngestConfigand corresponding function demonstrate good practices:
- Clear Pydantic field definitions with descriptions
- Proper async handling with
asyncio.to_threadfor blocking operations- Graceful error handling returning user-friendly messages suitable for LLM consumption
- Comprehensive logging for debugging
175-383: VDB ingestion function follows the same clean pattern.Good implementation with:
- Verbose configuration logging for visibility (Lines 281-292)
- Proper handling of partial failures (Lines 358-365)
- Chunk counting for user feedback
385-509: Semantic search function is well-implemented with good error handling.The function properly:
- Validates collection existence before searching (Lines 465-471)
- Provides helpful messages when collection is empty or missing
- Returns structured results with document count
examples/nvingest_mcp/src/nvingest_mcp_rag/configs/nvingest_mcp_server.yml
Show resolved
Hide resolved
examples/nvingest_mcp/pyproject.toml
Outdated
| name = "nvingest_mcp" | ||
| version = "0.1.0" | ||
| dependencies = [ | ||
| "nvidia-nat[mcp,langchain]", |
There was a problem hiding this comment.
Pls pin the version to ensure the example continues to work consistently. Use two digits for NAT nvidia-nat[mcp,langchain]~=1.3
Other packages may also need to be pinned (as there is no uv.lock here)
…in nvidia-nat~=1.3, update retry syntax
|
@KyleZheng1284 do you have interest in updating this Examples PR? |
3 participants
Description
This PR adds a new example demonstrating how to use NeMo Agent Toolkit with NV-Ingest for document processing and retrieval workflows. The example exposes NV-Ingest capabilities as MCP tools that AI agents can use to ingest documents, build knowledge bases, and perform semantic search.
Features
Document Ingestion Tools
Three Operation Modes
Sample Documents: Includes test PDFs for demonstrating multimodal extraction (text, tables, charts) that we use in the nv-ingest repo
Check out the README for prerequisites, installation, and detailed usage instructions.
This is assuming you have a local deployed version of nv-ingest
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.