added support for clip to natural language search #965
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @henry-encord, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces support for specifying the embedding model within the metadata schema, specifically adding the CLIP model. This enhancement allows for more precise definition of embedding types, which is crucial for features like natural language search that rely on specific embedding models.
Highlights
- Support for Embedding Models: Introduced a new StrEnum _ClientMetadataSchemaTypeEmbeddingModel to define supported embedding models, starting with CLIP.
- Extended Embedding Schema: The _ClientMetadataSchemaTypeEmbedding now includes an optional model field to associate embeddings with a specific model.
- Updated add_embedding Method: The add_embedding method in MetadataSchema has been updated to allow specifying the embedding model when adding new embedding types to the schema.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to add support for CLIP models in natural language search by extending the metadata schema for embeddings. The changes introduce a new model parameter for embeddings. However, I've found a critical issue in the implementation that would cause a runtime error, and I've also noticed that the new functionality lacks test coverage. My review includes a fix for the bug and a recommendation to add tests to ensure the feature is robust.
encord/metadata_schema.py
Outdated
| raise MetadataSchemaError(f"{k} is already defined") | ||
| _assert_valid_metadata_key(k) | ||
| self._schema[k] = _ClientMetadataSchemaOption(root=_ClientMetadataSchemaTypeEmbedding(size=size)) | ||
| self._schema[k] = _ClientMetadataSchemaOption(root=_ClientMetadataSchemaTypeEmbeddingModel(size=size, model=encord_supported_model if encord_supported_model else None)) |
There was a problem hiding this comment.
There's a typo here. You're instantiating _ClientMetadataSchemaTypeEmbeddingModel, which is a StrEnum and cannot be initialized with size and model parameters. You should be using _ClientMetadataSchemaTypeEmbedding, which is the correct Pydantic model for this purpose.
Also, the expression encord_supported_model if encord_supported_model else None is redundant since the encord_supported_model parameter is already either a model instance or None.
| self._schema[k] = _ClientMetadataSchemaOption(root=_ClientMetadataSchemaTypeEmbeddingModel(size=size, model=encord_supported_model if encord_supported_model else None)) | |
| self._schema[k] = _ClientMetadataSchemaOption(root=_ClientMetadataSchemaTypeEmbedding(size=size, model=encord_supported_model)) |
encord/metadata_schema.py
Outdated
| self._dirty = False | ||
|
|
||
| def add_embedding(self, k: str, *, size: int) -> None: | ||
| def add_embedding(self, k: str, *, size: int, encord_supported_model: _ClientMetadataSchemaTypeEmbeddingModel | None = None) -> None: |
There was a problem hiding this comment.
While adding the encord_supported_model parameter is a good enhancement, the changes are not covered by tests. Please add unit tests to tests/test_metadata_schema.py to verify the new functionality, including cases where the model is provided and when it's not. This will help ensure correctness and prevent future regressions.
Consider also adding a public method like get_embedding_model(k: str) to retrieve the model associated with an embedding key, similar to get_embedding_size.
There was a problem hiding this comment.
change type or at least ensure the type isn't a _ starting arg
encord/metadata_schema.py
Outdated
|
|
||
|
|
||
| class _ClientMetadataSchemaTypeEmbeddingModel(StrEnum): | ||
| CLIP = "$ENCORD$/DEFAULT" |
There was a problem hiding this comment.
bad copy - this should be CLIP = "CLIP"
henry-encord
force-pushed
the
henry/mgc-373/support-nls-for-compatible-models-in-sdk
branch
from
722ffbe to
295531d
Compare
No description provided.