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: function schemas #463

Merged
merged 3 commits into from
Nov 20, 2024
Merged

fix: function schemas #463

merged 3 commits into from
Nov 20, 2024

Conversation

jamescalam
Copy link
Member

@jamescalam jamescalam commented Nov 20, 2024

PR Type

Bug fix


Description

  • Fixed the function schemas handling in from_tuple method by ensuring function_schemas is a list, even if initially provided as a dictionary.
  • Updated the sync strategy method call to use get_sync_strategy with the sync_mode parameter in sync method.

Changes walkthrough 📝

Relevant files
Bug fix
layer.py
Update sync strategy method call with parameter                   

semantic_router/layer.py

  • Changed method call from to_sync_strategy to get_sync_strategy with
    sync_mode parameter.
  • +1/-1     
    schema.py
    Fix function schemas handling in from_tuple method             

    semantic_router/schema.py

  • Added check for function_schemas to convert dict to list if necessary.

  • +2/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Consistency
    Ensure that the transformation of function_schemas from a dictionary to a list is handled consistently across the application. This change might affect other parts of the system expecting function_schemas in its original dictionary format.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Verify the handling of the sync_mode parameter in the get_sync_strategy method to maintain application consistency

    Ensure that the get_sync_strategy method handles the sync_mode parameter correctly,
    especially if the method's behavior significantly differs from the previously used
    to_sync_strategy. This is crucial if the sync modes are not compatible or if the new
    method introduces different handling that could affect the application's state or
    data consistency.

    semantic_router/layer.py [491]

    -sync_strategy = diff.get_sync_strategy(sync_mode=sync_mode)
    +sync_strategy = diff.get_sync_strategy(sync_mode=sync_mode)  # Ensure compatibility with all sync modes
    Suggestion importance[1-10]: 1

    Why: The suggestion is non-actionable as it only advises to ensure compatibility without providing a specific code change or identifying a concrete issue.

    1
    Ensure the type conversion of function_schemas does not disrupt application functionality

    Validate that the conversion of function_schemas from a dictionary to a list is
    appropriate for all use cases in the application. This change could impact how
    functions schemas are processed elsewhere in the application, potentially leading to
    type errors or logic bugs if other parts of the code expect a dictionary instead of
    a list.

    semantic_router/schema.py [101-102]

     if isinstance(function_schemas, dict):
    -    function_schemas = [function_schemas]
    +    function_schemas = [function_schemas]  # Confirm compatibility with expected data types in all usages
    Suggestion importance[1-10]: 1

    Why: Similar to the first suggestion, this is non-actionable as it only advises checking compatibility without suggesting a specific improvement or identifying a definite problem.

    1

    Copy link

    codecov bot commented Nov 20, 2024

    Codecov Report

    Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

    Project coverage is 72.29%. Comparing base (06e3a6a) to head (a5103d6).
    Report is 6 commits behind head on main.

    Files with missing lines Patch % Lines
    semantic_router/schema.py 50.00% 1 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #463      +/-   ##
    ==========================================
    + Coverage   72.09%   72.29%   +0.19%     
    ==========================================
      Files          39       39              
      Lines        3290     3292       +2     
    ==========================================
    + Hits         2372     2380       +8     
    + Misses        918      912       -6     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.


    🚨 Try these New Features:

    @jamescalam jamescalam merged commit 15c38d0 into main Nov 20, 2024
    8 checks passed
    @jamescalam jamescalam deleted the james/function-schema-fix branch November 20, 2024 14:28
    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.

    1 participant