Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(agents-api): fix model=None case for chat endpoint #1206

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

Ahmad-mtos
Copy link
Contributor

@Ahmad-mtos Ahmad-mtos commented Mar 3, 2025

User description

closes #1060


EntelligenceAI PR Summary

Purpose:

  • Enhancements to model validation and schema changes streamline the codebase and improve document search flexibility.

Changes:

  • Enhancement: Model validation moved to settings for better accuracy.
  • Refactor: Consolidated ApiCallDef and SpiderIntegrationDef in schema files.
  • New Feature: Added TextOnlyDocSearch and VectorDocSearch for improved search capabilities.
  • Test: Debug statements and TODOs added for model validation testing.

Impact:

  • Enhances validation accuracy, codebase efficiency, and search flexibility, boosting reliability and user experience.

PR Type

Bug fix, Enhancement, Tests


Description

  • Fixed validation for None model in chat endpoint.

  • Enhanced schema definitions by consolidating redundant entries.

  • Added new TextOnlyDocSearch and VectorDocSearch schema types.

  • Introduced tests for None model validation and improved error handling.


Changes walkthrough 📝

Relevant files
Enhancement
render.py
Refactored model validation in chat endpoint                         

agents-api/agents_api/routers/sessions/render.py

  • Moved model validation to settings for better accuracy.
  • Removed redundant validation logic for chat_input.model.
  • +2/-3     
    create_agent_request.json
    Enhanced schema definitions with new types                             

    schemas/create_agent_request.json

  • Consolidated ApiCallDef and SpiderIntegrationDef schema definitions.
  • Added TextOnlyDocSearch and VectorDocSearch schema types.
  • Adjusted default values for HybridDocSearch properties.
  • +262/-260
    create_task_request.json
    Enhanced schema definitions with new types                             

    schemas/create_task_request.json

  • Consolidated ApiCallDef and SpiderIntegrationDef schema definitions.
  • Added TextOnlyDocSearch and VectorDocSearch schema types.
  • Adjusted default values for HybridDocSearch properties.
  • +262/-260
    Bug fix
    model_validation.py
    Enhanced model validation logic                                                   

    agents-api/agents_api/routers/utils/model_validation.py

  • Updated validate_model to handle None as input.
  • Improved error handling for unavailable models.
  • +1/-1     
    Tests
    test_model_validation.py
    Added tests for model validation                                                 

    agents-api/tests/test_model_validation.py

  • Added new test case for None model validation.
  • Updated test descriptions for clarity.
  • +13/-2   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    Walkthrough

    The update enhances the model validation process by relocating it from chat_input to the settings dictionary, improving validation accuracy. It also introduces significant schema changes, consolidating input and output definitions for API calls and Spider integration, and adding new document search mode definitions. These changes streamline the codebase and enhance document search flexibility.

    Changes

    File(s) Summary
    agents-api/agents_api/routers/sessions/render.py Moved model validation from chat_input to settings dictionary.
    agents-api/agents_api/routers/utils/model_validation.py Added debug print statements for model validation.
    agents-api/tests/test_model_validation.py Added a TODO comment for testing when the model is None.
    schemas/create_agent_request.json, schemas/create_task_request.json Consolidated ApiCallDef and SpiderIntegrationDef input/output definitions. Added TextOnlyDocSearch and VectorDocSearch definitions.
    Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below

    Emoji Descriptions:

    • ⚠️ Potential Issue - May require further investigation.
    • 🔒 Security Vulnerability - Fix to ensure system safety.
    • 💻 Code Improvement - Suggestions to enhance code quality.
    • 🔨 Refactor Suggestion - Recommendations for restructuring code.
    • ℹ️ Others - General comments and information.

    Interact with the Bot:

    • Send a message or request using the format:
      @bot + *your message*
    Example: @bot Can you suggest improvements for this code?
    
    • Help the Bot learn by providing feedback on its responses.
      @bot + *feedback*
    Example: @bot Do not comment on `save_auth` function !
    

    @Ahmad-mtos Ahmad-mtos marked this pull request as ready for review March 3, 2025 12:18
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    1060 - Partially compliant

    Compliant requirements:

    • Fix error handling for unsupported models

    Non-compliant requirements:

    • Wrap internal server error in a nice message when model is not supported

    Requires further human verification:

    • Verify that the error message shown to users is clear and helpful
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Validation Order

    Model validation happens after merging settings, which could allow invalid settings to be merged before validation occurs. Consider validating model before merging.

    chat_context.merge_settings(chat_input)
    settings: dict = chat_context.settings or {}
    
    await validate_model(settings.get("model"))

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate empty string input

    Add input validation to ensure model_name is not an empty string, as this could
    lead to ambiguous error messages.

    agents-api/agents_api/routers/utils/model_validation.py [7-10]

     async def validate_model(model_name: str | None) -> None:
         """
         Validates if a given model name is available in LiteLLM.
         Raises HTTPException if model is not available.
    +    """
    +    if model_name == "":
    +        raise HTTPException(status_code=400, detail="Model name cannot be empty")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds important input validation to prevent empty string model names, which could lead to confusing error messages or undefined behavior. This is a meaningful improvement to the robustness of the validation function.

    Medium
    Strengthen mode validation constraints

    Add validation to ensure mode values are consistent with schema types -
    TextOnlyDocSearch should only allow "text" mode, VectorDocSearch only "vector"
    mode, and HybridDocSearch only "hybrid" mode.

    schemas/create_task_request.json [5968-5973]

     "mode": {
    -  "const": "hybrid",
    +  "enum": ["hybrid"],
       "default": "hybrid",
       "title": "Mode",
       "type": "string"
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    __

    Why: The suggestion proposes using enum instead of const for mode validation, but both approaches are valid for enforcing a single allowed value. The change would have minimal impact since const already provides the needed validation.

    Low
    • More

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Reviewed everything up to b2599e5 in 4 minutes and 12 seconds

    More details
    • Looked at 1643 lines of code in 5 files
    • Skipped 0 files when reviewing.
    • Skipped posting 17 drafted comments based on config settings.
    1. schemas/create_agent_request.json:11696
    • Draft comment:
      This schema duplicates the existing VectorDocSearchRequest Pydantic model. Consider reusing the existing model instead of creating a new schema definition.

    • class VectorDocSearchRequest (Docs.py)

    • Reason this comment was not posted:
      Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
      The comment appears to be incorrect. While there is some overlap in fields between VectorDocSearch and VectorDocSearchRequest, they serve different purposes and have different field sets. VectorDocSearch appears to be a more complete configuration schema while VectorDocSearchRequest seems to be a more focused request schema. This is a common pattern in APIs where you have different schemas for different use cases even if they share some fields. The comment also violates the rule about only commenting on changed code, since it's referencing an existing model rather than focusing on the changes.
      Could I be missing some architectural context where reusing the model would actually be beneficial? Perhaps there's a broader pattern of model reuse I'm not seeing?
      Even if model reuse might be beneficial in some cases, the schemas clearly serve different purposes with different field sets. Additionally, per the review rules, we should only comment on the changes themselves, not suggest architectural changes based on existing code.
      The comment should be deleted because it incorrectly suggests the schemas are duplicates when they serve different purposes with different fields, and it violates the rule about only commenting on the changes themselves.

    2. agents-api/agents_api/routers/sessions/render.py:80
    • Draft comment:
      Instead of validating chat_input.model directly, this uses settings.get('model'). Ensure that merging settings doesn’t override a valid model provided in the request unintentionally.
    • Reason this comment was not posted:
      Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
      The comment raises a reasonable point about potential model overrides. However, merge_settings() is likely designed to handle this properly, and without seeing its implementation, we can't be certain there's an actual issue. The code change appears intentional to validate the final merged model rather than just the input model. The suggestion to revert seems speculative without strong evidence of a problem.
      I don't have visibility into how merge_settings() works or what the intended behavior is. The current code could be the correct approach if we want to validate the final model after merging all settings.
      Without clear evidence of a bug or seeing merge_settings() implementation, this comment is speculative and asks for verification rather than pointing out a definite issue.
      The comment should be deleted as it's speculative and asks for verification without strong evidence of an actual problem.
    3. agents-api/agents_api/routers/utils/model_validation.py:4
    • Draft comment:
      Updated function signature to accept None, but consider explicitly checking for None and returning a clear, user‐friendly error message.
    • Reason this comment was not posted:
      Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
      The current implementation will already handle None gracefully - it will result in a 400 error saying "Model None not available". This is actually a perfectly clear and user-friendly error message. Adding a special case for None would just duplicate error handling logic without adding value. The comment suggests adding complexity that isn't necessary.
      Maybe None deserves a more specific error message like "Model name cannot be null" instead of treating it like any other invalid model name?
      The current error message is actually more helpful because it shows the available models list, which is more actionable than just saying the name can't be null.
      The comment should be deleted because the current implementation already handles None gracefully with a clear error message, and adding special None handling would be unnecessary complexity.
    4. agents-api/tests/test_model_validation.py:26
    • Draft comment:
      Test updated to expect failure when model is None. Confirm that the error message 'Model None not available' is acceptable for end users.
    • Reason this comment was not posted:
      Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
      The comment is asking the PR author to confirm if the error message is acceptable for end users. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
    5. schemas/create_agent_request.json:102
    • Draft comment:
      Renaming 'ApiCallDef-Input' and similar definitions improves consistency but ensure that any downstream clients or integrations are updated accordingly.
    • Reason this comment was not posted:
      Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
      The comment is asking the PR author to ensure that downstream clients or integrations are updated, which violates the rule against asking the author to ensure things are tested or verified. It does not provide a specific code suggestion or ask for a specific test to be written.
    6. schemas/create_agent_request.json:3734
    • Draft comment:
      Adding '$ref' for VectorDocSearch and TextOnlyDocSearch in 'recall_options' enhances search flexibility. Verify that the new schemas meet all integration tests and client expectations.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    7. schemas/create_agent_request.json:6056
    • Draft comment:
      Updated HybridDocSearch alpha default from 0.75 to 0.5 and added a const constraint for 'mode'. Ensure that this adjustment is intentional and consistently applied across all environments.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    8. schemas/create_agent_request.json:10664
    • Draft comment:
      New entries for TextOnlyDocSearch and VectorDocSearch definitions are added. Confirm validation constraints (e.g., defaults, limits, const for mode) match requirements thoroughly.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    9. schemas/create_task_request.json:102
    • Draft comment:
      Renaming 'ApiCallDef-Input' to 'ApiCallDef' is repeated in create_task_request.json for consistency. Make sure any backward compatibility concerns are addressed.
    • Reason this comment was not posted:
      Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
      The comment is asking the author to ensure backward compatibility, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. It is more of a general cautionary note.
    10. agents-api/agents_api/routers/sessions/render.py:80
    • Draft comment:
      Ensure that chat_context.merge_settings properly copies chat_input.model into settings, so that validate_model receives the intended value rather than unnecessarily receiving None.
    • Reason this comment was not posted:
      Comment looked like it was already resolved.
    11. agents-api/agents_api/routers/utils/model_validation.py:4
    • Draft comment:
      Since the function now accepts a None value, consider refining the error message to clarify if no model was provided (e.g., 'No model specified' vs. 'Model None not available').
    • Reason this comment was not posted:
      Confidence changes required: 50% <= threshold 50%
      None
    12. schemas/create_agent_request.json:102
    • Draft comment:
      Renaming 'ApiCallDef-Input' to 'ApiCallDef' is a significant breaking change. Please ensure that all downstream consumers are updated to use the new definition.
    • Reason this comment was not posted:
      Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
      The comment is asking the PR author to ensure that all downstream consumers are updated, which violates the rule against asking the author to ensure compatibility or test changes. It does not provide a specific code suggestion or ask for a specific test to be written.
    13. schemas/create_agent_request.json:6056
    • Draft comment:
      The default alpha value in HybridDocSearch has been changed from 0.75 to 0.5. Confirm that this adjustment is intentional and that related functionality has been tested.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    14. schemas/create_agent_request.json:9686
    • Draft comment:
      SpiderIntegrationDef now contains only the 'arguments' property; properties like 'method', 'provider', and 'setup' have been removed. Verify that this intentional removal matches the current integration requirements.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    15. schemas/create_agent_request.json:3734
    • Draft comment:
      The recall_options now include new alternatives ($ref to VectorDocSearch and TextOnlyDocSearch). Ensure that downstream systems correctly handle these new search types.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    16. schemas/create_task_request.json:102
    • Draft comment:
      Similar to the agent request schema, 'ApiCallDef-Input' has been renamed to 'ApiCallDef' in the task request schema. Ensure that all client integrations referencing these schemas are aware of this breaking change.
    • Reason this comment was not posted:
      Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
      The comment is asking the PR author to ensure that all client integrations are aware of a breaking change. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
    17. schemas/create_agent_request.json:2920
    • Draft comment:
      Typo detected: The schema titled 'CompetionUsage' appears to be misspelled. Consider renaming it to 'CompletionUsage' for clarity and consistency.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.

    Workflow ID: wflow_9Nn12D0uYUF0KeOE


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    @Ahmad-mtos Ahmad-mtos merged commit 3aaf949 into dev Mar 3, 2025
    14 checks passed
    @Ahmad-mtos Ahmad-mtos deleted the x/none-model-validation branch March 3, 2025 12:26
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [Bug]: Getting Internal Server Error on unsupported models
    1 participant