OPIK-796: Fix issue with LLM setup and stream shutdown #1108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ora/OPIK-796_implement_clickhouse_user_logs
…ora/OPIK-796_implement_clickhouse_user_logs
…ttps://github.com/comet-ml/opik into thiagohora/OPIK-796_implement_clickhouse_user_logs
…o thiagohora/OPIK-796_implement_get_log_api
…lication lifecycle
…ithub.com/comet-ml/opik into thiagohora/OPIK-796_implement_get_log_api
thiagohora
force-pushed
the
thiagohora/OPIK-796_implement_fix_current_issues
branch
from
62ad07f to
a64dd61
Compare
thiagohora
force-pushed
the
thiagohora/OPIK-796_implement_fix_current_issues
branch
from
9f0bdf9 to
9d86e17
Compare
BorisTkachenko
approved these changes
Jan 24, 2025
|
|
||
| @Provides | ||
| @Singleton | ||
| public LlmProviderFactory llmProviderFactory(LlmProviderApiKeyService llmProviderApiKeyService) { |
There was a problem hiding this comment.
Suggested change
| public LlmProviderFactory llmProviderFactory(LlmProviderApiKeyService llmProviderApiKeyService) { | |
| public LlmProviderFactory llmProviderFactory(@NonNull LlmProviderApiKeyService llmProviderApiKeyService) { |
| return createInstance(llmProviderApiKeyService); | ||
| } | ||
|
|
||
| public LlmProviderFactory createInstance(LlmProviderApiKeyService llmProviderApiKeyService) { |
There was a problem hiding this comment.
Suggested change
| public LlmProviderFactory createInstance(LlmProviderApiKeyService llmProviderApiKeyService) { | |
| public LlmProviderFactory createInstance(@NonNull LlmProviderApiKeyService llmProviderApiKeyService) { |
| } | ||
|
|
||
| @Override | ||
| public LlmProviderService getService(String apiKey) { |
There was a problem hiding this comment.
Suggested change
| public LlmProviderService getService(String apiKey) { | |
| public LlmProviderService getService(@NonNull String apiKey) { |
Other functions in this class as well
| } | ||
|
|
||
| @Override | ||
| public LlmProviderService getService(String apiKey) { |
There was a problem hiding this comment.
Suggested change
| public LlmProviderService getService(String apiKey) { | |
| public LlmProviderService getService(@NonNull String apiKey) { |
Same for other functions in this class
| @ParameterizedTest | ||
| @MethodSource("credentials") | ||
| @DisplayName("get logs per rule evaluators: when api key is present, then return proper response") | ||
| void getLogsPerRuleEvaluators__whenSessionTokenIsPresent__thenReturnProperResponse( |
There was a problem hiding this comment.
There are two getLogsPerRuleEvaluators__whenSessionTokenIsPresent__thenReturnProperResponse almost copypasted tests (only difference seems to be workspace). If you really need both it's worth incapsulating into function and reuse it.
There was a problem hiding this comment.
I will see what can be reused and add the validations in the next iteration
thiagohora
deleted the
thiagohora/OPIK-796_implement_fix_current_issues
branch
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Add tests to new logs endpoint and this PR also unveiled the following issues:
Issues
OPIK-796