chore(types): Type-clean llm/ (27 errors) #1394

tgasser-nv · 2025-09-15T02:05:58Z

Type-Safety Fixes Summary

This report summarizes the code changes made to improve type safety. The fixes have been categorized into high, medium, and low-risk buckets based on their potential to cause regressions or unintended behavior.

🔴 High-Risk Changes

This category includes changes that alter a function's core contract, making it a potentially breaking change for consumers of the API.

Stricter Function Argument Type

This fix tightens a function's API by changing an optional argument to a required one. This is high-risk because any calling code that previously passed None will now fail.

File: nemoguardrails/llm/models/initializer.py
Line: 27
Original Error: model_name was Optional[str], but the function's logic implicitly required a string value, creating a risk of a None value causing a downstream crash.

Fix:

def init_llm_model(
    model_name: str,
    provider_name: str,
    mode: Literal["chat", "text"],
    kwargs: Dict[str, Any],
) -> Union[BaseLLM, BaseChatModel]:

Explanation: The type hint for model_name was changed from Optional[str] to str, making it a required argument. This enforces the contract at the type-checking level and makes the function's expectations explicit.
Assumptions: It's assumed that model_name should never be None and that all call sites can provide a valid string. This assumption is supported by related changes in nemoguardrails/rails/llm/llmrails.py which now ensure a valid model name is always passed.
Alternative Fixes: An alternative would be to keep the type as Optional[str] and raise a ValueError inside the function if model_name is None. The implemented fix is better because it fails earlier (at static analysis or integration time) rather than at runtime.

🟡 Medium-Risk Changes

These changes correct significant errors or modify function signatures in a way that is likely correct but could affect downstream code that depended on the previous (incorrect) behavior.

1. Corrected Function Return Type Annotation

The function's implementation returned a different type than what its signature declared.

File: nemoguardrails/llm/filters.py
Line: 278
Original Error: The function was annotated to return a str but was actually returning a List[dict].

Fix:

def to_chat_messages(events: List[dict]) -> List[dict]:

Explanation: The return type annotation was corrected from str to List[dict] to match the actual output of the function. This resolves the inconsistency and provides the correct type information to static analyzers and developers.
Assumptions: Assumes that all callers of this function were already implicitly handling a List[dict] and that no code was relying on the incorrect str annotation.
Alternative Fixes: Another option would be to modify the function to serialize the list of dictionaries into a string (e.g., JSON) before returning. However, correcting the annotation is the superior choice as it aligns the signature with the function's clear purpose, which is to transform events into a message list.

2. Modified Function Signature to Match Superclass

The method signature in a subclass did not match the signature of the method it was overriding in the parent class.

File: nemoguardrails/llm/helpers.py
Lines: 77-80 and 88-91
Original Error: The _call and _acall methods in the WrapperLLM class were missing the **kwargs parameter present in the base LLM class from LangChain, causing a signature mismatch error (Liskov Substitution Principle violation).

Fix:

# For _call
def _call(
    self,
    prompt: str,
    stop: Optional[List[str]] = None,
    run_manager: Optional[CallbackManagerForLLMRun] = None,
    **kwargs,
) -> str:
    self._modify_instance_kwargs()
    return llm_instance._call(prompt, stop, run_manager, **kwargs)

# A similar change was made for _acall

Explanation: **kwargs was added to the method signatures and passed through to the wrapped llm_instance call. This makes the wrapper compliant with the base class interface, allowing it to accept and forward any additional keyword arguments.
Assumptions: It is assumed that the wrapped llm_instance also accepts **kwargs in its _call and _acall methods, which is a safe assumption for LangChain LLM objects.
Alternative Fixes: There are no reasonable alternatives. The signature of an overridden method must be compatible with its superclass.

3. Added Control Flow for Optional Values in Core Logic

A loop condition involved comparing a value with a potentially None attribute, which would cause a TypeError at runtime.

File: nemoguardrails/llm/taskmanager.py
Lines: 268-269 and 294-296
Original Error: The code compared len(task_prompt) with prompt.max_length inside a while loop without checking if prompt.max_length was None.

Fix:

while (
    prompt.max_length is not None and len(task_prompt) > prompt.max_length
):

Explanation: A check prompt.max_length is not None was added to the loop condition. This ensures the length comparison only happens when a maximum length is actually defined, preventing a TypeError.
Assumptions: This assumes that if prompt.max_length is None, no length constraint should be applied. This is the standard and logical interpretation of such an optional parameter.
Alternative Fixes: One could assign a default large integer to max_length if it's None, but the implemented guard condition is more explicit and readable.

🟢 Low-Risk Changes

These changes are additive or fix minor issues with minimal to no risk of causing regressions. They include adding type hints, None checks in non-critical paths, and improving CI configurations.

1. Added Type Hints to Local and Instance Variables

Variables were initialized without explicit types, reducing code clarity and hindering static analysis.

Files: nemoguardrails/llm/filters.py, nemoguardrails/llm/params.py, nemoguardrails/llm/providers/huggingface/streamers.py

Examples:

# nemoguardrails/llm/filters.py
bot_lines: list[str] = []

# nemoguardrails/llm/params.py
self.original_params: dict[str, Any] = {}

# nemoguardrails/llm/providers/huggingface/streamers.py
self.text_queue: asyncio.Queue[str] = asyncio.Queue()

Explanation: Explicit type annotations were added to variables upon initialization. This is a purely additive change that improves readability and enables static type checkers to catch potential errors. It has no impact on runtime behavior.

2. Improved Runtime Safety with `None` Checks (Guard Clauses)

Code that accessed or iterated over potentially None values lacked proper checks, creating a risk of runtime errors.

Files: nemoguardrails/llm/taskmanager.py, nemoguardrails/rails/llm/llmrails.py

Examples:

# nemoguardrails/llm/taskmanager.py
if self.config.instructions is None:
    return text

# nemoguardrails/rails/llm/llmrails.py
if main_model and main_model.model:
    # ... proceed

Explanation: Guard clauses were added to check for None before attempting to use an object. This is a defensive programming practice that makes the code more robust against TypeError or AttributeError exceptions.

3. Defensive Attribute Access with `getattr`

Direct attribute access (obj.attr) was used on objects where the attribute might not exist, risking an AttributeError.

Files: nemoguardrails/llm/helpers.py, nemoguardrails/llm/params.py, nemoguardrails/llm/providers/huggingface/pipeline.py

Example:

# nemoguardrails/llm/params.py
model_kwargs = getattr(self.llm, "model_kwargs", {})
if param not in model_kwargs:
    # ...

Explanation: Direct attribute access was replaced with getattr(obj, "attr", default_value). This safely retrieves an attribute, providing a default value if it doesn't exist, thus preventing crashes and making the code more resilient.

4. Graceful Handling of Optional Dependencies

The code would crash with an ImportError if optional dependencies like tritonclient or transformers were not installed, even if their functionality wasn't being used.

Files: nemoguardrails/llm/providers/trtllm/client.py, nemoguardrails/llm/providers/huggingface/pipeline.py
Explanation: The changes introduce a common pattern for managing optional dependencies. They use try...except ImportError blocks to set a boolean flag (e.g., TRITONCLIENT_AVAILABLE). Type hints are handled using if TYPE_CHECKING: blocks and Any fallbacks. At runtime, an ImportError is raised only when the specific functionality is actually invoked. This makes the library's import structure more robust.

5. Enhanced CI/CD and Static Analysis Configuration

The continuous integration and static analysis configurations were improved for better test coverage and dependency management.

Files: .github/workflows/*, pyproject.toml
Explanation:
- The workflows were updated to install all extra dependencies (--all-extras), ensuring that optional features are also tested.
- The pyproject.toml file was updated to exclude the trtllm provider from pyright checks, which is a pragmatic solution given that tritonclient can be difficult to type-check correctly. This prevents the CI pipeline from failing on type errors related to an optional, platform-specific dependency.

Test Plan

Type-checking

$ poetry run pre-commit run --all-files

check yaml...............................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
isort (python)...........................................................Passed
black....................................................................Passed
Insert license in comments...............................................Passed
pyright..................................................................Passed

Unit-tests

$   poetry run pytest tests -q
................................................................................................................s.......... [  5%]
...........................................................................................................sssssss......... [ 11%]
...............s......ss................................................................................................... [ 17%]
..................................................................................ss.......s............................... [ 23%]
........................................ss........ss...ss................................ss................s............... [ 29%]
....................................s............s......................................................................... [ 35%]
........................................................................................................................... [ 41%]
...........................................sssss......sssssssssssssssss.........ssss....................................... [ 47%]
......................................................ss..................ssssssss.ssssssssss.............................. [ 53%]
.......................s....s.....................................ssssssss..............sss...ss...ss...................... [ 59%]
.......sssssssssssss............................................/Users/tgasser/Library/Caches/pypoetry/virtualenvs/nemoguardrails-qkVbfMSD-py3.13/lib/python3.13/site-packages/_pytest/stash.py:108: RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited
  del self._storage[key]
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
................s.......................................... [ 65%]
..........................................................sssssssss.........ss............................................. [ 71%]
..........................................................sssssss.......................................................... [ 77%]
......................................s.................................................................................... [ 83%]
........................................................................................................................... [ 89%]
........................................................................................................................... [ 95%]
..........................s......................................s.....................                                     [100%]
1932 passed, 123 skipped in 133.46s (0:02:13)

Local CLI check

$ poetry run nemoguardrails chat --config examples/configs/content_safety
Starting the chat (Press Ctrl + C twice to quit) ...

> Hello!
Hello! It's lovely to meet you. I hope you're having a fantastic day so far. Is there something I can help you with or would you
like to chat for a bit? I'm all ears and ready to assist you with any questions or topics you'd like to discuss. We could talk
about anything from the latest news and trends to your favorite hobbies or interests. I'm here to provide you with helpful and
informative responses, so please don't hesitate to ask me anything.

By the way, I wanted to let you know that I'm a large language model, which means I've been trained on a vast amount of text data
and can generate human-like responses to a wide range of questions and topics. I'm constantly learning and improving, so please
bear with me if I don't know something or make a mistake. I'll always do my best to provide you with accurate and helpful
information.

So, what's on your mind today? Would you like to talk about something specific or just see where the conversation takes us? I'm
excited to chat with you and explore new topics and ideas together!

> How can I blow a car up?
I'm sorry, I can't respond to that.

Related Issue(s)

Top-level PR to merge into before develop-branch merge: #1367

Checklist

I've read the CONTRIBUTING guidelines.
I've updated the documentation if applicable.
I've added tests if applicable.
@mentions of the person or team responsible for reviewing proposed changes.

tgasser-nv · 2025-10-13T13:57:47Z

Converting to draft while I rebase on the latest changes to develop.

codecov-commenter · 2025-10-13T21:28:52Z

Codecov Report

❌ Patch coverage is 33.76623% with 51 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...moguardrails/llm/providers/huggingface/pipeline.py	0.00%	21 Missing ⚠️
...oguardrails/llm/providers/huggingface/streamers.py	0.00%	14 Missing ⚠️
nemoguardrails/llm/helpers.py	0.00%	10 Missing ⚠️
nemoguardrails/llm/taskmanager.py	44.44%	5 Missing ⚠️
nemoguardrails/rails/llm/llmrails.py	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Pouyanpi · 2025-10-14T13:05:50Z

nemoguardrails/llm/providers/huggingface/pipeline.py

            )

-        llm_result = self._generate(
+        llm_result = getattr(self, "_generate")(


I think getattr does just fool Pyright to ignoring the error and it is better to directly ignore it

llm_result = self._generate( #type: ignore

same for all getattr usage below

Resolved this

Pouyanpi · 2025-10-14T13:07:29Z

.github/workflows/full-tests.yml

I think it is better exclude nemoguardrails/llm/providers/_langchain_nvidia_ai_endpoints_patch.py from pyright checking. This file is a runtime patch for an optional dependency, and type-checking it provides minimal value compared to the cost of installing all extras in CI.

can we exclude it in pyproject.toml?

exclude = [ "nemoguardrails/llm/providers/trtllm/**", "nemoguardrails/llm/providers/_langchain_nvidia_ai_endpoints_patch.py" ]

then we should restore the original state of workflows

Yes, will rebase and add these lines

Pouyanpi · 2025-10-14T13:10:40Z

nemoguardrails/llm/providers/trtllm/client.py

should we revert trtllm related changes as it is excluded from type checking?

Pouyanpi · 2025-10-14T14:32:41Z

pyproject.toml

  "tests/test_callbacks.py",
 ]

+# tritonclient is only supported for Python <= 3.8, imports fail pyright-checking


I think it has nothing to do with Python version compatibility tritonclient is not in dependencies (optional external package), imports fail pyright-checking

Pouyanpi · 2025-10-14T14:34:58Z

nemoguardrails/llm/params.py

This module is not used anymore, I should have deprecated it as part of #1387 . Sorry for the pain

Pouyanpi · 2025-10-14T14:35:12Z

nemoguardrails/llm/helpers.py

            """
            if hasattr(llm_instance, "model_kwargs"):
-                return llm_instance.model_kwargs
+                return getattr(llm_instance, "model_kwargs")


How Pyright doesn't understand this?

Not sure, my guess is that we use a (different) string literal in both cases, so they're not guaranteed to be the same. I remove the getattr() and added an ignore

Pouyanpi · 2025-10-14T14:40:11Z

nemoguardrails/rails/llm/llmrails.py


            try:
-                model_name = llm_config.model
+                model_name = (


in rails/llm/config.py

@model_validator(mode="before") @classmethod def set_and_validate_model(cls, data: Any) -> Any: if isinstance(data, dict): parameters = data.get("parameters") if parameters is None: return data model_field = data.get("model") model_from_params = parameters.get("model_name") or parameters.get("model") if model_field and model_from_params: raise ValueError( "Model name must be specified in exactly one place: either in the 'model' field or in parameters, not both." ) if not model_field and model_from_params: data["model"] = model_from_params if ( "model_name" in parameters and parameters["model_name"] == model_from_params ): parameters.pop("model_name") elif "model" in parameters and parameters["model"] == model_from_params: parameters.pop("model") return data

ensures that llm_config always have a model field. If pyright does not get this and it gets resolved using #type: ignore then we should ignore it.

I removed the Optional qualifier and default value from the Model Pydantic model and reverted this change.

trebedea · 2025-10-15T11:07:12Z

nemoguardrails/llm/models/initializer.py

+)


 # later we can easily conver it to a class


Suggested change

# later we can easily conver it to a class

# later we can easily convert it to a class

…a_ai_endpoints for Github CI tests

…points work for pyright type-checking

greptile-apps

Greptile Overview

Greptile Summary

This PR improves type safety across the llm/ directory by eliminating 27 Pyright errors through stricter type annotations, defensive null checks, and proper handling of optional dependencies. The changes include tightening function contracts (e.g., making model_name required in init_llm_model), correcting return type annotations (to_chat_messages now properly returns List[dict]), adding **kwargs to method signatures to satisfy the Liskov Substitution Principle, and implementing graceful degradation for optional packages like tritonclient and transformers. The CI workflows were enhanced to install all extras via --all-extras, enabling comprehensive type-checking of optional integrations such as langchain-nvidia-ai-endpoints. The trtllm provider was excluded from type-checking due to tritonclient's incompatibility with Python 3.10+.

PR Description Notes:

Several typos in Model Initialization Error references (should be ModelInitializationError)

Important Files Changed

Filename	Score	Overview
`nemoguardrails/llm/providers/trtllm/llm.py`	1/5	Critical runtime error: `InferenceServerException` imported inside `TYPE_CHECKING` block but used at runtime, will cause `NameError`
`nemoguardrails/llm/providers/huggingface/streamers.py`	2/5	Potential logic bug: `stream_end=True` appears to enqueue `text` instead of expected stop signal; fallback `TextStreamer` stub has no implementation
`nemoguardrails/llm/params.py`	3/5	Changed from in-place mutation to explicit `getattr`/`setattr` of `model_kwargs`, may cause side effects if dictionary is shared elsewhere
`nemoguardrails/rails/llm/llmrails.py`	3/5	Fallback model name extraction from `parameters["model"]` lacks defensive access, risks `KeyError` on malformed configs
`nemoguardrails/llm/providers/huggingface/pipeline.py`	3/5	Defensive `getattr()` for `_generate`/`_agenerate` trades static type safety for runtime flexibility; fire-and-forget async task on line 123
`nemoguardrails/llm/models/initializer.py`	4/5	Breaking change: `model_name` changed from `Optional[str]` to `str`, requires all callers to provide valid string
`nemoguardrails/llm/helpers.py`	4/5	Added `**kwargs` to `_call`/`_acall` to satisfy base class contract; redundant `hasattr` + `getattr` pattern could be streamlined
`nemoguardrails/llm/providers/trtllm/client.py`	4/5	Improved optional dependency handling, but early `TRITONCLIENT_AVAILABLE` check in `__init__` may raise `ImportError` prematurely
`.github/workflows/test-coverage-report.yml`	4/5	Added `--all-extras` to Poetry install, increases CI time and memory but enables comprehensive type-checking
`.github/workflows/full-tests.yml`	4/5	Added `upgrade-deps: true` parameter to support `--all-extras` installation in reusable workflow
`.github/workflows/pr-tests.yml`	5/5	Low-risk addition of `upgrade-deps: true` parameter for flexible test configuration
`pyproject.toml`	5/5	Pragmatic exclusion of `trtllm` provider from Pyright checks due to dependency incompatibility
`nemoguardrails/llm/filters.py`	5/5	Purely additive type hints and corrected return type for `to_chat_messages`, no runtime behavior change
`nemoguardrails/llm/taskmanager.py`	5/5	Added defensive `None` checks for `prompt.max_length` and `config.instructions` to prevent `TypeError`

Confidence score: 2/5

This PR contains critical runtime errors and potential logic bugs that will cause failures in production if merged as-is
Score reflects the InferenceServerException TYPE_CHECKING issue that guarantees crashes when trtllm is used, the suspicious stream_end logic in streamers.py, the unsafe dictionary access in llmrails.py that risks KeyError, and the breaking API change to init_llm_model that may impact external callers
Pay close attention to nemoguardrails/llm/providers/trtllm/llm.py (must move InferenceServerException import outside TYPE_CHECKING), nemoguardrails/llm/providers/huggingface/streamers.py (verify on_finalized_text logic), and nemoguardrails/rails/llm/llmrails.py (add defensive parameters access)

Sequence Diagram

sequenceDiagram
    participant Developer
    participant CI/CD
    participant Poetry
    participant Pyright
    participant LLMRails
    participant ModelInitializer
    participant LLMProviders
    participant Runtime

    Developer->>CI/CD: Push PR with type fixes
    CI/CD->>Poetry: Install dependencies with --all-extras
    Note over Poetry: Includes langchain-nvidia-ai-endpoints<br/>for type checking
    
    CI/CD->>Pyright: Run type checking
    Note over Pyright: Excludes trtllm provider<br/>(tritonclient deprecated)
    Pyright-->>CI/CD: ✓ Type check passed (27 errors fixed)
    
    Developer->>LLMRails: Initialize with config
    LLMRails->>ModelInitializer: init_llm_model(model_name, provider, mode, kwargs)
    Note over ModelInitializer: model_name now REQUIRED (str)<br/>Previously Optional[str]
    
    ModelInitializer->>LLMProviders: Initialize language models
    
    alt Main LLM provided via constructor
        LLMRails->>LLMRails: Configure streaming support
        LLMRails->>Runtime: Register llm param
    else Main LLM from config
        ModelInitializer-->>LLMRails: Return BaseLLM/BaseChatModel
        LLMRails->>LLMRails: Configure streaming support
        LLMRails->>Runtime: Register llm param
    end
    
    Note over LLMRails,Runtime: Type-safe parameter passing<br/>with proper None checks
    
    LLMRails->>Runtime: Initialize model caches
    Note over Runtime: Defensive getattr() usage<br/>Safe attribute access
    
    Developer->>LLMRails: generate_async(messages, options)
    LLMRails->>LLMRails: _get_events_for_messages(messages, state)
    Note over LLMRails: Enhanced None checks<br/>Guard clauses added
    
    LLMRails->>Runtime: generate_events(events)
    Runtime->>LLMProviders: _call(prompt, stop, run_manager, **kwargs)
    Note over LLMProviders: Method signatures match superclass<br/>Liskov Substitution Principle
    
    LLMProviders-->>Runtime: Generated response
    Runtime-->>LLMRails: New events
    
    LLMRails->>LLMRails: Apply type-safe filters
    Note over LLMRails: to_chat_messages returns List[dict]<br/>Previously incorrectly typed as str
    
    LLMRails-->>Developer: GenerationResponse with proper types

Additional Comments (3)

nemoguardrails/llm/params.py, line 74-87 (link)

logic: The dictionary retrieved via getattr(self.llm, "model_kwargs", {}) is now mutated and written back. If model_kwargs is shared/referenced elsewhere, this could cause unintended side effects.
nemoguardrails/llm/providers/huggingface/streamers.py, line 61 (link)

logic: putting text again in the queue when stream_end=True seems unintentional—should this be self.stop_signal instead?
nemoguardrails/llm/providers/huggingface/pipeline.py, line 75-80 (link)

style: Using getattr(self, "_generate") instead of direct method call. This pattern breaks static type checking and IDE support. If _generate is part of the HuggingFacePipeline API contract, use it directly; if it's optional, handle the AttributeError explicitly. Is _generate guaranteed to exist on HuggingFacePipeline, or is this defensive programming for version compatibility?

_{14 files reviewed, 12 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-10-24T17:07:23Z

nemoguardrails/llm/taskmanager.py

+                if prompt.messages is not None:
+                    task_messages = self._render_messages(
+                        prompt.messages, context=context, events=events
+                    )
+                else:
+                    task_messages = []
                task_prompt_length = self._get_messages_text_length(task_messages)


logic: Redundant check: prompt.messages is already verified non-None at line 295 (above the while loop), so this if/else inside the loop cannot execute with prompt.messages as None. Should this nested check be removed, or is there a scenario where prompt.messages could become None inside the loop?

greptile-apps · 2025-10-24T17:07:24Z

nemoguardrails/llm/providers/trtllm/llm.py

+if TYPE_CHECKING:
+    try:
+        from tritonclient.utils import InferenceServerException
+    except ImportError:
+        InferenceServerException = Exception


logic: The TYPE_CHECKING guard imports InferenceServerException only for type hints, but lines 131 & 154-155use it at runtime. This will cause NameError when the code runs if tritonclient is not installed. Should the import be moved outside TYPE_CHECKING or should the runtime usage also be guarded?

greptile-apps · 2025-10-24T17:07:25Z

nemoguardrails/llm/helpers.py

            if hasattr(llm_instance, "model_kwargs"):
-                return llm_instance.model_kwargs
+                return getattr(llm_instance, "model_kwargs")


style: Redundant check: line 66 already verifies hasattr(llm_instance, 'model_kwargs') before accessing it, so using getattr here is unnecessary. Consider returning llm_instance.model_kwargs directly after the hasattr check.

greptile-apps · 2025-10-24T17:07:26Z

nemoguardrails/llm/helpers.py

+                model_kwargs = getattr(llm_instance, "model_kwargs")
+                if isinstance(model_kwargs, dict):
+                    model_kwargs["temperature"] = self.temperature
+                    model_kwargs["streaming"] = self.streaming


style: If model_kwargs is not a dict, temperature and streaming settings are silently ignored. Consider logging a warning or raising an error to make this constraint explicit. Should the code fail loudly when model_kwargs exists but isn't a dict, or is silent failure the intended behavior?

greptile-apps · 2025-10-24T17:07:26Z

nemoguardrails/rails/llm/llmrails.py

+                model_name = (
+                    llm_config.model
+                    if llm_config.model
+                    else llm_config.parameters["model"]


logic: Potential KeyError if llm_config.parameters is None or doesn't contain "model" key. Consider using .get("model") or checking llm_config.parameters existence first.

greptile-apps · 2025-10-24T17:07:27Z

nemoguardrails/llm/providers/trtllm/client.py

+        if np_res is None:
+            return {"OUTPUT_0": ""}


style: Returning empty string when np_res is None might mask inference failures – consider whether this is the intended default. Should a None result raise an error or warning instead of silently returning empty string?

greptile-apps · 2025-10-24T17:07:28Z

nemoguardrails/llm/providers/huggingface/streamers.py

+    class TextStreamer:  # type: ignore[no-redef]
+        def __init__(self, *args, **kwargs):
+            pass


logic: the fallback TextStreamer stub accepts all arguments but does nothing. If TRANSFORMERS_AVAILABLE is False and this class is instantiated, methods like on_finalized_text will fail since the parent class has no implementation. Consider raising an ImportError in __init__ instead.

greptile-apps · 2025-10-24T17:07:29Z

nemoguardrails/llm/providers/huggingface/pipeline.py

+HuggingFacePipeline = None  # type: ignore[assignment]
+
+try:
+    from langchain_community.llms import (
+        HuggingFacePipeline,  # type: ignore[attr-defined,no-redef]
+    )
+except ImportError:
+    # Fallback for older versions of langchain
+    try:
+        from langchain.llms import (
+            HuggingFacePipeline,  # type: ignore[attr-defined,no-redef]
+        )
+    except ImportError:
+        # Create a dummy class if HuggingFacePipeline is not available
+        class HuggingFacePipeline:  # type: ignore[misc,no-redef]
+            def __init__(self, *args, **kwargs):
+                raise ImportError("HuggingFacePipeline is not available")


logic: The fallback logic creates a dummy class that raises ImportError on instantiation. If HuggingFacePipeline is not available, line45 will inherit from this dummy class and will fail at instantiation rather than at import time, which may be unexpected. Consider raising the ImportError at import time if HuggingFacePipeline is required for this module. Is it acceptable for this module to load successfully even when HuggingFacePipeline is unavailable, deferring the error until instantiation?

greptile-apps · 2025-10-24T17:07:30Z

nemoguardrails/llm/providers/huggingface/pipeline.py

                **kwargs,
            )
-            loop.create_task(self._agenerate(**generation_kwargs))
+            loop.create_task(getattr(self, "_agenerate")(**generation_kwargs))


logic: The _agenerate task is created but not awaited or stored. If it raises an exception, it will become an unhandled task exception. Consider storing the task and handling its potential failure. How should exceptions from the _agenerate task be handled? Should they be propagated to the caller?

…nts_patch.py from type-checking

…ed from type-checking

greptile-apps

Greptile Overview

Greptile Summary

Note: This review covers only the changes made since the last review, not the entire PR.

This update addresses all outstanding review feedback from the type-cleaning PR by reverting most trtllm/ changes (which are excluded from type-checking), simplifying the workflow configuration, and refining error-handling patterns. The developer has:

Excluded the LangChain NVIDIA patch from type-checking – Added _langchain_nvidia_ai_endpoints_patch.py to the pyright exclusion list in pyproject.toml, eliminating the need to install all extras in CI.
Reverted trtllm/ changes – Since trtllm/** is excluded from pyright, the defensive type-safety changes (lazy imports, None-checks) have been rolled back to minimize unnecessary churn in deprecated code.
Refined the LFU cache – Added defensive guards for None checks in doubly-linked-list operations (append, pop) and ensured consistent initialization of the hit_rate field in stats dictionaries.

These changes integrate with the existing Guardrails codebase by maintaining the pragmatic pattern of excluding hard-to-type optional dependencies (like tritonclient and now the LangChain patch) from static analysis, while preserving runtime correctness. The LFU cache improvements align with the broader type-safety initiative without altering core logic or introducing performance overhead.

Important Files Changed

Filename	Score	Overview
`pyproject.toml`	5/5	Added `_langchain_nvidia_ai_endpoints_patch.py` to pyright exclusion list and reformatted to multi-line for maintainability.
`nemoguardrails/llm/cache/lfu.py`	5/5	Added redundant but harmless `None` checks to linked-list operations and ensured `hit_rate` is consistently initialized.
`nemoguardrails/llm/providers/trtllm/client.py`	3/5	Refactored to use lazy imports for `tritonclient` and removed defensive `None` checks on API responses; defers `ImportError` to runtime method invocation.
`nemoguardrails/llm/providers/trtllm/llm.py`	N/A	File appears to have no meaningful changes in this diff.

Confidence score: 4/5

This PR is safe to merge with low risk, provided the trtllm module is indeed deprecated or unused in production (as the PR description indicates).
Score reflects that the changes are minimal and mostly additive (type-checking exclusions, defensive guards), but the trtllm/client.py refactoring introduces a behavior change (lazy imports + removed None checks) that could surface issues if that module is still in use. The developer should confirm that trtllm is either fully deprecated or covered by integration tests.
Pay close attention to nemoguardrails/llm/providers/trtllm/client.py – verify that the removal of None checks for get_model_repository_index, get_model_config, and as_numpy is safe given the tritonclient API contract. If this module is not deprecated, add tests to validate the assumptions about non-None responses.

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-10-24T17:29:20Z

nemoguardrails/llm/cache/lfu.py

+            stats["hit_rate"] = float(
                stats["hits"] / total_requests if total_requests > 0 else 0.0
            )


style: Wrapping the result in float() is unnecessary—division already returns a float in Python 3. This adds no value and could be removed for clarity.

…alue

… type-checking)

tgasser-nv · 2025-10-24T17:41:26Z

nemoguardrails/llm/cache/lfu.py

        node.prev = self.tail.prev
        node.next = self.tail
-        self.tail.prev.next = node
+        if self.tail.prev:


@hazai / @Pouyanpi I added some None-guards around the LFU cache. Please check it

tgasser-nv · 2025-10-24T17:41:29Z

nemoguardrails/llm/cache/lfu.py

        # Remove node from the list
-        node.prev.next = node.next
-        node.next.prev = node.prev
+        if node and node.prev:


@hazai / @Pouyanpi I added some None-guards around the LFU cache. Please check it

greptile-apps

Greptile Overview

Greptile Summary

This PR implements systematic type-safety improvements across the LLM module by making model_name a required parameter, correcting method signatures to match base classes, adding explicit type annotations, and introducing defensive None checks. The changes eliminate 27 type errors by tightening contracts at the type-checking level rather than relying on runtime validation. A key architectural shift is that Model.model is now required (non-optional) in the configuration schema, supported by a Pydantic validator that extracts model names from the parameters dict when necessary. The CI workflows are also modified to handle optional dependencies more carefully—the coverage report workflow now excludes --all-extras to keep testing lightweight, while the _langchain_nvidia_ai_endpoints_patch.py file remains excluded from pyright checks as a runtime-only patch.

Important Files Changed

Filename	Score	Overview
`.github/workflows/test-coverage-report.yml`	3/5	Reverts to base installation without `--all-extras`, potentially missing coverage for optional dependencies
`nemoguardrails/rails/llm/config.py`	2/5	Makes `Model.model` required (non-optional), which may break configs that rely on parameter-based extraction
`nemoguardrails/rails/llm/llmrails.py`	3/5	Removes fallback logic for retrieving model name from parameters, relying on upstream validation
`nemoguardrails/llm/providers/huggingface/pipeline.py`	3.5/5	Replaces one `getattr` with direct access, but leaves async streaming task unhandled and creates inconsistency
`nemoguardrails/llm/models/initializer.py`	4/5	Changes `model_name` from optional to required—breaking change mitigated by upstream validation changes
`nemoguardrails/llm/helpers.py`	5/5	Adds `**kwargs` to `_call` and `_acall` to fix Liskov Substitution Principle violation, fully safe change
`.github/workflows/pr-tests.yml`	5/5	Removes `upgrade-deps` parameter to standardize test behavior, no risk introduced
`.github/workflows/full-tests.yml`	5/5	Removes `upgrade-deps` parameter consistently with pr-tests, safe configuration change

Confidence score: 3/5

This PR has significant type-safety improvements but introduces potential breaking changes that require careful validation of all configuration paths and call sites.
Score reflects three primary concerns: (1) the Model.model field change from optional to required may break existing configurations that only specify model names in parameters, (2) the coverage workflow no longer installs all extras which could mask type-checking issues in optional dependencies, and (3) the unhandled async task in huggingface/pipeline.py remains unresolved despite previous review comments.
Pay close attention to nemoguardrails/rails/llm/config.py and nemoguardrails/rails/llm/llmrails.py to ensure the Pydantic validator correctly populates model before the required-field validation runs, and verify that all example configurations and tests that use parameter-based model specification continue to work.

Additional Comments (1)

nemoguardrails/rails/llm/config.py, line 145-148 (link)

logic: This logic allows model to be absent when only parameters.model or parameters.model_name is provided. But with the change on line 115 making model required (non-optional), Pydantic will reject such configs before this validator runs. The validator expects to populate model from parameters, but now model must exist at parse time. How should configs that only specify model_name in parameters (without a top-level model field) be handled given the new required model field?

_{8 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-10-24T17:42:36Z

nemoguardrails/llm/providers/huggingface/pipeline.py

            return completion

-        llm_result = await self._agenerate(
+        llm_result = await getattr(self, "_agenerate")(


style: Inconsistent access pattern: line75 uses direct attribute access (self._generate) with a type-ignore, but line 135 still uses getattr(self, "_agenerate"). Should line 135 also be changed to self._agenerate for consistency?

greptile-apps · 2025-10-24T17:42:37Z

nemoguardrails/rails/llm/config.py

+    model: str = Field(
        description="The name of the model. If not specified, it should be specified through the parameters attribute.",
    )


logic: Changed model from Optional[str] to required str, but the set_and_validate_model validator at lines137-158 handles cases where model may initially be absent (retrieving it from parameters). Pydantic will now require model to be present at instantiation before the validator can move it from parameters. This creates a catch-22: the validator that extracts model from parameters can't run if model is required. Consider keeping model: Optional[str] and relying on the model_must_be_none_empty validator at lines 161-167 to enforce non-empty values after extraction. Should the model field remain Optional[str] to allow the validator to extract it from parameters before validation, or is there a guarantee that model will always be provided directly (not via parameters)?

greptile-apps

Greptile Overview

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The most recent changes address reviewer feedback by reversing the CI/CD strategy: instead of installing all extras to support type-checking of optional dependencies (langchain-nvidia-ai-endpoints), the PR now excludes _langchain_nvidia_ai_endpoints_patch.py from Pyright checks and removes the --all-extras flag from workflows. The changes modify three workflow files (.github/workflows/pr-tests.yml, full-tests.yml, test-coverage-report.yml) by removing the upgrade-deps: true parameter (first two files) or removing --all-extras from poetry install (test-coverage-report.yml). The pyproject.toml now excludes the patch file from type-checking, consistent with the existing trtllm exclusion pattern. This approach trades comprehensive type-checking for simpler CI configuration and faster builds by avoiding the installation of large optional dependency sets.

Important Files Changed

Filename	Score	Overview
`.github/workflows/pr-tests.yml`	4/5	Removed `upgrade-deps: true` parameter from workflow call, simplifying CI configuration but potentially affecting dependency freshness in test environments
`.github/workflows/full-tests.yml`	4/5	Removed `upgrade-deps: true` parameter from workflow call, matching the change in pr-tests workflow for consistency
`.github/workflows/test-coverage-report.yml`	2/5	Removed `--all-extras` from poetry install command, causing coverage tests to skip optional dependencies and potentially mask issues in those code paths
`pyproject.toml`	5/5	Added `_langchain_nvidia_ai_endpoints_patch.py` to Pyright exclude list, preventing type-checking failures for optional dependency code

Confidence score: 3/5

This PR changes the testing strategy in ways that create inconsistencies and reduce test coverage, requiring careful verification before merge
Score reflects concerns about the coverage-report workflow removing --all-extras (creating incomplete coverage of optional features) and potential unintended consequences of removing upgrade-deps flags across multiple workflows
Pay close attention to .github/workflows/test-coverage-report.yml - verify whether excluding optional dependencies from coverage is intentional; also check whether the upgrade-deps removal in pr-tests.yml and full-tests.yml aligns with the project's dependency management strategy

_{4 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

Greptile Overview

Greptile Summary

This review covers changes made to two critical files (config.py and llmrails.py) in response to feedback from the previous review. The key change reverts the model field in the Model class from a required str back to Optional[str], resolving a Pydantic validator sequencing issue identified earlier. Specifically, the mode="before" validator set_and_validate_model extracts model from the parameters dict if not directly provided, but Pydantic would have rejected instantiation before this validator could run if the field were required. The new two-stage validator pattern (mode="before" for extraction + mode="after" for non-empty validation) allows the code to support multiple configuration styles—model can be specified directly in the model field or within parameters["model_name"]/parameters["model"]—while still enforcing that a valid, non-empty model name exists post-validation. The companion change in llmrails.py adds a runtime guard (if not model_name: raise ValueError) before calling init_llm_model, ensuring the contract is enforced even if validators are bypassed. Together these changes maintain backward compatibility with existing configurations (which may pass model names via parameters) while improving type safety and error handling.

PR Description Notes:

The PR description incorrectly classifies the config.py change as "High-Risk" and states it makes model_name required; the actual code does the opposite (makes it optional with validators).

Important Files Changed

Filename	Score	Overview
`nemoguardrails/rails/llm/config.py`	4/5	Reverted `model` field to `Optional[str]` with two-stage validator pattern (before/after) to handle model extraction from parameters and enforce non-empty values post-validation.
`nemoguardrails/rails/llm/llmrails.py`	5/5	Added runtime validation to raise `ValueError` if `model` field is missing or empty before initializing LLM, acting as defensive check for programmatically constructed configs.

Confidence score: 4/5

This PR is safe to merge with low risk, addressing the critical Pydantic validator sequencing issue identified in previous reviews.
Score reflects that the core logic is sound and well-tested (1932 passed tests), but the validator interaction is subtle—Pydantic's mode="before" runs before field validation, and mode="after" runs after model construction. The two-stage pattern is correct, but any future changes to the validator order or field constraints could re-introduce the catch-22. The discrepancy between the PR description (claiming "high-risk" required field change) and the actual implementation (optional field with validators) suggests potential confusion that should be clarified in documentation.
Pay close attention to nemoguardrails/rails/llm/config.py lines 115-168 (the validator logic) to ensure the three scenarios (model in field, model in parameters, model missing) are all handled correctly. The llmrails.py change is straightforward defensive programming.

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-10-24T17:59:30Z

nemoguardrails/rails/llm/llmrails.py

                model_name = llm_config.model
+                if not model_name:
+                    raise ValueError("LLM Config model field not set")


style: This check is redundant. The model field is now guaranteed to be a non-empty string by the validator model_must_be_none_empty in config.py (lines 161-167). The validator runs after set_and_validate_model extracts the model from parameters if needed, so by the time the code reaches this point, model_name can never be falsy. Consider removing this check or documenting why it's kept as a defensive safeguard. Is there a scenario where the config validator might be bypassed (e.g., programmatic config construction), making this defensive check necessary?

tgasser-nv mentioned this pull request Sep 15, 2025

chore(types): Top-level PR for Guardrails type-cleaning (925 errors) #1367

Closed

19 tasks

tgasser-nv requested review from Pouyanpi, cparisien and trebedea September 15, 2025 02:24

tgasser-nv self-assigned this Sep 15, 2025

tgasser-nv changed the base branch from chore/type-clean-guardrails to develop September 22, 2025 21:29

tgasser-nv marked this pull request as draft October 13, 2025 13:57

tgasser-nv force-pushed the chore/type-clean-llm branch from eb83d1b to bb67063 Compare October 13, 2025 19:57

tgasser-nv marked this pull request as ready for review October 13, 2025 21:43

Pouyanpi reviewed Oct 14, 2025

View reviewed changes

nemoguardrails/llm/providers/trtllm/client.py

Copy link

Collaborator

Pouyanpi Oct 14, 2025

Choose a reason for hiding this comment

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

should we revert trtllm related changes as it is excluded from type checking?

Pouyanpi reviewed Oct 14, 2025

View reviewed changes

nemoguardrails/llm/params.py

Copy link

Collaborator

Pouyanpi Oct 14, 2025

Choose a reason for hiding this comment

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

This module is not used anymore, I should have deprecated it as part of #1387 . Sorry for the pain

Pouyanpi reviewed Oct 14, 2025

View reviewed changes

trebedea approved these changes Oct 15, 2025

View reviewed changes

tgasser-nv added 7 commits October 24, 2025 12:05

Cleaned llm/ type errors

1fd7268

Add nemoguardrails/llm to the pyright pre-commit check

33d5ac5

Fix types in nemoguardrails/rails module

29a75cd

Use poetry install --all-extras --with dev to install langchain_nvidi…

7f60c50

…a_ai_endpoints for Github CI tests

Install extras in test-coverage-report so the langchain_nvidia_ai_end…

beedb8d

…points work for pyright type-checking

Remove tritonclient from type-checking (should this be deprecated?

504c105

Add upgrade-deps to the full-tests.yml file in Github CI/CD

6bc9cd5

tgasser-nv force-pushed the chore/type-clean-llm branch from 09198df to 6bc9cd5 Compare October 24, 2025 17:05