-
Notifications
You must be signed in to change notification settings - Fork 970
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
Adding support for VertexAi matching engine. #289
Conversation
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.
Hi @pascalconfluent thanks a lot for your contribution!
I have left some comments, please check them, thanks!
langchain4j-vertex-ai/src/main/java/dev/langchain4j/model/vertexai/VertexAiMatchingEngine.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
protected double getCosineSimilarity(Embedding embedding, Embedding referenceEmbedding) { | ||
return CosineSimilarity.between(embedding, referenceEmbedding); |
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.
It would be better to keep the behavior of all embedding stores the same. Score should be a number from 0 to 1.
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.
In VertexAI is depends on the algorithm you are using when creating the index. The one use default in the unit tests is not recommended by Google. But happy to revert back and put a note in the unit tests.
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.
which algorithms are supported and wich is the default one? can't find it in the code...
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.
It is defined during index creation. Here is different algorithm that can be configured:
SQUARED_L2_DISTANCE
Euclidean (L2) Distance
L1_DISTANCE
Manhattan (L1) Distance
DOT_PRODUCT_DISTANCE
Default value. Defined as a negative of the dot product.
COSINE_DISTANCE
Cosine Distance. We strongly suggest using DOT_PRODUCT_DISTANCE UNIT_L2_NORM instead of the COSINE distance. Our algorithms have been more optimized for the DOT_PRODUCT distance, and when combined with UNIT_L2_NORM, it offers the same ranking and mathematical equivalence as the COSINE distance.
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 am using the default one for testing!
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.
Hmm... is index created automatically by your implementation or user manually creates it? I see that you create VertexAiEmbeddingIndex
each time addAll()
is called, but I can't find the code responsible for defining algorithm in your implementation...
But my initial point was that instead of using CosineSimilarity.between(embedding, referenceEmbedding)
, please use RelevanceScore.fromCosineSimilarity(CosineSimilarity.between(embedding, referenceEmbedding))
. This way it will be in line with all other integrations. And it should work the same for both cosine similarity and dot product L2 normalization.
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.
Each List has an index file attached to it. This index file is used by the matching engine to index it. If you create a batch mode index, then matching engine scan all the files to add the embeddings to the index. In short, an index is created in 2 phases, first create an index in matching engine (that is where you define the algorithm, ....), then you need to create files that represent a list of embeddings. I implemented the 2 phase, will probably do the first one but this one can be done manually using the UI.
For the relevance, yes I can do that and assume that the index is created with the right algorithm.
By the way I looked the implementation from the python's langchain and they are doing the same.
...chain4j-vertex-ai/src/test/java/dev/langchain4j/model/vertexai/VertexAiMatchingEngineIT.java
Outdated
Show resolved
Hide resolved
langchain4j-vertex-ai/src/main/java/dev/langchain4j/model/vertexai/VertexAiMatchingEngine.java
Outdated
Show resolved
Hide resolved
final IndexDatapoint resultDatapoint = neighbor.getDatapoint(); | ||
return new EmbeddingMatch<>(neighbor.getDistance(), | ||
id, | ||
Embedding.from(resultDatapoint.getFeatureVectorList()), |
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.
since you made returnFullDatapoint configurable, the check is needed here that the embedding is returned
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.
It return an empty array. I think we should also return an empty embedding.
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.
An empty Embedding
makes no sense, if we don't have an embedding, it is better to return null
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.
That's debatable. Same goes with strings and other empty/null entities.
Anyway, happy to return null instead of empty if no result.
...chain4j-vertex-ai/src/main/java/dev/langchain4j/model/vertexai/internal/MatchingService.java
Outdated
Show resolved
Hide resolved
...4j-vertex-ai/src/main/java/dev/langchain4j/model/vertexai/internal/IndexEndpointService.java
Outdated
Show resolved
Hide resolved
...hain4j-vertex-ai/src/main/java/dev/langchain4j/model/vertexai/internal/VertexAIDocument.java
Outdated
Show resolved
Hide resolved
BTW is this a recommended way to store and search embeddings using Vertex AI? Somehow feels overly complicated and in some places inefficient. |
...in4j-core/src/test/java/dev/langchain4j/store/embedding/EmbeddingStoreWithoutMetadataIT.java
Outdated
Show resolved
Hide resolved
...j-vertex-ai/src/main/java/dev/langchain4j/store/embedding/vertexai/VertexAiVectorSearch.java
Outdated
Show resolved
Hide resolved
...j-vertex-ai/src/main/java/dev/langchain4j/store/embedding/vertexai/VertexAiVectorSearch.java
Outdated
Show resolved
Hide resolved
log.info("Uploading {} index to GCS.", filename); | ||
|
||
// Upload the index to GCS | ||
getGcpBlobService().upload(filename, index.toString()); |
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.
LangChain implementation seem to upload embeddings to GCS bucket first and then update the index, providing a reference to the bucket (where to read embeddings from). But in your implementation you provide embeddings directly in the index, so it feels that uploading them to GCS is unnecessary. Could you please remove this line and test if it still works? Thanks!
...ai/src/main/java/dev/langchain4j/store/embedding/vertexai/internal/IndexEndpointService.java
Show resolved
Hide resolved
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.
Hi @pascalconfluent, could you please check my comments? Thanks!
I assume this has gone pretty stale at this point :) |
AFAIK @glaforge wanted to work on adding Vertex AI Search soon, so maybe this PR can be revived? |
Would be interesting to know if @pascalconfluent wants to update the PR? |
Yes. I want to finish this PR.
… On Jun 17, 2024, at 11:19, Guillaume Laforge ***@***.***> wrote:
Would be interesting to know if @pascalconfluent <https://github.com/pascalconfluent> wants to update the PR?
We'll have to add support for metadata filtering as well, I guess.
But that would be great to see Vertex AI Search support (used to be called Matching Engine)
—
Reply to this email directly, view it on GitHub <#289 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKB2NQBZJDDLSXX5ND6XQUDZH35BDAVCNFSM6AAAAAA7PDIZFGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZTGY4TKMBWGU>.
You are receiving this because you were mentioned.
|
@pascalconfluent closing this PR due to inactivity. Please feel free to reopen if/when needed. |
No description provided.