-
Notifications
You must be signed in to change notification settings - Fork 178
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
vertexai: Add embeddings_task_type
parameter to embed_query
and embed_documents
#716
Changes from all commits
f3c1809
398aa03
ddeb681
30762b8
366c5f4
e9fa377
9caf9ab
3b4b7d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -53,6 +53,25 @@ def test_langchain_google_vertexai_embedding_documents( | |||
assert model.model_name == model_name | ||||
|
||||
|
||||
@pytest.mark.release | ||||
@pytest.mark.parametrize( | ||||
"model_name, embeddings_dim", | ||||
_EMBEDDING_MODELS, | ||||
) | ||||
def test_langchain_google_vertexai_embedding_documents_with_task_type( | ||||
model_name: str, | ||||
embeddings_dim: int, | ||||
) -> None: | ||||
documents = ["foo bar"] * 8 | ||||
model = VertexAIEmbeddings(model_name) | ||||
output = model.embed_documents(documents) | ||||
assert len(output) == 8 | ||||
for embedding in output: | ||||
assert len(embedding) == embeddings_dim | ||||
assert model.model_name == model.client._model_id | ||||
assert model.model_name == model_name | ||||
|
||||
|
||||
@pytest.mark.release | ||||
@pytest.mark.parametrize( | ||||
"model_name, embeddings_dim", | ||||
|
@@ -65,6 +84,21 @@ def test_langchain_google_vertexai_embedding_query(model_name, embeddings_dim) - | |||
assert len(output) == embeddings_dim | ||||
|
||||
|
||||
@pytest.mark.release | ||||
@pytest.mark.parametrize( | ||||
"model_name, embeddings_dim", | ||||
_EMBEDDING_MODELS, | ||||
) | ||||
def test_langchain_google_vertexai_embedding_query_with_task_type( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this integration test, or would a unit test be enough (we've tested in a test above that embeddings_task is passed to the Google API and it returns a valid output) we've got too many integration tests now and the execution time gets longer and longer :), it might be a good idea to keep the total amount of them reasonable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally makes sense! However, I'm not sure how we would write a clean unit test for the new argument in If we only want to check if the arguments are passed correctly, one approach could be to mock the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just by mocking the API:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved the tests into unit tests. |
||||
model_name: str, | ||||
embeddings_dim: int, | ||||
) -> None: | ||||
document = "foo bar" | ||||
model = VertexAIEmbeddings(model_name) | ||||
output = model.embed_query(document) | ||||
assert len(output) == embeddings_dim | ||||
|
||||
|
||||
@pytest.mark.release | ||||
@pytest.mark.parametrize( | ||||
"dim, expected_dim", | ||||
|
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.
please, add a
*,
so thatembeddings_task_type
can be provided by name onlyThere 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.
fixed