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: catch RuntimeError for missing torch import #532

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bruvduroiu
Copy link
Member

@bruvduroiu bruvduroiu commented Feb 11, 2025

PR Type

Bug fix, Tests


Description

  • Catch RuntimeError for missing torch import in HuggingFaceEncoder.

  • Update test case to validate transformers import error handling.

  • Improve robustness of import error handling in HuggingFaceEncoder.


Changes walkthrough 📝

Relevant files
Bug fix
huggingface.py
Enhance import error handling in HuggingFaceEncoder           

semantic_router/encoders/huggingface.py

  • Added RuntimeError handling for missing torch import.
  • Improved error message for missing transformers library.
  • Adjusted exception handling logic for robustness.
  • +5/-5     
    Tests
    test_huggingface.py
    Update test for import error handling in HuggingFaceEncoder

    tests/unit/encoders/test_huggingface.py

  • Updated test to validate transformers import error handling.
  • Adjusted assertion to match updated error message.
  • +3/-1     

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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Possible Error Message Mismatch

    The error message for the RuntimeError in the _initialize_hf_model method might be misleading as it mentions installing transformers instead of torch. This should be validated to ensure clarity.

    except (ImportError, RuntimeError):
        raise ImportError(
            "Please install transformers to use HuggingFaceEncoder. "
            "You can install it with: "
            "`pip install semantic-router[local]`"
    Test Assertion Mismatch

    The updated test assertion for missing torch import now checks for the transformers error message instead of the Pytorch error message. This should be reviewed to confirm it aligns with the intended behavior.

    assert "Please install transformers to use HuggingFaceEncoder" in str(
        error.value
    )

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Differentiate error messages for clarity

    Ensure that the RuntimeError exception handling for the torch import provides a more
    specific error message, as it currently uses the same message as the ImportError
    case, which might confuse users.

    semantic_router/encoders/huggingface.py [62-66]

     except RuntimeError:
    -    raise ImportError(
    -        "Please install Pytorch to use HuggingFaceEncoder. "
    -        "You can install it with: "
    -        "`pip install semantic-router[local]`"
    +    raise RuntimeError(
    +        "A runtime error occurred while importing Pytorch. "
    +        "Ensure your Pytorch installation is compatible with your system."
         )
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion improves error clarity by providing a distinct message for RuntimeError during the torch import. This differentiation helps users identify the specific issue, enhancing usability and debugging.

    Medium
    Test both ImportError and RuntimeError

    Update the test assertion to include a check for the RuntimeError case when torch is
    missing, ensuring both ImportError and RuntimeError scenarios are tested.

    tests/unit/encoders/test_huggingface.py [29-31]

    -assert "Please install transformers to use HuggingFaceEncoder" in str(
    -    error.value
    -)
    +assert "Please install transformers to use HuggingFaceEncoder" in str(error.value) or \
    +       "A runtime error occurred while importing Pytorch" in str(error.value)
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion ensures comprehensive testing by including a check for RuntimeError in addition to ImportError. This enhances test coverage and robustness, aligning with the changes in the main code.

    Medium

    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.

    2 participants