Align the Embedding api with the new meta-model #270
Conversation
tzolov
commented
Jan 25, 2024
tzolov
commented
- Craete new EmbeddingOptions -> ModelOptions, EmbeddigRequest -> ModelRequest, EmbeddingResponseMetadata -> ResponseMetadata and EmbeddignResultMetadata -> ResultMetadata.
- Make the EmbeddigClient interface extend from ModelClient<EmbeddingRequest, EmbeddingResponse>, EmbeddingResponse implements ModelResponise and Embedding implements ModelResult.
- Fix affected tests.
- Streamline the EmbeddingClient interface with default method implementations based on call.
- Craete new EmbeddingOptions -> ModelOptions, EmbeddigRequest -> ModelRequest, EmbeddingResponseMetadata -> ResponseMetadata and EmbeddignResultMetadata -> ResultMetadata. - Make the EmbeddigClient interface extend from ModelClient<EmbeddingRequest, EmbeddingResponse>, EmbeddingResponse implements ModelResponise and Embedding implements ModelResult. - Fix affected tests. - Steramline the EmbeddingClient interface with default method implementations based on call.
| /** | ||
| * @author Christian Tzolov | ||
| */ | ||
| public class EmbeddingResponseMetadata extends HashMap<String, Object> implements ResponseMetadata { |
There was a problem hiding this comment.
maybe composition instead? more for future proofing. put this list together of responses from the different providers.
We haven't yet address 'portable return options', the mirror of 'portable request options', so this maybe just info for future considerations.
OpenAI
metadata.put("model", model);
metadata.put("prompt-tokens", usage.promptTokens());
metadata.put("completion-tokens", usage.completionTokens());
metadata.put("total-tokens", usage.totalTokens());
Azure OpenAI
metadata.put("model", model);
metadata.put("prompt-tokens", embeddingsUsage.getPromptTokens());
metadata.put("total-tokens", embeddingsUsage.getTotalTokens());
PostgresML
Map.of(
"transformer", this.transformer,
"vector-type", this.vectorType.name(),
"kwargs", this.kwargs));
Vertex
{
"predictions": [
{
"embeddings": {
"statistics": {
"truncated": false,
"token_count": 6
},
"values": [ ... ]
}
}
]
}
| super(initialCapacity, loadFactor); | ||
| } | ||
|
|
||
| public EmbeddingResponseMetadata(Map<? extends String, ?> m) { |
There was a problem hiding this comment.
Sonar complained about this....
However, the type parameter for the Map accepts any subtype of String which doesn't quite make sense since String is a final class and cannot have any subclasses. Hence, the ? extends keyword is unnecessary in this case and code would be cleaner and simpler as Map<String, ?>. The constructors of EmbeddingResponseMetadata could be like this:
public EmbeddingResponseMetadata(Map<String, ?> m) {
super(m);
}
| assertThat(embeddingResponse.getData().get(0).getIndex()).isEqualTo(0); | ||
| assertThat(embeddingResponse.getData().get(1).getEmbedding()).isNotEmpty(); | ||
| assertThat(embeddingResponse.getData().get(1).getIndex()).isEqualTo(1); | ||
| assertThat(embeddingResponse.getResults()).hasSize(2); |
There was a problem hiding this comment.
unrelated to the change, EmbeddingUtil should prob be abstract class to avoid instantiation.
There was a problem hiding this comment.
Ok, the EmbeddingUtil is merged within the AbstractEmbeddingClient now
|
merged in 5d55b68 |
2 participants
