add embeddings space component #16
Conversation
There was a problem hiding this comment.
Great work! I left a couple of comments:
- Let's make generation methods private and remove them from the documentation
- Let's discuss a bit about error handling and logging. It could make sense to create a small error handler class that help us homogenize how we handle them project-wide. We should avoid masking unknown errors and log messages in the correct log level.
|
|
||
| // Or just | ||
| Embedding embedding = embeddingSpace.save(sampleText); | ||
| Embedding embedding = embeddingsSpace.save(sampleText); |
| public Embedding generate(String text) { | ||
| Embedding embedding = null; | ||
| try { | ||
| embedding = model.generateEmbedding(text); | ||
| } catch (RuntimeException e) { | ||
| // TODO: Create and catch EmbeddingGenerationException? | ||
| System.out.printf("Error while generating embedding tor text %s", text); | ||
| } | ||
| return embedding; | ||
| } | ||
|
|
||
| public Embedding generate(String text, Map<String, String> additionalMetadata) { | ||
| // TODO: What about metadata? | ||
| return model.generateEmbedding(text); | ||
| } |
There was a problem hiding this comment.
I think that we no longer need to make generation methods public. Without the vector arithmetic, there's no reason to expose embedding objects to the user through this interface anymore, because even search calculates the embedding for the reference text internally...
| try { | ||
| embedding = model.generateEmbedding(text); | ||
| } catch (RuntimeException e) { | ||
| // TODO: Create and catch EmbeddingGenerationException? | ||
| System.out.printf("Error while generating embedding tor text %s", text); | ||
| } |
There was a problem hiding this comment.
I'm not sure if capturing exceptions here makes sense because capturing any runtime exception can mask errors and make troubleshooting more challenging. If we want to log the error, that's a good idea, but then we should re-raise the original exception or generate a new one, wrapping the original one. Also, if Java supports log levels, these logs must probably be debug level to avoid stuffing production logs.
There was a problem hiding this comment.
I'll remove the try catchs then, we can detect if this is needed in the future :P
| // TODO: Create and catch EmbeddingStoreException? | ||
| System.out.printf("Error while storing embedding %s into the embedding space%n", embedding.toString()); | ||
| return null; |
There was a problem hiding this comment.
I've created a MissingRequiredCredentialException in https://github.com/theam/eLLMental/pull/15/files#diff-76b05f415c5a32450bc4c8092ca7a23ac0c019751b652a40173c696823a6bda4R4-R8
Not sure if it makes sense to capture Pinecone exceptions or is better to let them go through though
| private Embedding generate(String text) { | ||
| return embeddingsGenerationModel.generateEmbedding(text); | ||
| } |
There was a problem hiding this comment.
I like private methods at the bottom of the class because when we read a class we tend to look for public methods first, but no need to change this now.
|
|
||
| return mostSimilarEmbeddings(embedding, limit); | ||
| } | ||
|
|
There was a problem hiding this comment.
The extra whitespace triggers OCD feelings 😅. Maybe we can activate a default linter to take all these little details into account 🤔
|
Test will be done here -> #20. So I'm merging this 🎉 |
No description provided.