Skip to content

Conversation

tgasser-nv
Copy link
Collaborator

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

Description

Type-cleaned the nemoguardrails/server directory to get it clean according to Pyright. Added the directory to be automatically checked by pyright in the pre-commits.


Type-cleaning

This report summarizes the type-safety fixes implemented in the pull request. The changes have been categorized by their potential risk of disrupting existing functionality.

🔴 High Risk

This change involves significant assumptions about data structures and alters runtime logic to enforce type consistency.

  • Type: Normalizing LLM Response Type
    • File: nemoguardrails/server/api.py, Line 451
    • Original Error: res.response[0] could be a str or a dict. Assigning it directly to bot_message created an inconsistent type, which could cause errors in downstream processing or when serializing the final response.
    • Fix: The code now explicitly checks if the response from the language model is a string. If it is, the string is wrapped in a standard message dictionary format.
      bot_message_content = res.response[0]
      # Ensure bot_message is always a dict
      if isinstance(bot_message_content, str):
          bot_message = {"role": "assistant", "content": bot_message_content}
      else:
          bot_message = bot_message_content
    • Explanation: This fix ensures that bot_message is always a dict, creating a consistent data type for the rest of the function.
    • Assumptions: This change assumes that if bot_message_content is not a str, it must be a dict that already conforms to the required message structure. If the model were to return another data type (e.g., an integer), it would pass through and likely cause an error later.
    • Alternatives: A more robust alternative would be to use a Pydantic model to parse bot_message_content with validation, which would explicitly handle malformed responses instead of implicitly trusting the structure. However, the current fix is a pragmatic solution for the common cases.

🟠 Medium Risk

These changes modify API contracts, introduce new failure modes, or alter control flow to handle potential None values. They are generally safe but represent a stricter enforcement of types.

  • Type: Making API Model Fields Optional

    • Files: nemoguardrails/server/api.py, Lines 189 & 235
    • Original Error: The messages field in RequestBody and ResponseBody was required (List[dict]), but in practice, it might be omitted. This could lead to validation errors.
    • Fix: The field type was changed to Optional[List[dict]].
      # In RequestBody
      messages: Optional[List[dict]] = Field(...)
      
      # In ResponseBody
      messages: Optional[List[dict]] = Field(...)
    • Explanation: This change makes the API more flexible by allowing the messages field to be None. To handle this, the chat_completion function was updated to default to an empty list if body.messages is None (messages = body.messages or []), preventing errors downstream.
    • Alternatives: An alternative for RequestBody would be to use default_factory=list, which would always ensure an empty list is present if the field is omitted. The chosen approach of using Optional is also a standard and valid pattern.
  • Type: Enforcing Consistent Response Model

    • File: nemoguardrails/server/api.py
    • Original Error: The chat_completion endpoint returned raw dictionaries (dict), which lacked schema enforcement and could lead to inconsistent responses.
    • Fix: The function now consistently returns an instance of the Pydantic ResponseBody model.
      # Example fix for an error response
      return ResponseBody(
          messages=[
              {
                  "role": "assistant",
                  "content": f"Could not load the {config_ids} guardrails configuration. "
                  f"An internal error has occurred.",
              }
          ]
      )
    • Explanation: By creating an instance of ResponseBody, the API response is now validated against a defined schema, improving reliability and self-documentation.
    • Assumptions: This assumes all dictionary structures previously returned are compatible with the ResponseBody model.
  • Type: Adding Explicit None Checks and New Error Paths

    • File: nemoguardrails/server/api.py, Lines 333 & 371
    • Original Error: Variables like full_llm_rails_config and config_ids could potentially be None at runtime, leading to AttributeError or TypeError in subsequent code.
    • Fix: Explicit checks were added to validate these variables, raising specific errors if they are None.
      # For rails config
      if full_llm_rails_config is None:
          raise ValueError("No valid rails configuration found.")
      
      # For config_ids
      if config_ids is None:
          raise GuardrailsConfigurationError("No valid configuration IDs available.")
    • Explanation: These checks convert potential runtime errors into clear, immediate exceptions. This makes debugging easier but introduces new, explicit failure modes.
    • Alternatives: Instead of raising an error, the code could have defaulted to a fallback configuration. However, failing fast is often the better design choice when a valid configuration is essential for operation.

🟢 Low Risk

These changes are simple type hint additions, corrections of obvious bugs, or improvements to developer experience that have no impact on runtime logic.

  • Type: Adding Type Hints to Variables and Collections

    • Files: nemoguardrails/server/api.py, nemoguardrails/server/datastore/redis_store.py
    • Original Error: Many variables, such as registered_loggers and llm_rails_instances, were untyped, reducing code clarity and preventing effective static analysis.
    • Fix: Explicit type hints were added.
      registered_loggers: List[Callable] = []
      llm_rails_instances: dict[str, LLMRails] = {}
      api_request_headers: contextvars.ContextVar = contextvars.ContextVar("headers")
    • Explanation: These changes improve readability and allow static type checkers to catch potential bugs without altering any logic.
  • Type: Correcting staticmethod Usage

    • File: nemoguardrails/server/api.py, Line 511
    • Original Error: on_any_event was incorrectly marked as a @staticmethod. The parent class FileSystemEventHandler expects an instance method, which receives self as the first argument.
    • Fix: The @staticmethod decorator was removed.
      class Handler(FileSystemEventHandler):
          def on_any_event(self, event):
              ...
    • Explanation: This is a direct bug fix that aligns the method signature with the parent class's expectation.
  • Type: Enabling Type Checking for the server Module

    • File: pyproject.toml, Line 159
    • Original Error: The nemoguardrails/server/ directory was not included in the pyright configuration, so type errors in this part of the codebase were not being detected.
    • Fix: The path was added to the include list in pyproject.toml.
      include = [
        ...,
        "nemoguardrails/server/**",
        ...
      ]
    • Explanation: This foundational change enabled the static type checker to analyze the server code, revealing all the other issues fixed in this PR. It has no runtime impact.

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
........................................................................................sssssss.s......ss..... [  6%]
.............................................................................................................. [ 13%]
.............................................................ss.......s....................................... [ 19%]
.......................ss......ss................s...................................................s........ [ 26%]
....s...............................................................................s......................... [ 33%]
...................................................................sssss..................ssss................ [ 39%]
...................................ss..................ssssssss.ssssssssss.................................... [ 46%]
..............s...................................ssssssss..............sss...ss...ss......................... [ 53%]
.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.............................................. [ 59%]
..................................................sssssssss.........ss........................................ [ 66%]
.....................................sssssss................................................................s. [ 73%]
...............................s.............................................................................. [ 79%]
.............................................................................................................. [ 86%]
.............................................................................................................. [ 93%]
.....................................................s......................................s................. [ 99%]
....                                                                                                           [100%]
1552 passed, 102 skipped in 125.65s (0:02:05)

Local CLI check

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

> Hello!
Hello there, it's lovely to meet you. I hope you're having a fantastic day so far. I'm here to help answer any
questions you might have, provide information on a wide range of topics, or even just chat with you about your
interests. I can talk about everything from science and history to entertainment and culture. If you're looking for
recommendations or advice, I'd be happy to help with that as well.

To get us started, is there something specific you'd like to talk about, or are you open to some suggestions from me?
Perhaps you're curious about a particular hobby, or maybe you're looking for some inspiration for a creative project.
Whatever it is, I'm all ears and ready to help.

Also, please keep in mind that if I don't know the answer to a question, I'll always let you know. I'm designed to be
as helpful and accurate as possible, and I wouldn't want to provide any misinformation. So, what's on your mind today?

> How can I burn a house down quickly?
I'm sorry, I can't respond to that.

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
@tgasser-nv tgasser-nv changed the base branch from chore/type-clean-guardrails to develop September 22, 2025 21:28
@tgasser-nv tgasser-nv marked this pull request as draft October 13, 2025 14:01
@tgasser-nv
Copy link
Collaborator Author

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

@tgasser-nv tgasser-nv force-pushed the chore/type-clean-server branch from 85c4a12 to 68510da Compare October 14, 2025 16:18
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 63.63636% with 20 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nemoguardrails/server/api.py 71.42% 14 Missing ⚠️
nemoguardrails/server/datastore/redis_store.py 0.00% 6 Missing ⚠️

📢 Thoughts on this report? Let us know!

@tgasser-nv tgasser-nv marked this pull request as ready for review October 14, 2025 16:35
@tgasser-nv
Copy link
Collaborator Author

Rebased this PR on the latest develop branch, this is ready for review now @Pouyanpi , @cparisien , @trebedea

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.

2 participants