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: Fix the mypy check error #1373

Merged
merged 4 commits into from
Apr 7, 2024
Merged

Conversation

yyhhyyyyyy
Copy link
Contributor

Description

This PR attempts to fix the 'make mymp' error outlined in CONTRIBUTING.md that was introduced by PR #1359, particularly when submitting new PRs. Due to the lack of a Milvus environment on my end, I would kindly ask for testing before merging to ensure the issue with PR #1359 has been resolved. Apologies for the inconvenience.

How Has This Been Tested?

make fmt
make test
make mypy

Snapshots:

Due to working on an internal network, I cannot submit screenshots. Please proceed with direct testing.

Checklist:

  • My code follows the style guidelines of this project
  • I have already rebased the commits and make the commit message conform to the project standard.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged and published in downstream modules

@csunny
Copy link
Collaborator

csunny commented Apr 3, 2024

And when remove the code

if vector_store_config.embedding_fn is None:
    raise ValueError("embedding_fn is required for MilvusStore")

make mypy will raise an error.

dbgpt/storage/vector_store/milvus_store.py:181: error: Incompatible types in assignment (expression has type "Embeddings | None", variable has type "Embeddings")  [assignment]
Found 1 error in 1 file (checked 94 source files)
make: *** [mypy] Error 1

This issue should not fixed use remove embedding_fn config. @Aries-ckt

@csunny
Copy link
Collaborator

csunny commented Apr 3, 2024

When we delete knowledge space at dbgpt/app/knowledge/service, Each knowledge space should have a saved embedding_fn model parameter. We can query this embedding_fn according to knowledge space when later embedding_fn is not equal to pre, we need to raise an error.

    def delete_space(self, space_name: str):
        """delete knowledge space
        Args:
            - space_name: knowledge space name
        """
        query = KnowledgeSpaceEntity(name=space_name)
        spaces = knowledge_space_dao.get_knowledge_space(query)
        if len(spaces) == 0:
            raise Exception(f"delete error, no space name:{space_name} in database")
        space = spaces[0]
        # TODO query embedding_fn according space.name
        config = VectorStoreConfig(name=space.name) 
        ...
        # delete vectors
        vector_store_connector.delete_vector_name(space.name)
        document_query = KnowledgeDocumentEntity(space=space.name)
        # delete chunks
        documents = knowledge_document_dao.get_documents(document_query)
        for document in documents:
            document_chunk_dao.raw_delete(document.id)
        # delete documents
        knowledge_document_dao.raw_delete(document_query)
        # delete space
        return knowledge_space_dao.delete_knowledge_space(space)

@yyhhyyyyyy
Copy link
Contributor Author

Thanks @Aries-ckt for fixing the PR, it has been tested and confirmed resolved.
Also, thanks to @csunny for your insights.

@fangyinc fangyinc changed the title Attempt to fix the 'make mymp' error introduced by PR #1359 fix: Fix the mypy check error Apr 7, 2024
@github-actions github-actions bot added the fix Bug fixes label Apr 7, 2024
fangyinc
fangyinc previously approved these changes Apr 7, 2024
Copy link
Collaborator

@fangyinc fangyinc left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@Aries-ckt Aries-ckt left a comment

Choose a reason for hiding this comment

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

r+

Copy link
Collaborator

@fangyinc fangyinc left a comment

Choose a reason for hiding this comment

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

r+

@fangyinc fangyinc merged commit f2a6284 into eosphoros-ai:main Apr 7, 2024
2 checks passed
Hopshine pushed a commit to Hopshine/DB-GPT that referenced this pull request Sep 10, 2024
Co-authored-by: yyhhyy <[email protected]>
Co-authored-by: aries_ckt <[email protected]>
Co-authored-by: Fangyin Cheng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants