Allison/eng 1108 flexible model support eg via litellm #35
Conversation
This file contains hidden or 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
dexhorthy
reviewed
Mar 25, 2025
Contributor
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to b7d1dcf in 3 minutes and 14 seconds
More details
- Looked at
2262lines of code in11files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. kubechain/internal/controller/llm/llm_controller.go:225
- Draft comment:
The block in the 'mistral' case calls GenerateFromSinglePrompt as a validation step. Consider adding more detailed error context (or logging) here to help diagnose issues if the test call fails. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. kubechain/internal/controller/llm/llm_controller.go:162
- Draft comment:
In the OpenAI provider section, the API type and version are conditionally set. Consider handling the case where APIType is empty explicitly, or document that empty defaults to OPEN_AI. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code already handles empty APIType implicitly through the default case. The behavior is clear - if APIType is empty, it won't enter the if block at all, and if it has an unrecognized value, it defaults to OpenAI. The comment suggests making this more explicit, but that wouldn't change the behavior and might just add unnecessary verbosity.
The comment does point out a documentation opportunity - future maintainers might benefit from knowing the default behavior explicitly.
While documentation could help, the code is already clear and follows standard Go patterns for handling defaults. The behavior can be understood from reading the code.
The comment should be deleted as it suggests documenting something that is already clearly handled in the code using standard Go patterns.
3. kubechain/internal/controller/llm/llm_controller.go:334
- Draft comment:
The validateSecret function returns the API key as a string. Ensure that secrets are handled securely and consider trimming whitespace if necessary. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
4. kubechain/internal/controller/llm/llm_controller_test.go:296
- Draft comment:
The test case for OpenAI configuration covers a successful validation scenario. Consider adding negative test cases for other providers for more comprehensive coverage. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. kubechain/internal/controller/llm/llm_controller.go:261
- Draft comment:
When using the vertex provider, credentials are applied via WithCredentialsJSON without validating that the API key string is valid JSON. Consider adding basic JSON validation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment identifies a real potential issue - if invalid JSON is passed, it would fail later during provider initialization rather than giving a clear error. However, looking at the code, any errors are already caught and returned with context in the validateProviderConfig function. The error handling on line 273-274 would catch and properly report JSON parsing errors. Adding explicit JSON validation wouldn't meaningfully improve the error handling.
The comment identifies a real technical concern. Invalid JSON could cause confusing errors. We should be careful about dismissing input validation suggestions.
While input validation is important, the existing error handling already adequately covers this case. Adding explicit JSON validation would be redundant since JSON parsing errors are already caught and reported clearly.
The comment should be deleted since the existing error handling already adequately handles invalid JSON cases, making explicit validation unnecessary.
6. kubechain/internal/controller/llm/llm_controller_test.go:330
- Draft comment:
Several test cases for providers (Anthropic, Mistral, Google, Vertex) are disabled (using XIt). A comment explaining when/why to enable them would improve clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for an explanation on when/why to enable certain test cases, which falls under asking the PR author to explain or ensure something. This violates the rule against asking for explanations or confirmations.
7. kubechain/go.mod:10
- Draft comment:
Dependency updates (e.g. langchaingo v0.1.13) appear in go.mod. Please ensure compatibility with other modules and that test environments use these versions. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. kubechain/internal/controller/llm/llm_controller.go:62
- Draft comment:
Consider replacing 'base url' with 'base URL' for consistency and clarity in the comment on lines 62-67. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
While technically correct that URL should be capitalized, this is an extremely minor documentation issue. The meaning is perfectly clear either way. The comment doesn't point out any actual code issues or suggest meaningful improvements to the code quality. It's the kind of nitpicky comment that adds noise without value.
The capitalization of URL is a widely accepted standard in technical writing, so the comment is technically correct. Consistency in documentation can be important for professionalism.
While technically correct, this level of nitpicking about documentation formatting doesn't meet our bar for "clearly a code change required". The meaning is clear either way, and this kind of minor style feedback creates unnecessary noise.
Delete this comment as it's too minor of a documentation formatting issue to be worth addressing. It doesn't point out any actual code problems.
9. kubechain/internal/controller/llm/llm_controller.go:334
- Draft comment:
In the TODO comment on line 334, consider capitalizing 'Amazon Bedrock' and using a single question mark for clarity and consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is suggesting a change in a TODO comment, which is not directly related to the code functionality or logic. It is more of a stylistic suggestion and does not impact the code's behavior.
Workflow ID: wflow_kFfofjSco68fH1Sq
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
dexhorthy
approved these changes
Mar 25, 2025
dexhorthy
reviewed
Mar 28, 2025
dexhorthy
reviewed
Mar 28, 2025
dexhorthy
reviewed
Apr 3, 2025
dexhorthy
reviewed
Apr 3, 2025
kubechain/internal/controller/taskrun/taskrun_controller_test.go
Outdated
Show resolved
Hide resolved
dexhorthy
reviewed
Apr 3, 2025
dexhorthy
reviewed
Apr 3, 2025
dexhorthy
reviewed
Apr 3, 2025
dexhorthy
approved these changes
Apr 3, 2025
Contributor
dexhorthy
left a comment
There was a problem hiding this comment.
approved, we can iterate from here
Co-authored-by: Dex <dexter@humanlayer.dev>
Co-authored-by: Dex <dexter@humanlayer.dev>
balanceiskey
approved these changes
Apr 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Draft because tests are important and so is cleanup but posting in case anyone curious wants to glance to see how they feel about this direction.
Important
Adds flexible model support for multiple LLM providers with new configuration options and extensive testing.
llm_controller.go.BaseConfigandProviderConfigfor common and provider-specific settings inllm_types.go.kubechain.humanlayer.dev_llms.yamlto include new provider configurations.kubechain_v1alpha1_llm.yaml.llm_controller_test.go.LLMTestFixturefor setting up test resources.This description was created by
for b7d1dcf. It will automatically update as commits are pushed.