Integration of VLM embedding model #446

FUYICC · 2024-02-01T05:45:00Z

Description

Issue #445

Summary by CodeRabbit

New Features
- Introduced CLIPEmbedding for image and text embedding functionalities.
Bug Fixes
- Improved file encoding handling in license updates.
Tests
- Added tests for the new CLIPEmbedding functionality, covering initialization, embedding processes, and output dimension retrieval.

…ndarray

…rEncoder

Wendong-Fan · 2024-03-09T03:09:10Z

Hey @Appointat, thanks for your detailed polish on the docstring! If other parts is also good for you could you approve this PR? Also one quick tip, maybe it's better to use review mode list suggestions rather than push commit directly, it would be better for PR owner to track the issue and learn from your review~

dandansamax

Thanks a lot. leave some suggestions.

camel/embeddings/clip_embedding.py

dandansamax · 2024-03-09T07:43:20Z

camel/embeddings/clip_embedding.py

+
+ def embed_list(
+ self,
+ objs: List[Union[Image.Image, str]], # to do


what does to do mean here?

Sorry, I forgot to move it.

camel/embeddings/clip_embedding.py

licenses/update_license.py

Appointat · 2024-03-09T21:06:27Z

Hey @Appointat, thanks for your detailed polish on the docstring! If other parts is also good for you could you approve this PR? Also one quick tip, maybe it's better to use review mode list suggestions rather than push commit directly, it would be better for PR owner to track the issue and learn from your review~

Hey, thanks for the tip! I appreciate the tip for the code review. It does make sense to maintain clarity and help the PR owner to track changes. I was previously concerned that my comments were too numerous, to the point where code review would waste too much time for both parties, so I committed the changes directly. By the way, my recent commits were focused on formatting and adding comments to improve code readability and maintainability. I've gone through the rest of the changes in the PR, and everything looks good to me. Good job!

Co-authored-by: Tianqi Xu <[email protected]>

dandansamax · 2024-03-11T15:58:57Z

By the way, my recent commits were focused on formatting and adding comments to improve code readability and maintainability.

Yeah. It makes sense not to leave every single detail in comments. The contributor can directly learn from reviewer's commits.

Co-authored-by: Tianqi Xu <[email protected]>

zechengz · 2024-03-14T07:53:01Z

camel/embeddings/clip_embedding.py

+from camel.embeddings import BaseEmbedding
+
+
+class CLIPEmbedding(BaseEmbedding[Union[str, Image.Image]]):


Can we change this to something like VisionLanguageEmbedding? There are some other models also similar to CLIP such as BLIP etc.

zechengz · 2024-03-14T07:54:26Z

camel/embeddings/clip_embedding.py

+ (default: :obj:`openai/clip-vit-base-patch32`)
+ """
+
+ from transformers import CLIPModel, CLIPProcessor


As mentioned above, to make this class more general you can use AutoModel and AutoProcessor here instead of specific ones.

Suggested change

from transformers import CLIPModel, CLIPProcessor

from transformers import AutoModel, AutoProcessor

zechengz · 2024-03-14T07:59:31Z

camel/embeddings/clip_embedding.py

+ from transformers import CLIPModel, CLIPProcessor
+ self.model = CLIPModel.from_pretrained(model_name)
+ self.processor = CLIPProcessor.from_pretrained(model_name)
+ text = 'dimension'


IMO we can make this attribute lazily initialized. For example, during init you can have self.dim = None.
If self.dim is None and someone is calling get_output_dim, you can assign and return self.dim in get_output_dim.
If self.dim is None and someone is calling embed_list, you can assign self.dim within embed_list
Or you can explore using AutoConfig which may have the embedding size, though I am not sure.

zechengz · 2024-03-14T08:01:34Z

test/embeddings/test_clip_embeddings.py

+
+
+def test_CLIPEmbedding_initialization():
+ embedding = CLIPEmbedding()


Can we create some mock tests for the embedding instead of download the model every time (which is quite expensive)?

Yes you are right.

Appointat

I have updated my code reviewing. Please let me know if there are any questions.

Appointat · 2024-03-08T21:15:34Z

camel/embeddings/clip_embedding.py

+
+ def embed_list(
+ self,
+ objs: List[Union[Image.Image, str]], # to do


The commentation # to do, what do you mean? Is the PR not finished, remaining something to be done? Remove it or complete the commentation to make it understandable.

Appointat · 2024-03-08T21:20:58Z

camel/embeddings/clip_embedding.py

+ def embed_list(
+ self,
+ objs: List[Union[Image.Image, str]], # to do
+ **kwargs: Any,


It is noticed that the **kwargs parameter doesn't appear to be utilized within the function's implementation, and it's also not covered by the existing unit tests. Could we enhance our test suite by including tests that verify the handling of **kwargs? This would ensure that all aspects of the function's behavior are thoroughly tested." Thanks.

Appointat · 2024-03-08T21:32:35Z

test/embeddings/test_clip_embeddings.py

+ assert isinstance(embeddings, list)
+ assert len(embeddings) == 2
+ for e in embeddings:
+ assert len(e) == embedding.get_output_dim()


Regarding the tow funcs test_image_embed_list_with_valid_input and test_text_embed_list_with_valid_input, to ensure robustness, could we consider introducing a combined test case, such as test_image_and_text_embed_list_with_valid_input, whose input can be test_image_text = [image, "Hello world"]? If not, I suggest adding a small check to validate that the function correctly raises an error message, The type of the input is inconsistent., when encountering mixed input types? Thank you for addressing this.

Thank you for bringing this up, at the moment we do have a design that can only accept the same type of input, I will add an error reporting tip.

Appointat · 2024-03-08T21:37:43Z

camel/embeddings/clip_embedding.py

+ as a list of floating-point numbers.
+ """
+ if not objs:
+ raise ValueError("Input text list is empty.")


The error msg should be "Input objs list is empty."

Appointat · 2024-03-22T22:27:13Z

@FUYICC Hi, is the pr still in progress? Let me know if you have any difficulties.

FUYICC · 2024-03-27T06:48:21Z

@FUYICC Hi, is the pr still in progress? Let me know if you have any difficulties.

Thank you for your kind help! Sorry I've been mostly working on my dissertation for the past 3 weeks so I haven't had time to move forward, I'll be up and running starting next week, we'll discuss any questions anytime!

…tion

camel/embeddings/vlm_embedding.py

Wendong-Fan and others added 18 commits November 24, 2023 01:01

add e5 embedding

d300035

fix typo in toml file

524bfd4

allow user to switch embeeding model from SentenceTransformer

0f13021

Move the import to __init__

9ddc871

polish docstring

e9c3135

remove # type: ignore

aeae92d

change embed_list return type and polish docstring

b431e67

use Union[List[List[float]], ndarray] instead of List[List[float]] | …

884f190

…ndarray

change return of embed_list from ndarray to list

9cce263

change name from SentenceTransformerEmbedding into SentenceTransforme…

4c7b67c

…rEncoder

update poetry

939808e

update poetry

e8ce692

update poetry

1bf7320

update poetry

653b381

remove ndarry and union in embedding base file

93e795e

Merge branch 'master' into feature/open_source_embedding_model

a50b478

sentence-transformer

4d5ba2d

integration of clip embedding and update of license

692a670

FUYICC self-assigned this Feb 1, 2024

FUYICC linked an issue Feb 1, 2024 that may be closed by this pull request

[Feature Request] Multi-modal RAG(Retrieval-Augmented Generation) #445

Open

4 tasks

FUYICC added the Embeddings label Feb 3, 2024

Limit embed_list input type

20654fd

FUYICC closed this Feb 4, 2024

revert changes of sentence embedding

9e0de62

FUYICC reopened this Feb 5, 2024

FUYICC added 2 commits February 5, 2024 14:18

poetry change of pillow

b3ea26c

change of docstring of functions

f1adf18

FUYICC requested review from lightaime and Wendong-Fan February 9, 2024 03:05

FUYICC marked this pull request as ready for review February 9, 2024 03:11

dandansamax requested changes Mar 9, 2024

View reviewed changes

FUYICC and others added 2 commits March 11, 2024 22:03

Use generics to support the type system

8fe17cb

Co-authored-by: Tianqi Xu <[email protected]>

store dimension into a variable

1afd27b

Update update_license.py for windows compatibility

e8d073d

Co-authored-by: Tianqi Xu <[email protected]>

zechengz requested changes Mar 14, 2024

View reviewed changes

Appointat requested changes Mar 15, 2024

View reviewed changes

FUYICC and others added 5 commits April 9, 2024 23:58

Change to general visual language model class and use lazy initializa…

f0a1573

…tion

Merge branch 'master' into CLIP_model

0fc220d

test for inconsistancy of inputs with different types

1fa0c0f

update of poetry

71d48a2

usage of **kwargs

4de4fad

FUYICC changed the title ~~Integration of CLIP embedding model~~ Integration of VLM embedding model Apr 15, 2024

FUYICC and others added 5 commits May 2, 2024 15:28

debug for pytest

1517d52

Merge branch 'master' into CLIP_model

2105510

poetry dependency

ed54edf

ruff

a667614

poetry

8aab43d

FUYICC requested review from Appointat, zechengz and dandansamax May 3, 2024 05:58

Wendong-Fan reviewed May 5, 2024

View reviewed changes

camel/embeddings/vlm_embedding.py Show resolved Hide resolved

Wendong-Fan requested changes May 5, 2024

View reviewed changes

camel/embeddings/vlm_embedding.py Show resolved Hide resolved

FUYICC added 2 commits May 5, 2024 21:32

return list of float

8c1f086

change of tests

b8bd94e

FUYICC requested a review from Wendong-Fan May 5, 2024 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration of VLM embedding model #446

Integration of VLM embedding model #446

FUYICC commented Feb 1, 2024 •

edited by coderabbitai bot

Wendong-Fan commented Mar 9, 2024

dandansamax left a comment

dandansamax Mar 9, 2024

FUYICC Mar 11, 2024

Appointat commented Mar 9, 2024 •

edited

dandansamax commented Mar 11, 2024

zechengz Mar 14, 2024

zechengz Mar 14, 2024

zechengz Mar 14, 2024

zechengz Mar 14, 2024

FUYICC Apr 9, 2024

Appointat left a comment

Appointat Mar 8, 2024

Appointat Mar 8, 2024

Appointat Mar 8, 2024

FUYICC Apr 9, 2024

Appointat Mar 8, 2024

Appointat commented Mar 22, 2024

FUYICC commented Mar 27, 2024

		from camel.embeddings import BaseEmbedding


		class CLIPEmbedding(BaseEmbedding[Union[str, Image.Image]]):

	from transformers import CLIPModel, CLIPProcessor
	from transformers import AutoModel, AutoProcessor



		def test_CLIPEmbedding_initialization():
		embedding = CLIPEmbedding()

Integration of VLM embedding model #446

Are you sure you want to change the base?

Integration of VLM embedding model #446

Conversation

FUYICC commented Feb 1, 2024 • edited by coderabbitai bot

Description

Summary by CodeRabbit

Wendong-Fan commented Mar 9, 2024

dandansamax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Appointat commented Mar 9, 2024 • edited

dandansamax commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Appointat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Appointat commented Mar 22, 2024

FUYICC commented Mar 27, 2024

FUYICC commented Feb 1, 2024 •

edited by coderabbitai bot

Appointat commented Mar 9, 2024 •

edited