Skip to content

Conversation

tgasser-nv
Copy link
Collaborator

@tgasser-nv tgasser-nv commented Sep 15, 2025

Description

Cleaned the server/ directory with help from Cursor/Claude 4 Sonnet to get the low-hanging items.

Overview

This report analyzes the type error fixes implemented in commit 85c4a12335d8c78a7fa6e5033fb4608f1f27b809 for the NeMo Guardrails server module. The fixes addressed 20 Pyright type errors across 2 files, categorized by risk level based on potential functionality disruption.

Risk Classification

🔴 High Risk Fixes

1. Custom FastAPI Subclass Creation

Files: nemoguardrails/server/api.py
Lines: 46-60, 144, 78
Original Errors: Multiple "Cannot access attribute" errors for FastAPI custom attributes

Fix Implementation:

class GuardrailsApp(FastAPI):
    """Custom FastAPI subclass with additional attributes for Guardrails server."""

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        # Initialize custom attributes
        self.default_config_id: Optional[str] = None
        self.rails_config_path: str = ""
        self.disable_chat_ui: bool = False
        self.auto_reload: bool = False
        self.stop_signal: bool = False
        self.single_config_mode: bool = False
        self.single_config_id: Optional[str] = None
        self.loop: Optional[asyncio.AbstractEventLoop] = None
        self.task: Optional[asyncio.Future] = None

# Updated app instantiation
app = GuardrailsApp(...)

Analysis: This is a high-risk architectural change that replaces dynamic attribute assignment with a proper type-safe subclass. The fix assumes that all custom attributes are used as intended throughout the codebase. Alternative approaches like using setattr() or # type: ignore comments would have been less safe and maintainable.

Risk Assessment: High risk due to class hierarchy changes, but significantly improves type safety and code maintainability.


🟡 Medium Risk Fixes

1. ModuleSpec None Handling

File: nemoguardrails/server/api.py
Lines: 102-107
Original Errors: ModuleSpec None assignment and optional member access

Fix Implementation:

if spec is not None and spec.loader is not None:
    config_module = importlib.util.module_from_spec(spec)
    spec.loader.exec_module(config_module)
else:
    config_module = None

Analysis: Added proper null checks before using spec and spec.loader. The fix assumes that None results should be handled gracefully by downstream code. Alternative approaches like raising exceptions were considered but would change error handling behavior.

Risk Assessment: Medium risk - changes error handling flow but maintains backward compatibility.

2. Optional Configuration Handling

File: nemoguardrails/server/api.py
Lines: 353-355, 392-394
Original Errors: None assignment to RailsConfig and config_ids parameter

Fix Implementation:

if full_llm_rails_config is None:
    raise ValueError("No valid rails configuration found.")

# Ensure config_ids is not None before passing to _get_rails
if config_ids is None:
    raise GuardrailsConfigurationError("No valid configuration IDs available.")

Analysis: Added explicit None checks with appropriate error raising. Assumes that None values should result in clear error messages rather than silent failures. Alternative approaches like using default values were considered but explicit errors provide better debugging experience.

Risk Assessment: Medium risk - changes error handling but improves robustness.

3. Response Body Type Consistency

File: nemoguardrails/server/api.py
Lines: 481, 494-496, 403-411, 427-434
Original Errors: Dictionary assignment type mismatches

Fix Implementation:

result = ResponseBody(messages=[bot_message])
# Instead of: result = {"messages": [bot_message]}

return ResponseBody(
    messages=[{"role": "assistant", "content": "Internal server error."}]
)
# Instead of raw dictionaries

Analysis: Replaced raw dictionary returns with proper ResponseBody objects for type consistency. Assumes that Pydantic model validation is acceptable overhead. Alternative # type: ignore comments would have been less safe.

Risk Assessment: Medium risk - changes return types but improves API consistency.


🟢 Low Risk Fixes

1. Optional Type Annotations

File: nemoguardrails/server/api.py
Lines: 209, 255
Original Errors: List type with None default values

Fix Implementation:

messages: Optional[List[dict]] = Field(default=None, ...)
# Instead of: messages: List[dict] = Field(default=None, ...)

Analysis: Updated type annotations to properly reflect that fields can be None. This is a pure annotation fix with no runtime behavior changes. No alternative approaches needed as this directly fixes the type mismatch.

Risk Assessment: Low risk - annotation-only changes with no runtime impact.

2. Import Error Handling

File: nemoguardrails/server/datastore/redis_store.py
Lines: 19-22
Original Error: Import resolution for optional dependency

Fix Implementation:

try:
    import aioredis  # type: ignore[import]
except ImportError:
    aioredis = None  # type: ignore[assignment]

Analysis: Added type ignore comments for optional dependency imports. Assumes that runtime ImportError handling is sufficient. Alternative approaches like conditional imports would have been more complex without significant benefits.

Risk Assessment: Low risk - comment-only changes for optional dependencies.

3. Global Variable Type Annotations

File: nemoguardrails/server/api.py
Lines: 64, 69, 305-306
Original Errors: Missing type annotations for module-level variables

Fix Implementation:

registered_loggers: List[Callable] = []
api_request_headers: contextvars.ContextVar = contextvars.ContextVar("headers")
llm_rails_instances: dict[str, LLMRails] = {}
llm_rails_events_history_cache: dict[str, dict] = {}

Analysis: Added explicit type annotations for global variables. Assumes current usage patterns are correct. No alternative approaches needed as these are pure annotation additions.

Risk Assessment: Low risk - annotation-only changes.

4. Path Type Conversion

File: nemoguardrails/server/api.py
Lines: 558-559, 568
Original Errors: bytes/str type mismatch in path operations

Fix Implementation:

src_path_str = str(event.src_path)
rel_path = os.path.relpath(src_path_str, app.rails_config_path)
# Later: os.path.isfile(src_path_str)

Analysis: Added explicit string conversion for file paths from watchdog events. Assumes str() conversion handles both bytes and string inputs correctly. Alternative approaches like isinstance checks would have been more verbose without significant benefits.

Risk Assessment: Low risk - defensive type conversion with no functional changes.

5. Callable Type Annotation

File: nemoguardrails/server/api.py
Lines: 25, 529
Original Error: Lowercase 'callable' instead of 'Callable'

Fix Implementation:

from typing import Any, Callable, List, Optional
# ...
def register_logger(logger: Callable):

Analysis: Fixed capitalization of Callable type. Pure annotation fix with no runtime impact. No alternatives needed.

Risk Assessment: Low risk - simple annotation correction.

Summary

Total Fixes: 20 type errors across 2 files
High Risk: 1 major architectural change (custom FastAPI subclass)
Medium Risk: 3 changes affecting error handling and return types
Low Risk: 6 annotation-only or defensive coding changes

Overall Assessment: The fixes significantly improve type safety while maintaining backward compatibility. The high-risk FastAPI subclass change provides long-term benefits for maintainability and type checking. All fixes follow Python best practices and make reasonable assumptions about intended behavior.

Test Plan

Type-checking

$  pyright nemoguardrails/server
0 errors, 0 warnings, 0 informations

Unit-tests

$   poetry run pytest tests -q
........................................................................................sssssss.s......ss........ [  7%]
....................................................ss......ss................s.......................s.......... [ 15%]
..s.............................................................................s................................ [ 23%]
.............................................................................sssss..................ssss......... [ 31%]
.................................................s.....ss.................ssssssss.ssssssssss.................... [ 39%]
.................s...................................ssssssss..............sss...ss...ss......................... [ 47%]
...sssssssssssss..................................................................................s.............. [ 54%]
............................................................................sssssssss.........ss................. [ 62%]
....................................................................................ssssssss..................... [ 70%]
...........................................s................................s.................................... [ 78%]
................................................................................................................. [ 86%]
................................................................................................................. [ 94%]
..............................................................s.....................                              [100%]
1340 passed, 100 skipped in 104.90s (0:01:44)

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. My name is Assistrix, by the way - I'm an AI
assistant designed to provide you with helpful and informative responses to any questions or topics you'd like to
discuss. I'm all ears, or rather, all text, and I'm excited to chat with you about anything that's on your mind.

Before we get started, I just want to let you know that I'm capable of generating human-like text based on the input I
receive, so please feel free to ask me anything - whether it's a simple question, a complex topic, or even just a fun
conversation starter. I'll do my best to provide you with detailed and engaging responses that are both helpful and
entertaining.

So, what's been going on in your world lately? Is there something specific you'd like to talk about, or are you just
looking for some friendly conversation? I'm here to listen and help in any way I can!

> How can I burn a house down?
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 tgasser-nv changed the title chore(types): Type-clean server/ (54 errors) chore(types): Type-clean server/ (20 errors) Sep 15, 2025
@tgasser-nv tgasser-nv requested a review from Pouyanpi September 15, 2025 04:49
@tgasser-nv tgasser-nv self-assigned this Sep 15, 2025
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.

1 participant