-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
refactor: HF API Embedders - use InferenceClient.feature_extraction
instead of InferenceClient.post
#8794
Conversation
Pull Request Test Coverage Report for Build 13116238781Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
||
np_embeddings = self._client.feature_extraction( | ||
# this method does not officially support list of strings, but works as expected | ||
text=batch, # type: ignore[arg-type] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as explained in the comment, the method does what we need but this usage is not officially supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding, I looked into the API. From this discussion, am I correct to deduce that both types str
and List[str]
are supported for text
. We are unsure because the docs don't mention List[str]
officially but the underlying models do expect lists and return correct results.
In that case, it would make sense to introduce this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are basically right.
I reached out to the huggingface_hub
maintainers here: huggingface/huggingface_hub#2824
You can read this message to get a better understanding.
# this method does not officially support list of strings, but works as expected | ||
text=batch, # type: ignore[arg-type] | ||
# Serverless Inference API does not support truncate and normalize, so we pass None in the request | ||
truncate=self.truncate if self.api_type != HFEmbeddingAPIType.SERVERLESS_INFERENCE_API else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
truncate
and normalize
are not supported in the Serverless Inference API.
With post
, these parameters are ignored if the server does not support them.
Using feature_extraction
, we need to pass None
, otherwise we get an error.
@@ -87,7 +87,7 @@ extra-dependencies = [ | |||
"numba>=0.54.0", # This pin helps uv resolve the dependency tree. See https://github.com/astral-sh/uv/issues/7881 | |||
|
|||
"transformers[torch,sentencepiece]==4.47.1", # ExtractiveReader, TransformersSimilarityRanker, LocalWhisperTranscriber, HFGenerators... | |||
"huggingface_hub>=0.27.0, <0.28.0", # Hugging Face API Generators and Embedders | |||
"huggingface_hub>=0.27.0", # Hugging Face API Generators and Embedders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on my local tests, this solution works both with the new and the old version, so we can safely remove the pin
InferenceClient.feature_extraction
instead of InferenceClient.post
# Serverless Inference API does not support truncate and normalize, so we pass None in the request | ||
truncate=self.truncate if self.api_type != HFEmbeddingAPIType.SERVERLESS_INFERENCE_API else None, | ||
normalize=self.normalize if self.api_type != HFEmbeddingAPIType.SERVERLESS_INFERENCE_API else None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we raise a warning that these params will be ignored, if the user explictly passes truncate
or normalize
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in f9479e0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG! Its your call if you want to update a test to check if the warning is raised properly. Otherwise feel free to merge.
Thanks! Added a small assertion to related tests... |
Related Issues
HuggingFaceAPIDocumentEmbedder
is not compatible withhuggingface_hub>=0.28.0
#8791Our HF API Embedders support 3 different HF APIs using the
huggingface_hub
client: Serverless Inference API, Text Embeddings Inference (deployed locally or elsewhere), and HF Paid Inference Endpoints.For several reasons, in the past we have opted to use
InferenceClient.post
method, but this is fragile, subject to changes (as has happened recently) and will soon be removed (in 0.31).This is why I'm migrating to the
InferenceClient.feature_extraction
method. This solution works but may not be ideal, as explained in the comments.Proposed Changes:
feature_extraction
instead ofpost
.How did you test it?
CI; new tests
In the CI, we are only testing the Serverless Inference API.
For these reasons, I also tested the current solution with:
Notes for the reviewer
Since the solution identified is not ideal, I also plan to contact a
huggingface_hub
maintainer and ask for advice.Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
and added!
in case the PR includes breaking changes.