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: local package dep fix #303

Merged
merged 3 commits into from
May 31, 2024
Merged

fix: local package dep fix #303

merged 3 commits into from
May 31, 2024

Conversation

jamescalam
Copy link
Member

@jamescalam jamescalam commented May 31, 2024

User description

Due to constraints in current fastembed dependencies, I have temporarily dropped the package so that HuggingFace and other local package dependencies can still function. In the meantime user's can install fastembed separately to use the fastembed encoders.

See related issue in fastembed repo here


PR Type

Bug fix, Enhancement


Description

  • Updated the version number from 0.0.44 to 0.0.45.
  • Removed fastembed dependency to resolve local package conflicts.
  • Added tokenizers as a new dependency.
  • Adjusted version constraints for transformers to ensure compatibility.

Changes walkthrough 📝

Relevant files
Enhancement
__init__.py
Bump version number to 0.0.45                                                       

semantic_router/init.py

  • Updated the version number from 0.0.44 to 0.0.45.
+1/-1     
Bug fix
pyproject.toml
Update dependencies and version in pyproject.toml               

pyproject.toml

  • Updated the version number from 0.0.44 to 0.0.45.
  • Removed fastembed dependency.
  • Added tokenizers dependency.
  • Adjusted version constraints for transformers.
  • +4/-5     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @jamescalam jamescalam added the bug Something isn't working label May 31, 2024
    @github-actions github-actions bot added enhancement Enhancement to existing features Bug fix labels May 31, 2024
    Copy link

    PR Description updated to latest commit (d3468ff)

    @jamescalam jamescalam changed the title James/local package dep fix fix: local package dep fix May 31, 2024
    Copy link

    github-actions bot commented May 31, 2024

    PR Review 🔍

    (Review updated until commit d3468ff)

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and involve version updates and dependency modifications. The PR is small and the changes are clear, making it relatively easy to review.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Dependency Compatibility: Removing fastembed and adding tokenizers might introduce compatibility issues with existing code that relies on fastembed. It's crucial to ensure that all functionalities work as expected without fastembed.

    🔒 Security concerns

    No

    Code feedback:
    relevant filepyproject.toml
    suggestion      

    Consider specifying a more precise version range for the tokenizers dependency to avoid potential future compatibility issues with other packages. For example, you could use tokenizers = "^0.19.0" to ensure compatibility with minor updates but not major changes that could break functionality. [important]

    relevant linetokenizers = {version = ">=0.19", optional = true}

    relevant filepyproject.toml
    suggestion      

    Since fastembed is removed from the dependencies, it's important to ensure that no residual code or references to fastembed remain in the project that could cause runtime errors. A thorough search in the codebase for any fastembed imports or usage would be prudent. [important]

    relevant line-fastembed = {version = "^0.2.4", optional = true, python = "<3.12"}

    relevant filepyproject.toml
    suggestion      

    It might be beneficial to add a comment next to the transformers dependency explaining why the version constraint was changed to >=4.36.2. This can help maintainers understand the reason for this change, especially if it's related to compatibility or specific features needed from the newer versions. [medium]

    relevant linetransformers = {version = ">=4.36.2", optional = true}

    relevant filesemantic_router/__init__.py
    suggestion      

    Ensure that all references to the version number in the codebase, documentation, and possibly CI/CD configurations are updated to reflect the new version 0.0.45. This helps in maintaining consistency and avoiding issues during deployment or package releases. [medium]

    relevant line__version__ = "0.0.45"

    Copy link

    PR Description updated to latest commit (d3468ff)

    Copy link

    Persistent review updated to latest commit d3468ff

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Adjust the version constraints for transformers and tokenizers to be more restrictive to avoid potential compatibility issues

    Ensure that the version constraints for transformers and tokenizers are consistent with
    the rest of the dependencies. The current change introduces a more permissive version
    constraint for transformers and tokenizers, which might lead to compatibility issues with
    other packages or the project's environment.

    pyproject.toml [29-30]

    -transformers = {version = ">=4.36.2", optional = true}
    -tokenizers = {version = ">=0.19", optional = true}
    +transformers = {version = "^4.36.2", optional = true}
    +tokenizers = {version = "^0.19", optional = true}
     
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a potential compatibility issue by making the version constraints more restrictive, which is crucial for maintaining a stable environment.

    9
    Maintainability
    Implement dynamic versioning by fetching the package version directly from the project's metadata

    Consider using a more dynamic approach for version management to avoid manual updates.
    This can be done by reading the version from a single source like pyproject.toml or
    another configuration file, which helps in maintaining consistency across the project.

    semantic_router/init.py [7]

    -__version__ = "0.0.45"
    +from importlib.metadata import version
    +__version__ = version("semantic-router")
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves maintainability by centralizing version management, reducing the risk of inconsistencies across the project.

    8
    Enhancement
    Add fastembed back to the local extras to ensure no loss of functionality

    Consider adding back fastembed or providing a replacement if it was intentionally removed.
    The removal of fastembed from the dependencies and extras without a clear replacement
    might lead to missing functionality that was previously available.

    pyproject.toml [46]

    -local = ["torch", "transformers", "tokenizers", "llama-cpp-python"]
    +local = ["torch", "transformers", "tokenizers", "llama-cpp-python", "fastembed"]
     
    Suggestion importance[1-10]: 7

    Why: The suggestion is valid as it ensures no loss of functionality by re-adding fastembed, but it may not be necessary if the removal was intentional and the functionality is no longer needed.

    7
    Best practice
    Ensure essential dependencies are not marked as optional if they are critical for the package's operation

    Verify the necessity of marking transformers and tokenizers as optional dependencies. If
    these packages are essential for the local extras functionality, consider removing the
    optional = true flag to ensure they are installed with the package.

    pyproject.toml [29-30]

    -transformers = {version = ">=4.36.2", optional = true}
    -tokenizers = {version = ">=0.19", optional = true}
    +transformers = {version = ">=4.36.2", optional = false}
    +tokenizers = {version = ">=0.19", optional = false}
     
    Suggestion importance[1-10]: 6

    Why: The suggestion is reasonable for ensuring critical dependencies are always installed, but it might not be necessary if the packages are truly optional for certain use cases.

    6

    @jamescalam jamescalam merged commit 590cf2d into main May 31, 2024
    3 of 6 checks passed
    @jamescalam jamescalam deleted the james/local-package-dep-fix branch May 31, 2024 04:51
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix bug Something isn't working enhancement Enhancement to existing features Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    1 participant