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 the potential duplicate embeddings in the RAG module #1224

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

seehi
Copy link
Contributor

@seehi seehi commented Apr 24, 2024

User description

Features

  • Avoid duplicate embeddings. In some cases, such as pass the retriever_configs in from_docs, get_retriver rebuild index, but not reuse the embeddings actually.

Feature Docs

Influence

Result

image

Other


Type

enhancement, bug_fix


Description

  • Enhanced the RAGExample class with detailed docstrings and added exception handling to improve robustness.
  • Refactored SimpleEngine to use a transformation-based approach instead of a direct index, enhancing flexibility and maintainability.
  • Improved error handling in ConfigBasedFactory and modified value retrieval to prevent unnecessary exceptions.
  • Introduced dynamic index handling in RetrieverFactory to optimize retriever creation based on configurations.
  • Updated unit tests to align with the refactoring and new features in the codebase.

Changes walkthrough

Relevant files
Enhancement
rag_pipeline.py
Enhance RAGExample with Docstrings and Exception Handling

examples/rag_pipeline.py

  • Added detailed docstrings to the RAGExample class and its methods.
  • Introduced exception handling decorators to various asynchronous
    methods.
  • +9/-2     
    simple.py
    Refactor SimpleEngine to Use Transformations Instead of Direct Index

    metagpt/rag/engines/simple.py

  • Removed dependency on VectorStoreIndex and shifted to a
    transformation-based architecture.
  • Added a new method _from_nodes to create an engine instance from node
    transformations.
  • Refactored methods to use transformations instead of a direct index.
  • +49/-14 
    base.py
    Improve Error Handling and Configuration Value Retrieval in Factories

    metagpt/rag/factories/base.py

  • Updated error handling in ConfigBasedFactory to provide clearer error
    messages.
  • Modified _val_from_config_or_kwargs to return None instead of raising
    KeyError.
  • +11/-6   
    retriever.py
    Enhance Retriever Factory with Dynamic Index Handling       

    metagpt/rag/factories/retriever.py

  • Added decorators to handle index building or retrieval dynamically.
  • Refactored methods to support building or retrieving indexes based on
    configuration.
  • +69/-20 
    Tests
    test_simple.py
    Update Tests for SimpleEngine Refactoring                               

    tests/metagpt/rag/engines/test_simple.py

  • Updated tests to reflect changes in the SimpleEngine regarding
    transformations.
  • Removed outdated mocks related to VectorStoreIndex.
  • +7/-17   
    test_base.py
    Update Factory Base Tests for Configuration Handling         

    tests/metagpt/rag/factories/test_base.py

  • Updated tests to reflect new behavior of returning None instead of
    raising KeyError.
  • +2/-3     
    test_retriever.py
    Enhance Tests for Retriever Factory's Dynamic Index Handling

    tests/metagpt/rag/factories/test_retriever.py

  • Added tests for new dynamic index handling in the retriever factory.
  • +37/-13 

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

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Description updated to latest commit (eb564bb)

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR involves multiple complex changes across several files, including refactoring and enhancements in error handling, transformations, and retriever factories. The changes are substantial and affect core functionalities, requiring a thorough review to ensure that all new and modified functionalities are correctly implemented and do not introduce regressions.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Exception Handling: The use of a generic @handle_exception decorator might obscure the source of errors and make debugging more difficult. It's important to ensure that this does not hide valuable stack trace information or lead to unhandled exceptions in different parts of the application.

    Refactoring Impact: The shift from using direct index references to transformation-based approaches in SimpleEngine could lead to unexpected behaviors if not all use cases are covered by the updated implementation. This needs careful testing, especially in edge cases.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add error handling to the index building process in the decorator.

    Consider adding error handling for the case where build_index_func might raise an
    exception during the index building process. This will ensure the robustness of the
    get_or_build_index decorator.

    metagpt/rag/factories/retriever.py [40-44]

     def wrapper(self, config, **kwargs):
         index = self._extract_index(config, **kwargs)
         if index is not None:
             return index
    -    return build_index_func(self, config, **kwargs)
    +    try:
    +        return build_index_func(self, config, **kwargs)
    +    except Exception as e:
    +        # Handle or log the exception appropriately
    +        raise e
     
    Add specific exception handling to the index building method.

    Consider using a more specific exception handling or logging within the
    _build_index_from_vector_store method to provide clearer error messages and make debugging
    easier.

    metagpt/rag/factories/retriever.py [138-145]

    -storage_context = StorageContext.from_defaults(vector_store=vector_store)
    -index = VectorStoreIndex(
    -    nodes=self._extract_nodes(config, **kwargs),
    -    storage_context=storage_context,
    -    embed_model=self._extract_embed_model(config, **kwargs),
    -)
    -return index
    +try:
    +    storage_context = StorageContext.from_defaults(vector_store=vector_store)
    +    index = VectorStoreIndex(
    +        nodes=self._extract_nodes(config, **kwargs),
    +        storage_context=storage_context,
    +        embed_model=self._extract_embed_model(config, **kwargs),
    +    )
    +    return index
    +except Exception as e:
    +    logger.error(f"Failed to build index: {e}")
    +    raise
     
    Improve the specificity of the mock assertion for better test accuracy.

    Replace the direct assertion of mock_get_rankers.assert_called_once() with a more specific
    assertion that includes expected arguments to ensure the function is called correctly.

    tests/metagpt/rag/engines/test_simple.py [80]

    -mock_get_rankers.assert_called_once()
    +mock_get_rankers.assert_called_once_with(expected_arguments)
     
    Enhance the test by verifying that mock_get_retriever is called with the correct parameters.

    Ensure that mock_get_retriever is called with specific expected arguments to verify
    correct parameter passing and function behavior.

    tests/metagpt/rag/engines/test_simple.py [79]

    -mock_get_retriever.assert_called_once()
    +mock_get_retriever.assert_called_once_with(expected_configurations)
     
    Specify which exceptions to handle in the @handle_exception decorator for better error management.

    The decorator @handle_exception should include specific exception handling logic to manage
    different types of exceptions that might occur during the execution of the methods.

    examples/rag_pipeline.py [65]

    -@handle_exception
    +@handle_exception(exception_types=(IndexError, KeyError))
     
    Best practice
    Avoid potential mutable default argument issues.

    To avoid potential issues with mutable default arguments, consider using a more defensive
    approach when initializing transformations in the constructor.

    metagpt/rag/factories/retriever.py [74]

    -self._transformations = transformations or self._default_transformations()
    +self._transformations = transformations if transformations is not None else self._default_transformations()
     
    Use a custom exception for clearer and more specific error handling.

    Instead of raising a generic ValueError, consider creating a custom exception class for
    better error handling and clarity.

    metagpt/rag/factories/base.py [51]

    -raise ValueError(f"Unknown config: `{type(key)}`, {key}")
    +raise UnknownConfigError(f"Unknown config: `{type(key)}`, {key}")
     
    Maintainability
    Refactor repeated index handling code into a reusable method.

    Refactor the repeated pattern of checking and building indexes in multiple methods by
    creating a more generic method that can be reused, which will improve maintainability.

    metagpt/rag/factories/retriever.py [80]

    -index = self._extract_index(config, **kwargs)
    -if index is not None:
    -    return index
    -return self._build_faiss_index(config, **kwargs)
    +return self._get_or_build_index(config, self._build_faiss_index, **kwargs)
     
    Refactor the index creation into a more readable and maintainable structure.

    To improve code readability and reduce complexity, consider refactoring the
    _build_default_index method by separating concerns into smaller, more focused methods.

    metagpt/rag/factories/retriever.py [109-113]

    -index = VectorStoreIndex(
    -    nodes=self._extract_nodes(**kwargs),
    -    embed_model=self._extract_embed_model(**kwargs),
    -)
    -return index
    +nodes = self._extract_nodes(**kwargs)
    +embed_model = self._extract_embed_model(**kwargs)
    +return self._create_index(nodes, embed_model)
     
    Add logging for missing keys to improve debugging and traceability.

    Refactor the method _val_from_config_or_kwargs to log a warning when neither config nor
    kwargs contain the key, before returning None.

    metagpt/rag/factories/base.py [67]

    +logger.warning(f"Key '{key}' not found in config or kwargs.")
     return None
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @codecov-commenter
    Copy link

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 70.66%. Comparing base (e212e7b) to head (eb564bb).
    Report is 14 commits behind head on main.

    ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main    #1224      +/-   ##
    ==========================================
    + Coverage   70.60%   70.66%   +0.06%     
    ==========================================
      Files         314      314              
      Lines       18714    18753      +39     
    ==========================================
    + Hits        13213    13252      +39     
      Misses       5501     5501              

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

    @geekan geekan merged commit c779f69 into geekan:main Apr 24, 2024
    1 of 3 checks passed
    @seehi seehi deleted the fix-rag-redundant-embedding branch May 14, 2024 08:05
    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.

    None yet

    3 participants