-
Notifications
You must be signed in to change notification settings - Fork 290
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
[OPIK-611] support gemini models in playground #987
Conversation
apps/opik-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderFactoryTest.java
Outdated
Show resolved
Hide resolved
26842dd
to
7a42987
Compare
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.
Many comments, but mostly minor, no blockers. You can send a follow up PR.
apps/opik-backend/src/main/java/com/comet/opik/api/LlmProvider.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/ChunkedResponseHandler.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/ChunkedResponseHandler.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/ChunkedResponseHandler.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/ChunkedResponseHandler.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderGemini.java
Outdated
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.
Please address the topics raised in the previous PR
b7b5979
to
82d7a2b
Compare
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 an excellent refactor; just a few more polishing points.
...pik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropicMapper.java
Outdated
Show resolved
Hide resolved
...pik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderAnthropicMapper.java
Show resolved
Hide resolved
...pik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientGenerator.java
Outdated
Show resolved
Hide resolved
...pik-backend/src/main/java/com/comet/opik/domain/llmproviders/LlmProviderClientGenerator.java
Outdated
Show resolved
Hide resolved
...-backend/src/test/java/com/comet/opik/domain/llmproviders/LlmProviderClientsMappersTest.java
Outdated
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.
Improvements, including mappers etc. look great!
This information is taken from: https://ai.google.dev/gemini-api/docs/models/gemini | ||
*/ | ||
@RequiredArgsConstructor | ||
public enum GeminiModelName { |
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.
Make sure these values are as complete as possible and also aligned with the FrontEnd choices.
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 double checked against the gemini documentation (link in comments). Will update the FE ticket accordingly.
13892f3
to
396c28c
Compare
Details
Issues
OPIK-611
Testing
Covered new logic with tests