Skip to content

add NVIDIA NIM support with embedder and LLM providers#53

Merged
laxmanclo merged 3 commits into
vektori-ai:mainfrom
sankalphs:main
Apr 14, 2026
Merged

add NVIDIA NIM support with embedder and LLM providers#53
laxmanclo merged 3 commits into
vektori-ai:mainfrom
sankalphs:main

Conversation

@sankalphs

@sankalphs sankalphs commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Added support for NVIDIA NIM (NVIDIA Inference Microservices) as a model provider for both embeddings and LLM-based fact extraction. This enables users to use GPU-optimized models hosted on NVIDIA's platform.

Changes Made

  • New file: vektori/models/nvidia.py - Complete NVIDIA NIM provider implementation
    NvidiaEmbedder class with Matryoshka embedding support (384-2048 dimensions)
    NvidiaLLM class for fact/episode extraction
    Automatic namespace handling (nvidia/ prefix for NVIDIA models, full path for third-party models)
    Support for input_type parameter on asymmetric embedding models
  • Updated: vektori/models/factory.py - Added NVIDIA to embedding and LLM registries
  • Updated: pyproject.toml - Added nvidia optional dependency (openai>=1.12)
  • Updated: README.md - Added NVIDIA NIM documentation section
  • Updated: tests/unit/test_factory.py - Added comprehensive NVIDIA tests

Testing

✅ All 53 unit tests pass
✅ Integration tests verify actual NVIDIA API calls work correctly
✅ Matryoshka dimensions verified (384, 512, 768, 1024, 2048)
✅ Factory creation tests for all model types

Notes

  • BGE-M3 via NVIDIA NIM currently returns 500 errors (server-side issue) - users should use local BGE provider instead: vektori[embedding_model="bge:BAAI/bge-m3"]
  • NVIDIA NIM uses OpenAI-compatible API (https://integrate.api.nvidia.com/v1)

Summary by CodeRabbit

  • New Features

    • Added support for NVIDIA NIM GPU‑optimized embeddings and LLMs via an OpenAI‑compatible API, with configurable embedding dimensions and model name handling.
  • Documentation

    • README updated with usage examples for NVIDIA embedding and extraction models, including dimension customization and example model strings.
  • Chores

    • New optional "nvidia" dependency group (includes openai>=1.12) and added to the combined extras.
  • Tests

    • Added unit tests validating NVIDIA provider creation and expected behavior.

Copilot AI review requested due to automatic review settings April 13, 2026 15:35
@coderabbitai

coderabbitai Bot commented Apr 13, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Added NVIDIA NIM support: new NvidiaEmbedder and NvidiaLLM providers (via OpenAI-compatible client), registered "nvidia" in model factory, added optional nvidia dependency, updated README with usage examples, and expanded unit tests covering factory resolution for NVIDIA providers.

Changes

Cohort / File(s) Summary
Documentation
README.md
Added "NVIDIA NIM - GPU-optimized models" section with Python usage examples showing embedding_model="nvidia:...", optional embedding_dimension, extraction model examples, and third-party NVIDIA-hosted model usage.
Dependency Configuration
pyproject.toml
Added optional dependency group nvidia = ["openai>=1.12"] and included openai>=1.12 in the all extra.
Provider Implementation
vektori/models/nvidia.py
New module implementing NvidiaEmbedder and NvidiaLLM: lazy AsyncOpenAI client init, API key env fallback, Matryoshka embedding-dimension support, ordered batch embeddings, and LLM generate with JSON-mode fallback and model-name normalization.
Factory Registry
vektori/models/factory.py
Registered "nvidia" in EMBEDDING_REGISTRY and LLM_REGISTRY mapping to vektori.models.nvidia.NvidiaEmbedder / NvidiaLLM for nvidia: model resolution.
Tests
tests/unit/test_factory.py
Expanded tests to assert create_embedder("nvidia") returns NvidiaEmbedder with default model, create_embedder("nvidia:<model>", dimensions=1024) sets dimensions, create_llm("nvidia:<model>") normalizes model to nvidia/..., and that "nvidia" is present in LLM_REGISTRY.

Sequence Diagram

sequenceDiagram
    actor User
    participant Factory as Factory
    participant NvidiaEmbedder as NvidiaEmbedder
    participant OpenAI as AsyncOpenAI Client
    participant NVIDIA as NVIDIA NIM API

    User->>Factory: create_embedder("nvidia")
    Factory->>Factory: lookup "nvidia" in EMBEDDING_REGISTRY
    Factory->>NvidiaEmbedder: instantiate (model, dimensions)
    Factory-->>User: NvidiaEmbedder instance

    User->>NvidiaEmbedder: embed_batch(texts)
    NvidiaEmbedder->>NvidiaEmbedder: lazy init AsyncOpenAI client (api_key)
    NvidiaEmbedder->>OpenAI: client.embeddings.create(model=..., input=[...], extra_body={...})
    OpenAI->>NVIDIA: POST /v1/embeddings
    NVIDIA-->>OpenAI: embedding vectors (with indices)
    OpenAI-->>NvidiaEmbedder: response
    NvidiaEmbedder->>NvidiaEmbedder: sort by index, preserve original order
    NvidiaEmbedder-->>User: list[list[float]]
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 I nibble docs and hop through code,
GPUs hum where embeddings flowed,
NVIDIA models, sleek and bright,
Dimensions bend from day to night,
A rabbit cheers for vectors in flight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding NVIDIA NIM support as a new model provider with both embedder and LLM capabilities, which aligns with the comprehensive changes across five files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
tests/unit/test_factory.py (1)

69-74: Add NVIDIA LLM factory coverage alongside the embedder test.

Line 69 validates the embedder registration path, but the PR also registers NVIDIA in LLM_REGISTRY (vektori/models/factory.py Line 35). Add a matching LLM factory assertion to guard both provider mappings.

Proposed test addition
 def test_create_nvidia_embedder_default_model():
     from vektori.models.nvidia import NvidiaEmbedder, DEFAULT_EMBEDDING_MODEL

     embedder = create_embedder("nvidia")
     assert isinstance(embedder, NvidiaEmbedder)
     assert embedder.model == DEFAULT_EMBEDDING_MODEL  # "nvidia/llama-nemotron-embed-1b-v2"
+
+
+def test_create_nvidia_llm_default_model():
+    from vektori.models.nvidia import NvidiaLLM, DEFAULT_LLM_MODEL
+
+    llm = create_llm("nvidia")
+    assert isinstance(llm, NvidiaLLM)
+    assert llm.model == DEFAULT_LLM_MODEL
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_factory.py` around lines 69 - 74, Add a parallel LLM factory
assertion to the embedder test: import create_llm and LLM_REGISTRY from
vektori.models.factory, assert "nvidia" is present in LLM_REGISTRY, call
create_llm("nvidia") and assert the result is not None and that its class name
includes "Nvidia" (e.g., result.__class__.__name__.contains("Nvidia")) to ensure
the LLM mapping for the "nvidia" provider is registered and returns an instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@vektori/models/nvidia.py`:
- Around line 104-118: Validate the dimensions parameter in __init__: ensure if
dimensions is not None it is an int and >0 (and optionally belongs to the set of
supported sizes for the selected model), otherwise raise a ValueError; store
only validated values in self._custom_dims so downstream uses (places that read
self._custom_dims or call methods that rely on dimensions) cannot proceed with
invalid config. Also add the same validation/check where dimensions are consumed
(any methods that read self._custom_dims) to fail fast if __init__ was bypassed.
- Around line 292-299: The current try/except around
client.chat.completions.create unconditionally retries without response_format
on any exception; change it so you only fallback when the error clearly
indicates the model doesn't support the JSON response_format (e.g., inspect the
caught exception from client.chat.completions.create and check its type or
message for keywords like "response_format", "json", "json_object", or an
SDK/model error code that denotes unsupported response format), and for all
other exceptions (auth, network, rate-limit, etc.) re-raise the exception
instead of retrying; keep references to kwargs["response_format"] and the same
client.chat.completions.create call, remove the unconditional del/retry, and
perform the conditional delete-and-retry only when the inspected error matches
the JSON-mode incompatibility signature.

---

Nitpick comments:
In `@tests/unit/test_factory.py`:
- Around line 69-74: Add a parallel LLM factory assertion to the embedder test:
import create_llm and LLM_REGISTRY from vektori.models.factory, assert "nvidia"
is present in LLM_REGISTRY, call create_llm("nvidia") and assert the result is
not None and that its class name includes "Nvidia" (e.g.,
result.__class__.__name__.contains("Nvidia")) to ensure the LLM mapping for the
"nvidia" provider is registered and returns an instance.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9eeb607d-c323-4bf0-9b28-0931cf8b0462

📥 Commits

Reviewing files that changed from the base of the PR and between 2500816 and 262368f.

📒 Files selected for processing (5)
  • README.md
  • pyproject.toml
  • tests/unit/test_factory.py
  • vektori/models/factory.py
  • vektori/models/nvidia.py

Comment thread vektori/models/nvidia.py
Comment thread vektori/models/nvidia.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new NVIDIA NIM model provider to Vektori so users can run embeddings and extraction via NVIDIA’s OpenAI-compatible inference endpoint.

Changes:

  • Introduces NvidiaEmbedder and NvidiaLLM providers (OpenAI SDK with NVIDIA base URL) with model normalization and Matryoshka dimension support.
  • Registers the new provider in the model factory for both embedding and LLM creation.
  • Updates docs/tests and project metadata to expose the new provider.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
vektori/models/nvidia.py New NVIDIA NIM embedding + LLM provider implementations.
vektori/models/factory.py Adds nvidia to embedder/LLM registries.
tests/unit/test_factory.py Adds a basic factory creation test for the NVIDIA embedder.
README.md Documents how to configure NVIDIA NIM models in Vektori.
pyproject.toml Adds an (currently redundant) nvidia optional dependency entry and updates all.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread vektori/models/nvidia.py
Comment on lines +95 to +96
384, 512, 768, 1024, or 2048. Use the dimensions parameter or
embedding_dimension config to set custom dimensions.

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring says Matryoshka dimensions can be set via the embedding_dimension config, but the embedder only accepts dimensions and nothing in Vektori/create_embedder() passes embedding_dimension through. Either plumb VektoriConfig.embedding_dimension into this provider (or factory) or adjust the docstring to avoid implying it works today.

Suggested change
384, 512, 768, 1024, or 2048. Use the dimensions parameter or
embedding_dimension config to set custom dimensions.
384, 512, 768, 1024, or 2048. Use the dimensions parameter to
set custom dimensions.

Copilot uses AI. Check for mistakes.
Comment thread vektori/models/nvidia.py
Comment on lines +187 to +195
# Some NVIDIA embedding models require input_type parameter
# "passage" is used for indexing, "query" for querying
# We default to "passage" for general embedding use
if self.model in {
"nvidia/llama-nemotron-embed-1b-v2",
"nvidia/llama-nemotron-embed-vl-1b-v2",
}:
extra_body["input_type"] = "passage"

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

input_type is hard-coded to "passage" for models that distinguish between indexing (passage) vs querying (query). Vektori uses the same embedder for both ingestion and retrieval queries, so this will also embed user queries as "passage" and can degrade retrieval quality for asymmetric models. Consider making input_type configurable (e.g., constructor arg) and/or exposing separate methods/instances for passage vs query embeddings.

Copilot uses AI. Check for mistakes.
Comment thread vektori/models/nvidia.py
Comment on lines +292 to +299
# Try JSON mode first (for structured extraction)
try:
kwargs["response_format"] = {"type": "json_object"}
response = await client.chat.completions.create(**kwargs)
except Exception:
# Fallback if model doesn't support JSON mode
del kwargs["response_format"]
response = await client.chat.completions.create(**kwargs)

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generate() retries without response_format on any exception. This can mask real failures (auth/rate limit/network) and can double-request/cost on transient errors. Prefer only falling back for the specific "response_format not supported"/400-type error (similar to the GitHub provider’s pattern) and re-raise unexpected exceptions.

Copilot uses AI. Check for mistakes.
Comment thread README.md
Comment on lines +303 to +306
# NVIDIA embedding models (Matryoshka: 384-2048 dimensions)
v = Vektori(
embedding_model="nvidia:llama-nemotron-embed-1b-v2",
embedding_dimension=1024, # Optional: 384, 512, 768, 1024, or 2048

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README suggests embedding_dimension=1024 will control NVIDIA Matryoshka output size, but the current code path (Vektori -> create_embedder) doesn’t pass embedding_dimension into NvidiaEmbedder(dimensions=...). Either update the wiring so this example works as written, or adjust the docs to show how to set dimensions (e.g., via provider kwargs/config).

Suggested change
# NVIDIA embedding models (Matryoshka: 384-2048 dimensions)
v = Vektori(
embedding_model="nvidia:llama-nemotron-embed-1b-v2",
embedding_dimension=1024, # Optional: 384, 512, 768, 1024, or 2048
# NVIDIA embedding models
# Note: llama-nemotron-embed-1b-v2 supports Matryoshka output sizes,
# but `embedding_dimension=` is not currently forwarded by this Vektori code path.
v = Vektori(
embedding_model="nvidia:llama-nemotron-embed-1b-v2",

Copilot uses AI. Check for mistakes.
Comment thread tests/unit/test_factory.py Outdated
Comment on lines +69 to +74
def test_create_nvidia_embedder_default_model():
from vektori.models.nvidia import NvidiaEmbedder, DEFAULT_EMBEDDING_MODEL

embedder = create_embedder("nvidia")
assert isinstance(embedder, NvidiaEmbedder)
assert embedder.model == DEFAULT_EMBEDDING_MODEL # "nvidia/llama-nemotron-embed-1b-v2"

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description claims “comprehensive NVIDIA tests” (including integration tests and Matryoshka dimension verification), but in this PR’s changes the only NVIDIA test added is a factory default-model check. Either add the missing unit/integration tests that exercise the new provider behavior, or update the PR description/testing notes to match what’s actually included.

Copilot uses AI. Check for mistakes.
Comment thread pyproject.toml
Comment on lines 35 to +43
[project.optional-dependencies]
postgres = ["asyncpg>=0.29", "pgvector>=0.2"]
neo4j = ["neo4j>=5.14"]
qdrant = ["qdrant-client>=1.7"]
anthropic = ["anthropic>=0.20", "voyageai>=0.2"]
sentence-transformers = ["sentence-transformers>=2.6"]
bge = ["FlagEmbedding>=1.2"]
litellm = ["litellm>=1.30"]
nvidia = ["openai>=1.12"] # NVIDIA NIM uses OpenAI-compatible API

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openai>=1.12 is already a required base dependency, so adding it again under the nvidia extra (and again in the all extra) is redundant and can mislead readers into thinking OpenAI is optional unless .[nvidia] is installed. Consider removing the nvidia extra entirely (or, if the goal is to make OpenAI optional, moving it out of base deps and updating providers accordingly).

Copilot uses AI. Check for mistakes.

@sankalphs sankalphs left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made changes suggested by coderabbit

@laxmanclo

Copy link
Copy Markdown
Contributor

Hey @sankalphs , thanks so much for this contribution! The code looks really solid and the integration is clean
We can merge it after you can look into the the CI/test failing issue

@sankalphs

Copy link
Copy Markdown
Contributor Author

@laxmanclo fixed the CI error

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (9)
tests/unit/test_factory.py (3)

77-80: Use consistent assertion style.

Line 80 uses Yoda-style assertion ("literal" == variable) which is inconsistent with line 75 and other tests in this file that use variable == "literal".

🔧 Proposed fix
 def test_create_nvidia_llm_default_model():
     llm = create_llm("nvidia")
     assert isinstance(llm, NvidiaLLM)
-    assert "nvidia/llama-3.3-nemotron-super-49b-v1" == llm.model
+    assert llm.model == "nvidia/llama-3.3-nemotron-super-49b-v1"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_factory.py` around lines 77 - 80, The test
test_create_nvidia_llm_default_model uses a Yoda-style assertion; change the
equality check to match the file's consistent style by asserting llm.model ==
"nvidia/llama-3.3-nemotron-super-49b-v1" (keep the other assertions as-is: llm =
create_llm("nvidia") and isinstance(llm, NvidiaLLM)).

82-89: Consider consolidating redundant test logic.

This test overlaps significantly with test_create_nvidia_llm_default_model — both verify that create_llm("nvidia") returns a valid instance. The class name substring check (line 87-88) is also weaker than the isinstance check in line 79. Consider whether this test adds sufficient value or could be merged.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_factory.py` around lines 82 - 89, The test
test_nvidia_llm_registered duplicates assertions already covered in
test_create_nvidia_llm_default_model; remove redundancy by either deleting
test_nvidia_llm_registered or merging its unique check into
test_create_nvidia_llm_default_model: ensure LLM_REGISTRY contains "nvidia" and
that create_llm("nvidia") returns the expected type using the stronger
isinstance assertion (matching the class used in the other test) rather than the
weaker substring check of llm.__class__.__name__.

67-70: Missing blank line between test functions.

PEP8 requires two blank lines between top-level function definitions. There should be a blank line before test_create_nvidia_embedder_custom_dimensions.

🔧 Proposed fix
     assert embedder.model == DEFAULT_EMBEDDING_MODEL
 
+
 def test_create_nvidia_embedder_custom_dimensions():
     embedder = create_embedder("nvidia:llama-nemotron-embed-1b-v2", dimensions=1024)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_factory.py` around lines 67 - 70, Add a single blank line
before the top-level test function test_create_nvidia_embedder_custom_dimensions
to satisfy PEP8's requirement of two blank lines between top-level function
definitions; locate the test function definition (def
test_create_nvidia_embedder_custom_dimensions():) near other tests that call
create_embedder and instantiate NvidiaEmbedder, and insert the blank line
immediately above it.
vektori/models/nvidia.py (6)

164-176: Consider extracting dimension validation to a helper method.

The same validation logic for _custom_dims is repeated in __init__ (lines 119-130), here in the dimension property (lines 165-176), and in embed_batch (lines 211-222). While this defensive approach ensures fail-fast behavior, extracting to a private _validate_custom_dims() method would reduce duplication and ensure consistent validation.

🔧 Proposed refactor
+    def _validate_custom_dims(self) -> None:
+        """Validate _custom_dims if set."""
+        if self._custom_dims is None:
+            return
+        if not isinstance(self._custom_dims, int):
+            raise ValueError(
+                f"_custom_dims must be an integer, got {type(self._custom_dims).__name__}"
+            )
+        if self._custom_dims <= 0:
+            raise ValueError(f"_custom_dims must be positive, got {self._custom_dims}")
+        supported_dims = {384, 512, 768, 1024, 2048}
+        if self._custom_dims not in supported_dims:
+            raise ValueError(
+                f"_custom_dims must be one of {sorted(supported_dims)}, got {self._custom_dims}"
+            )
+
     `@property`
     def dimension(self) -> int:
-        # Validate _custom_dims in case __init__ was bypassed
-        if self._custom_dims is not None:
-            if not isinstance(self._custom_dims, int):
-                raise ValueError(
-                    f"_custom_dims must be an integer, got {type(self._custom_dims).__name__}"
-                )
-            ...
+        self._validate_custom_dims()
         if self._custom_dims is not None and self.model in MATRYOSHKA_MODELS:
             return self._custom_dims
         return EMBEDDING_DIMENSIONS.get(self.model, 2048)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@vektori/models/nvidia.py` around lines 164 - 176, Extract the repeated
validation logic for _custom_dims into a single private helper (e.g.
_validate_custom_dims()) and call it from __init__, the dimension property, and
embed_batch to remove duplication; _validate_custom_dims() should check type is
int, >0, and membership in supported_dims = {384,512,768,1024,2048} and raise
the same ValueError messages so behavior remains identical, and replace the
in-place checks in __init__, dimension, and embed_batch with calls to this
helper.

356-356: Add defensive check for empty response choices.

If the API returns an empty choices list (edge case, but possible with certain errors or rate limits), this line will raise an IndexError. Consider adding a guard.

🛡️ Proposed fix
-        return response.choices[0].message.content or ""
+        if not response.choices:
+            logger.warning("NVIDIA API returned empty choices")
+            return ""
+        return response.choices[0].message.content or ""
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@vektori/models/nvidia.py` at line 356, Add a defensive guard before accessing
response.choices[0] to avoid IndexError when the API returns an empty choices
list: check that response.choices is truthy and has at least one element and
that response.choices[0].message exists, then return
response.choices[0].message.content or an empty string; otherwise return an
empty string (or raise a clear exception). Update the return site that currently
does "return response.choices[0].message.content or \"\"" to perform this
guarded check (referencing response.choices and response.choices[0].message).

226-233: Hardcoded input_type="passage" may affect retrieval quality.

For asymmetric embedding models, using "passage" for both indexing and querying can degrade retrieval quality. When embedding queries for search, input_type="query" is typically preferred. Consider:

  1. Adding an input_type parameter to embed_batch
  2. Or providing a separate embed_query method for query-time embeddings

This is noted in the docstring (lines 227-228) but the current implementation always defaults to "passage".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@vektori/models/nvidia.py` around lines 226 - 233, The NVIDIA model branch
hardcodes extra_body["input_type"]="passage" which forces passage-style
embeddings for both indexing and queries; modify the embedding API by adding an
input_type parameter to embed_batch (default "passage") and pass that value into
extra_body for the NVIDIA models (referencing embed_batch, extra_body, and the
model name checks), and/or add a convenience embed_query method that calls
embed_batch(input_type="query") so callers can request query-style embeddings;
update any callers/tests and the docstring to reflect the new parameter or new
method.

284-305: Consider extracting shared client initialization.

The _get_client method is nearly identical between NvidiaEmbedder and NvidiaLLM. Consider extracting to a module-level helper function or a shared mixin to reduce duplication.

🔧 Proposed helper function
def _create_nvidia_client(api_key: str | None):
    """Create AsyncOpenAI client configured for NVIDIA API."""
    try:
        from openai import AsyncOpenAI
    except ImportError as e:
        raise ImportError(
            "openai package required for NVIDIA NIM: pip install openai>=1.12"
        ) from e

    resolved_key = api_key or os.environ.get("NVIDIA_API_KEY")
    if not resolved_key:
        raise ValueError(
            "NVIDIA API key required. Set NVIDIA_API_KEY environment variable "
            "or pass api_key parameter."
        )

    return AsyncOpenAI(api_key=resolved_key, base_url=NVIDIA_BASE_URL)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@vektori/models/nvidia.py` around lines 284 - 305, Extract the duplicated
client-init logic used in NvidiaEmbedder._get_client and NvidiaLLM._get_client
into a single module-level helper (e.g. _create_nvidia_client(api_key: str |
None)), which should perform the AsyncOpenAI import, resolve api_key from the
parameter or os.environ["NVIDIA_API_KEY"], raise the same ImportError/ValueError
messages, and return AsyncOpenAI(api_key=resolved_key,
base_url=NVIDIA_BASE_URL); then update both NvidiaEmbedder._get_client and
NvidiaLLM._get_client to call this helper and assign/return its result (preserve
lazy initialization of self._client and all existing error messages).

322-326: Consider making temperature configurable.

The temperature is hardcoded to 0.1, which is reasonable for extraction tasks but limits flexibility. Other LLM providers in the codebase may allow temperature configuration. Consider adding a temperature parameter to __init__ or generate() for consistency.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@vektori/models/nvidia.py` around lines 322 - 326, The temperature is
hardcoded in the kwargs, so make it configurable by adding a temperature
parameter (default 0.1) to the class constructor (__init__) or to the generate()
method, persist it as self.temperature when added to __init__, and replace the
hardcoded value in the kwargs block ("temperature": 0.1) with self.temperature
(or the generate() arg). Update the relevant function signature(s) and any
docstring/comments for the class/method (e.g., __init__, generate, kwargs,
self.model) so callers can override temperature consistently with other
providers.

119-131: Consider validating model compatibility when dimensions is specified.

The validation checks if dimensions is a supported Matryoshka size, but doesn't verify whether the specified model actually supports Matryoshka embeddings. A user could specify dimensions=1024 for a non-Matryoshka model (e.g., nvidia/nv-embed-v1), and the parameter would be silently ignored at embedding time (lines 223-224). Failing fast here would prevent confusion.

🔧 Proposed fix
             if dimensions not in supported_dims:
                 raise ValueError(
                     f"dimensions must be one of {sorted(supported_dims)}, got {dimensions}"
                 )
+            # Check model supports Matryoshka dimensions
+            if self.model not in MATRYOSHKA_MODELS:
+                raise ValueError(
+                    f"Model '{self.model}' does not support Matryoshka dimensions. "
+                    f"Supported models: {sorted(MATRYOSHKA_MODELS)}"
+                )
         self._custom_dims = dimensions
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@vektori/models/nvidia.py` around lines 119 - 131, When dimensions is provided
you must also validate that the selected model supports Matryoshka embeddings
rather than only checking supported_dims: after the existing type/size checks
(dimensions, supported_dims) add a compatibility check against the current model
(e.g., self._model) and/or a known set of Matryoshka-capable model names (or a
helper like _is_matryoshka_model(model)); if the model does not support
Matryoshka raise a ValueError explaining that dimensions is only valid for
Matryoshka models, and only set self._custom_dims if the compatibility check
passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/test_factory.py`:
- Around line 77-80: The test test_create_nvidia_llm_default_model uses a
Yoda-style assertion; change the equality check to match the file's consistent
style by asserting llm.model == "nvidia/llama-3.3-nemotron-super-49b-v1" (keep
the other assertions as-is: llm = create_llm("nvidia") and isinstance(llm,
NvidiaLLM)).
- Around line 82-89: The test test_nvidia_llm_registered duplicates assertions
already covered in test_create_nvidia_llm_default_model; remove redundancy by
either deleting test_nvidia_llm_registered or merging its unique check into
test_create_nvidia_llm_default_model: ensure LLM_REGISTRY contains "nvidia" and
that create_llm("nvidia") returns the expected type using the stronger
isinstance assertion (matching the class used in the other test) rather than the
weaker substring check of llm.__class__.__name__.
- Around line 67-70: Add a single blank line before the top-level test function
test_create_nvidia_embedder_custom_dimensions to satisfy PEP8's requirement of
two blank lines between top-level function definitions; locate the test function
definition (def test_create_nvidia_embedder_custom_dimensions():) near other
tests that call create_embedder and instantiate NvidiaEmbedder, and insert the
blank line immediately above it.

In `@vektori/models/nvidia.py`:
- Around line 164-176: Extract the repeated validation logic for _custom_dims
into a single private helper (e.g. _validate_custom_dims()) and call it from
__init__, the dimension property, and embed_batch to remove duplication;
_validate_custom_dims() should check type is int, >0, and membership in
supported_dims = {384,512,768,1024,2048} and raise the same ValueError messages
so behavior remains identical, and replace the in-place checks in __init__,
dimension, and embed_batch with calls to this helper.
- Line 356: Add a defensive guard before accessing response.choices[0] to avoid
IndexError when the API returns an empty choices list: check that
response.choices is truthy and has at least one element and that
response.choices[0].message exists, then return
response.choices[0].message.content or an empty string; otherwise return an
empty string (or raise a clear exception). Update the return site that currently
does "return response.choices[0].message.content or \"\"" to perform this
guarded check (referencing response.choices and response.choices[0].message).
- Around line 226-233: The NVIDIA model branch hardcodes
extra_body["input_type"]="passage" which forces passage-style embeddings for
both indexing and queries; modify the embedding API by adding an input_type
parameter to embed_batch (default "passage") and pass that value into extra_body
for the NVIDIA models (referencing embed_batch, extra_body, and the model name
checks), and/or add a convenience embed_query method that calls
embed_batch(input_type="query") so callers can request query-style embeddings;
update any callers/tests and the docstring to reflect the new parameter or new
method.
- Around line 284-305: Extract the duplicated client-init logic used in
NvidiaEmbedder._get_client and NvidiaLLM._get_client into a single module-level
helper (e.g. _create_nvidia_client(api_key: str | None)), which should perform
the AsyncOpenAI import, resolve api_key from the parameter or
os.environ["NVIDIA_API_KEY"], raise the same ImportError/ValueError messages,
and return AsyncOpenAI(api_key=resolved_key, base_url=NVIDIA_BASE_URL); then
update both NvidiaEmbedder._get_client and NvidiaLLM._get_client to call this
helper and assign/return its result (preserve lazy initialization of
self._client and all existing error messages).
- Around line 322-326: The temperature is hardcoded in the kwargs, so make it
configurable by adding a temperature parameter (default 0.1) to the class
constructor (__init__) or to the generate() method, persist it as
self.temperature when added to __init__, and replace the hardcoded value in the
kwargs block ("temperature": 0.1) with self.temperature (or the generate() arg).
Update the relevant function signature(s) and any docstring/comments for the
class/method (e.g., __init__, generate, kwargs, self.model) so callers can
override temperature consistently with other providers.
- Around line 119-131: When dimensions is provided you must also validate that
the selected model supports Matryoshka embeddings rather than only checking
supported_dims: after the existing type/size checks (dimensions, supported_dims)
add a compatibility check against the current model (e.g., self._model) and/or a
known set of Matryoshka-capable model names (or a helper like
_is_matryoshka_model(model)); if the model does not support Matryoshka raise a
ValueError explaining that dimensions is only valid for Matryoshka models, and
only set self._custom_dims if the compatibility check passes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5a835c9-e1b6-4e48-a6f5-d6f0a4d6c8bc

📥 Commits

Reviewing files that changed from the base of the PR and between 262368f and 94ae7b7.

📒 Files selected for processing (2)
  • tests/unit/test_factory.py
  • vektori/models/nvidia.py

@laxmanclo
laxmanclo merged commit 1692c98 into vektori-ai:main Apr 14, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants