Skip to content

Conversation

@JashG
Copy link
Contributor

@JashG JashG commented Nov 17, 2025

Description

Related Issue(s)

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.

Comment on lines -149 to -151
# FIXME: is text and chat a good idea?
# For text mode, use text completion, we are using both text and chat as the last resort
ModelInitializer(_init_text_completion_model, ["text", "chat"]),
Copy link
Contributor Author

@JashG JashG Nov 18, 2025

Choose a reason for hiding this comment

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

The NeMo Guardrails MS uses a custom chat client for main models. We often face this generic error:

nemoguardrails.llm.models.langchain_initializer.ModelInitializationError: Failed to initialize model 'meta/llama-3.3-70b-instruct' with provider 'nimchat' in 'chat' mode: Could not find LLM provider 'nimchat'

However, the relevant exception was thrown by our custom class, but gets swallowed here by the exception thrown by _init_text_completion_model.

I repositioned _init_text_completion_model to come before ModelInitializer(_init_community_chat_models, ["chat"]), to ensure the relevant exception is bubbled up. Let me know if there are any issues with this.

@JashG JashG changed the title feat: Add custom exceptions fix: Propagate model and base URL in LLMCallException; improve error handling Nov 19, 2025
@JashG JashG marked this pull request as ready for review November 19, 2025 17:25
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 19, 2025

Greptile Summary

  • Enhanced LLMCallException to propagate model name and endpoint URL from various provider-specific attributes for better debugging
  • Introduced centralized exception hierarchy (ConfigurationError, InvalidModelConfigurationError, InvalidRailsConfigurationError) with improved error messages throughout config validation
  • Reordered model initializers to prioritize text completion before chat/community models (FIXME comment indicates this behavior change needs verification)

Confidence Score: 3/5

  • This PR has one behavioral change that requires verification before merging
  • The exception handling improvements and new exception types are well-tested and safe. However, the reordering of model initializers in langchain_initializer.py changes fallback behavior (text completion now runs before chat models), which could affect existing configurations. The FIXME comment suggests the author is uncertain about this change.
  • Pay close attention to nemoguardrails/llm/models/langchain_initializer.py - verify the initializer reordering doesn't break existing chat model configurations

Important Files Changed

Filename Overview
nemoguardrails/exceptions.py New file introducing centralized exception hierarchy with LLMCallException enhanced to include context messages
nemoguardrails/actions/llm/utils.py Added _raise_llm_call_exception helper to enrich errors with model name and endpoint URL from multiple provider-specific attributes
nemoguardrails/llm/models/langchain_initializer.py Reordered model initializers to try text completion before chat/community models; FIXME comment indicates uncertainty about this change

Sequence Diagram

sequenceDiagram
    participant User
    participant LLMRails
    participant ActionDispatcher
    participant LLMUtils as "llm/utils"
    participant LangChainModel as "LangChain Model"
    participant ExceptionHandler as "Exception Handler"

    User->>LLMRails: "generate_async(prompt)"
    LLMRails->>ActionDispatcher: "execute action"
    ActionDispatcher->>LLMUtils: "llm_call(llm, prompt)"
    LLMUtils->>LangChainModel: "ainvoke(prompt)"
    
    alt Success
        LangChainModel-->>LLMUtils: "response"
        LLMUtils-->>ActionDispatcher: "response"
    else Failure
        LangChainModel-->>LLMUtils: "Exception"
        LLMUtils->>ExceptionHandler: "_raise_llm_call_exception(e, llm)"
        ExceptionHandler->>ExceptionHandler: "extract model_name from llm"
        ExceptionHandler->>ExceptionHandler: "extract endpoint_url from llm attributes"
        ExceptionHandler->>ExceptionHandler: "build context message"
        ExceptionHandler-->>LLMUtils: "raise LLMCallException(e, context)"
        LLMUtils-->>ActionDispatcher: "LLMCallException"
        ActionDispatcher-->>User: "Error with model and endpoint context"
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (2)

  1. tests/test_embeddings_only_user_messages.py, line 21 (link)

    syntax: Import needs updating to use the new location

  2. tests/test_tool_calling_utils.py, line 22 (link)

    syntax: Import needs updating - LLMCallException was moved

8 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

10 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +145 to 151
# FIXME: is text and chat a good idea?
# For text mode, use text completion, we are using both text and chat as the last resort
ModelInitializer(_init_text_completion_model, ["text", "chat"]),
# For chat mode, first try the standard chat completion API
ModelInitializer(_init_chat_completion_model, ["chat"]),
# For chat mode, fall back to community chat models
ModelInitializer(_init_community_chat_models, ["chat"]),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: reordering changes fallback behavior - text completion now runs before chat/community models. FIXME comment suggests uncertainty. verify this doesn't break existing configurations expecting chat completion first

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 90.27778% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nemoguardrails/rails/llm/config.py 78.94% 4 Missing ⚠️
nemoguardrails/rails/llm/llmrails.py 81.81% 2 Missing ⚠️
nemoguardrails/actions/llm/utils.py 96.29% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

12 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +145 to +147
# FIXME: is text and chat a good idea?
# For text mode, use text completion, we are using both text and chat as the last resort
ModelInitializer(_init_text_completion_model, ["text", "chat"]),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: text completion now runs before chat models - verify this doesn't break existing chat configurations that expect chat completion to be tried first. the FIXME suggests uncertainty about this ordering.

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