Refactor HF loader and add poolingMethod #954

wanliAlex · 2024-09-05T05:56:17Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature
What is the current behavior? (You can also link to an open issue here)

The HF loader class is outdated.
We can't specify the pooling method
Some document links are outdated

What is the new behavior (if this is a feature change)?

We refactor the HF class code
Add a new field "poolingMethod" to the model properties of the HF loader so users can specify the poolingMethod
Fix some documents links

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

no

Have unit tests been run against this PR? (Has there also been any additional testing?)

running

Related Python client changes (link commit/PR here)
Related documentation changes (link commit/PR here)
Other information:
Please check if the PR fulfills these requirements

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added / updated (for bug fixes / features)

…into li/add-pooling-hf

vicilliar · 2024-10-22T07:36:47Z

src/marqo/core/inference/inference_models/abstract_clip_model.py

-from marqo.s2_inference.types import *
-from marqo.core.inference.image_download import (_is_image, format_and_load_CLIP_images,
- format_and_load_CLIP_image)
+from marqo.core.inference.inference_models.abstract_embedding_model import AbstractEmbeddingModel


I think we should be consistent when renaming classes/directories. I notice there's a new directory called core/inference/inference_models. Maybe it should be core/inference/embedding_models to keep consistency if we're referring to the same objects.

This goes for any other reference to inference models vs. embeddings models

vicilliar · 2024-10-22T07:52:27Z

src/marqo/core/inference/inference_models/hugging_face_model_properties.py

+
+
+class PoolingMethod(str, Enum):
+ Mean = "mean"


Should we add max and attention pooling as options as well? This can be a future feature.

vicilliar · 2024-10-22T08:02:13Z

src/marqo/core/inference/inference_models/hugging_face_model_properties.py

+ return PoolingMethod.Mean
+
+ if not isinstance(content, dict):
+ logger.warn(f"Could not infer pooling method from the model {name}. Defaulting to mean pooling.")


The code snippet:

logger.warn(f"Could not infer pooling method from the model {name}. Defaulting to mean pooling.") return PoolingMethod.Mean

Is repeated a lot. It could be split into a function, or put at the bottom of this function and triggered with a boolean flag

vicilliar · 2024-10-22T08:09:18Z

src/marqo/core/inference/inference_models/hugging_face_model_properties.py

+ CLS = "cls"
+
+
+class HuggingFaceModelProperties(MarqoBaseModel):


It's probably worth making a ModelProperties class and having this class subclass from it. We may need an OpenClipModelProperties in the future.

papa99do · 2024-10-22T23:23:44Z

src/marqo/core/inference/inference_models/hugging_face_model.py

+ if not (self.model_properties.name or self.model_properties.url or self.model_properties.model_location):
+ raise InvalidModelPropertiesError(
+ f"Invalid model properties for the 'hf' model. "
+ f"You do not have the necessary information to load the model. "
+ f"Check {marqo_docs.bring_your_own_model()} for more information."
+ )


nit. This logic is covered in the next section, can be removed. You also have a validator in the HuggingFaceModelProperties class to ensure this.

papa99do · 2024-10-23T00:04:16Z

src/marqo/core/inference/inference_models/abstract_embedding_model.py

 self._load_necessary_components()
 self._check_loaded_components()


Why do we need to separate these two method?

papa99do · 2024-10-23T00:06:34Z

src/marqo/core/inference/inference_models/hugging_face_model.py

+ sentence = [sentence]
+
+ if self._model is None:
+ self.load()


Do we need concurrency control here?

papa99do · 2024-10-23T00:09:02Z

src/marqo/core/inference/inference_models/hugging_face_model.py

+ @staticmethod
+ def extract_huggingface_archive(path: str) -> str:
+ '''
+ This function takes the path as input. The path can must be a string that can be:


nit. The path can must be a string that can be -> The path is a string that can be

papa99do · 2024-10-23T00:16:45Z

src/marqo/core/inference/inference_models/hugging_face_model.py

+ with tarfile.open(path, 'r') as tar_ref:
+ tar_ref.extractall(new_dir)
+ # return the path to the new directory
+ return new_dir


Do we need to keep the extracted files after the model is loaded?

papa99do · 2024-10-23T00:18:51Z

src/marqo/core/inference/inference_models/hugging_face_model.py

+ @staticmethod
+ def _average_pool_func(model_output, attention_mask):
+ """A pooling function that averages the hidden states of the model."""
+ last_hidden = model_output.last_hidden_state.masked_fill(~attention_mask[..., None].bool(), 0.0)
+ return last_hidden.sum(dim=1) / attention_mask.sum(dim=1)[..., None]
+
+ @staticmethod
+ def _cls_pool_func(model_output, attention_mask=None):
+ """A pooling function that extracts the CLS token from the model."""
+ return model_output[0][:, 0]


Are these pooling methods common across models? Will we support more pooling method in the future? If so, consider extract them out to a class hierarchy? (does not need to change now)

papa99do · 2024-10-23T00:27:18Z

src/marqo/core/inference/inference_models/hugging_face_model_properties.py

+ try:
+ file_path = hf_hub_download(repo_id, file_name, cache_dir=ModelCache.hf_cache_path)
+ except HfHubHTTPError:
+ logger.warn(f"Could not infer pooling method from the model {name}. Defaulting to mean pooling.")


nit. logger.warning. (logger.warn is deprecated)

papa99do · 2024-10-23T00:59:08Z

tests/core/inference/test_hugging_face_model.py

+class TestHuggingFaceModel(unittest.TestCase):
+ """Test initializing the HuggingFaceModel with valid properties."""
+
+ E5_BASE_V2_MODEL_EMBEDDINGS = np.squeeze(


nit. consider having these fixed embeddings in a separate json file, it's easier to read and maintain.

wanliAlex and others added 28 commits August 16, 2024 15:20

Finish initial commit

550c1f0

Finish tests

0685691

Upgrade requirements.txt

a8c9bf5

Upgrade requirements.txt

0d40b58

Remove max sequence length

a2bef12

Remove outdated open_clip tests

c3fd6f5

Fix unit tests error message

0cb12e9

Add mobile clipmodel

aaa9878

Merge branch 'mainline' into li/update-oc

8c3ec59

Resolve farshid's comments

1d3fa81

Merge branch 'mainline' into li/update-oc

8995d54

Fix tests

2e8e616

Update version to 2.12.0

e51405e

Change base version to 29

4f8f486

Fix exmaples

9e7116d

Fix tests

9a6089f

Fix tests

9a5922c

Add some new multilingual clip models

c21775f

Add subtests for large clip models

39c1557

Update file name

a124c0c

Finish HF class

dd61ac7

Add max_seq_length back

badcd44

Merge branch 'li/update-oc' into li/add-pooling-hf

3c6be4d

Update open clip code

0123c82

update open clip class

4e2b040

Finish open_clip refactoring

4d4ea24

Merge branch 'li/update-oc' into li/add-pooling-hf

4415f13

Finish the implementation. Need tests

945c589

wanliAlex had a problem deploying to marqo-test-suite September 5, 2024 05:58 — with GitHub Actions Failure

Catch mainline

c10e5fa

Merge branch 'mainline' into li/add-pooling-hf

8be1948

farshidz had a problem deploying to marqo-test-suite October 22, 2024 05:50 — with GitHub Actions Error

farshidz had a problem deploying to marqo-test-suite October 22, 2024 05:51 — with GitHub Actions Error

farshidz previously approved these changes Oct 22, 2024

View reviewed changes

wanliAlex requested a review from papa99do October 22, 2024 06:11

wanliAlex added 2 commits October 22, 2024 17:11

Fix Yihan's comments

0ed6606

Merge branch 'li/add-pooling-hf' of https://github.com/marqo-ai/marqo …

0e4c25d

…into li/add-pooling-hf

wanliAlex dismissed farshidz’s stale review via 0e4c25d October 22, 2024 06:13

wanliAlex temporarily deployed to marqo-test-suite October 22, 2024 06:15 — with GitHub Actions Inactive

wanliAlex had a problem deploying to marqo-test-suite October 22, 2024 06:15 — with GitHub Actions Failure

wanliAlex temporarily deployed to marqo-test-suite October 22, 2024 06:16 — with GitHub Actions Inactive

wanliAlex had a problem deploying to marqo-test-suite October 22, 2024 07:21 — with GitHub Actions Error

wanliAlex added 2 commits October 22, 2024 18:59

Merge remote-tracking branch 'origin/mainline' into li/add-pooling-hf

5ed9a21

Catch mainline

81c2c4a

wanliAlex temporarily deployed to marqo-test-suite October 22, 2024 08:05 — with GitHub Actions Inactive

wanliAlex had a problem deploying to marqo-test-suite October 22, 2024 08:05 — with GitHub Actions Failure

vicilliar requested changes Oct 22, 2024

View reviewed changes

papa99do reviewed Oct 22, 2024

View reviewed changes

papa99do reviewed Oct 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor HF loader and add poolingMethod #954

Refactor HF loader and add poolingMethod #954

wanliAlex commented Sep 5, 2024 •

edited

Loading

vicilliar Oct 22, 2024

vicilliar Oct 22, 2024

vicilliar Oct 22, 2024

vicilliar Oct 22, 2024

vicilliar Oct 22, 2024

papa99do Oct 22, 2024 •

edited

Loading

papa99do Oct 23, 2024

papa99do Oct 23, 2024

papa99do Oct 23, 2024

papa99do Oct 23, 2024

papa99do Oct 23, 2024 •

edited

Loading

papa99do Oct 23, 2024

papa99do Oct 23, 2024

		CLS = "cls"


		class HuggingFaceModelProperties(MarqoBaseModel):

		self._load_necessary_components()
		self._check_loaded_components()

Refactor HF loader and add poolingMethod #954

Are you sure you want to change the base?

Refactor HF loader and add poolingMethod #954

Conversation

wanliAlex commented Sep 5, 2024 • edited Loading

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

papa99do Oct 22, 2024 • edited Loading

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

papa99do Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanliAlex commented Sep 5, 2024 •

edited

Loading

papa99do Oct 22, 2024 •

edited

Loading

papa99do Oct 23, 2024 •

edited

Loading