feat: modelcatelog enhancements - #744
Conversation
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughRefactors rate-limit detection to use a renamed exported function, adds heuristic provider-resolution and a model refinement method to the model catalog, enables parallel execution for many provider tests, adjusts a couple of test expectations, and bumps multiple component versions and changelogs. Changes
Sequence Diagram(s)sequenceDiagram
participant Retry as RetryLogic
participant Utils as IsRateLimitErrorMessage
participant Backoff as BackoffManager
Retry->>Utils: Inspect error (message/type)
alt Rate-limit detected
Utils-->>Retry: true
Retry->>Backoff: apply rate-limit backoff & retry
else Not rate-limit
Utils-->>Retry: false
Retry-->>Backoff: normal/error handling
end
sequenceDiagram
participant Catalog as ModelCatalog
participant Heuristics as HeuristicChecks
participant Result as ProviderList
Catalog->>Heuristics: GetProvidersForModel(model)
Heuristics->>Heuristics: check OpenRouter heuristic
Heuristics->>Result: maybe append OpenRouter
Heuristics->>Heuristics: check Vertex heuristic
Heuristics->>Result: maybe append Vertex
Heuristics->>Heuristics: check Groq/OpenAI heuristic (gpt-*)
Heuristics->>Result: maybe append Groq
Heuristics->>Heuristics: check Bedrock/Anthropic heuristic (claude)
Heuristics->>Result: maybe append Bedrock
Result-->>Catalog: return provider list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (44)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/core-providers/cross_provider_test.go (2)
13-13: Good preparation for parallel execution.The
t.Parallel()call correctly prepares this test for concurrent execution when the skip is removed. The pattern aligns well with the PR's parallelization goals.As an optional enhancement when this test is enabled, consider adding
t.Parallel()to the nested subtests (lines 106 and 110) for finer-grained parallelism:for _, scenario := range scenariosList { // Test each scenario with both Chat Completions and Responses API t.Run(scenario.Name+"_ChatCompletions", func(t *testing.T) { + t.Parallel() scenarios.RunCrossProviderScenarioTest(t, client, ctx, testConfig, scenario, false) // false = Chat Completions API }) t.Run(scenario.Name+"_ResponsesAPI", func(t *testing.T) { + t.Parallel() scenarios.RunCrossProviderScenarioTest(t, client, ctx, testConfig, scenario, true) // true = Responses API }) }
117-117: Good preparation for parallel execution.The
t.Parallel()call correctly prepares this test for concurrent execution when the skip is removed.As an optional enhancement when this test is enabled, consider adding
t.Parallel()to the nested subtests (lines 143 and 147) for finer-grained parallelism:// Test same prompt across different providers t.Run("SamePrompt_DifferentProviders_ChatCompletions", func(t *testing.T) { + t.Parallel() scenarios.RunCrossProviderConsistencyTest(t, client, ctx, testConfig, false) // Chat Completions }) t.Run("SamePrompt_DifferentProviders_ResponsesAPI", func(t *testing.T) { + t.Parallel() scenarios.RunCrossProviderConsistencyTest(t, client, ctx, testConfig, true) // Responses API })tests/core-providers/parasail_test.go (1)
50-50: Consider deferring client shutdown for robust cleanup.While this is pre-existing code, consider deferring
client.Shutdown()to ensure cleanup occurs even if the test panics or fails. This becomes more important with parallel execution where multiple tests may fail simultaneously.Apply this diff to defer the shutdown:
defer cancel() + defer client.Shutdown() testConfig := config.ComprehensiveTestConfig{Then remove line 50:
t.Run("ParasailTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown() }tests/core-providers/README.md (1)
117-126: Consider consolidating duplicate parallel execution documentation.Lines 117-119 and 125-126 repeat information already covered in the dedicated "Parallel Test Execution" section (lines 24-46). Consider removing the duplicate note at lines 125-126 to keep the documentation DRY.
framework/modelcatalog/main.go (1)
220-263: Consider performance implications of nested loops under read lock.The special provider handling logic adds valuable heuristics for cross-provider model discovery (OpenRouter, Vertex, Groq, Bedrock). However, with multiple
slices.Containschecks and nested loops (particularly lines 254-260), this could impact performance ifmodelPoolis large.Consider:
- Adding early termination once all special cases are satisfied
- Profiling with realistic model pool sizes
- Potentially caching common lookups if this becomes a hot path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/bifrost.go(1 hunks)core/bifrost_test.go(5 hunks)core/utils.go(1 hunks)framework/modelcatalog/main.go(3 hunks)tests/core-providers/README.md(2 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/cross_provider_test.go(2 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(1 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/end_to_end_tool_calling.go(1 hunks)tests/core-providers/scenarios/transcription.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/vertex_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
core/bifrost.go (1)
core/utils.go (1)
IsRateLimitErrorMessage(96-112)
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
tests/core-providers/scenarios/validation_presets.go (2)
ConversationExpectations(131-137)ModifyExpectationsForProvider(326-373)
core/bifrost_test.go (1)
core/utils.go (1)
IsRateLimitErrorMessage(96-112)
framework/modelcatalog/main.go (1)
core/schemas/bifrost.go (5)
OpenRouter(48-48)Vertex(40-40)Groq(43-43)Bedrock(38-38)ModelProvider(32-32)
🔇 Additional comments (20)
tests/core-providers/cerebras_test.go (1)
13-13: LGTM! Parallel execution enabled correctly.The
t.Parallel()call is correctly placed at the start of the test function, and the test properly isolates its resources (client, context) for safe concurrent execution.tests/core-providers/gemini_test.go (1)
13-13: LGTM! Parallel execution enabled correctly.The placement of
t.Parallel()is correct and aligns with the PR's objective to enable concurrent provider test execution.Please verify that
config.SetupTest()is thread-safe for parallel execution, as multiple provider tests will now call it concurrently. Additionally, monitor for potential API rate limiting when running multiple Gemini tests in parallel—the-parallel 10flag mentioned in the PR instructions may trigger rate limits if the Gemini API has restrictive quotas.tests/core-providers/groq_test.go (1)
14-14: The t.Parallel() addition is safe and appropriate.Verification confirms that
SetupTest()is parallel-safe—it creates isolatedcontextand client instances per call with no shared mutable state. The single global variable found (AllProviderConfigs) is read-only test configuration data. The change aligns with PR objectives and introduces no race condition risks.tests/core-providers/bedrock_test.go (1)
13-13: Parallel execution is safe — change approved.Verification confirmed:
SetupTest()creates isolated resources per test invocation (fresh context, fresh bifrost client, fresh cancel function),AllProviderConfigsis read-only (never reassigned), and no shared mutable state exists in the call chain. Thet.Parallel()addition correctly enables concurrent execution without race conditions.tests/core-providers/parasail_test.go (1)
13-13: Verified: t.Parallel() addition is safe.SetupTest() is thread-safe for concurrent calls—each invocation creates isolated resources: new account instance, independent Bifrost initialization, and isolated context with timeout. No global mutable state or shared resources detected that would cause race conditions in parallel test execution.
tests/core-providers/ollama_test.go (1)
13-13: LGTM! Parallel execution properly enabled.The addition of
t.Parallel()is correctly placed and safe for concurrent execution, as each test creates an isolated client instance viaconfig.SetupTest().tests/core-providers/openrouter_test.go (1)
13-13: LGTM! Consistent parallel execution pattern.The
t.Parallel()addition follows the same pattern as other provider tests and enables safe concurrent execution.tests/core-providers/README.md (1)
24-46: Excellent documentation of parallel test execution.The new section clearly explains parallel testing capabilities, benefits, and usage patterns. The note about isolated client instances addresses potential concurrency concerns.
tests/core-providers/mistral_test.go (1)
13-13: LGTM! Parallel execution enabled.Consistent with the parallel execution pattern applied across all provider tests.
tests/core-providers/sgl_test.go (1)
13-13: LGTM! Parallel execution enabled.The addition follows the established pattern for enabling parallel test execution.
tests/core-providers/vertex_test.go (1)
13-13: LGTM! Parallel execution enabled.Consistent with the parallel execution pattern across all provider tests.
tests/core-providers/scenarios/transcription.go (1)
206-206: Verify intentional reduction in test coverage.The response formats have been reduced from
{"json", "verbose_json"}to{"json"}only. While this aligns with the PR objective to make test expectations more reliable, it does narrow test coverage.Confirm that:
- The
verbose_jsonformat is either tested elsewhere or intentionally excluded- This change is not a temporary workaround that should be addressed with proper verbose_json support
tests/core-providers/cohere_test.go (1)
13-13: LGTM! Parallel execution enabled.Consistent with the parallel execution pattern applied across all provider tests.
tests/core-providers/azure_test.go (1)
13-13: LGTM! Parallel execution enabled.The addition of
t.Parallel()enables this test to run concurrently with other tests, improving test suite performance.tests/core-providers/anthropic_test.go (1)
13-13: LGTM! Parallel execution enabled.Consistent with the broader pattern of enabling parallel test execution across provider tests.
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
172-175: LGTM! Test expectation relaxed for reliability.Removing "sunny" from the required keywords while keeping it as a warning-level check (lines 241-243, 258-260) is a sensible adjustment. Models may paraphrase weather descriptions differently (e.g., "clear", "fair", "sunny") even though the tool result contains "Sunny with light clouds". This change improves test reliability while still logging when the specific term isn't present.
core/utils.go (1)
95-96: LGTM! Rate-limit error detection function now exported.The rename from
isRateLimitErrortoIsRateLimitErrorMessagemakes this utility function public, allowing external packages to use the same rate-limit detection logic. The implementation remains unchanged and all call sites have been updated consistently.core/bifrost.go (1)
1989-1990: LGTM! Updated to use the exported rate-limit detection function.The call sites have been correctly updated to use
IsRateLimitErrorMessage, maintaining the same rate-limit detection behavior for both error messages and error types.core/bifrost_test.go (1)
356-562: LGTM! Test suite updated to use the public API.All test function names, comments, and call sites have been consistently updated to reference
IsRateLimitErrorMessage. The comprehensive test coverage for rate limit pattern detection remains intact, and the benchmark has been appropriately renamed toBenchmarkIsRateLimitError.tests/core-providers/openai_test.go (1)
13-13: LGTM: Parallel execution correctly enabled.The addition of
t.Parallel()is correctly placed and the test has proper isolation with its own client, context, and cleanup handlers.
ba5078f to
9782009
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
framework/modelcatalog/main.go (1)
284-294: Comment/implementation mismatch in RefineModelForProvider.The example comment on line 285 states that
"gpt-oss-120b"maps to"openai/gpt-4o-mini", but the implementation on line 290 returns"openai/" + model, which yields"openai/gpt-oss-120b", not"openai/gpt-4o-mini".This issue was previously flagged. Please clarify:
- If the comment is correct, update the implementation to map to the actual target model name
- If the implementation is correct, fix the comment to reflect the namespace prefix behavior
🧹 Nitpick comments (2)
tests/core-providers/cross_provider_test.go (1)
13-13: LGTM: Parallel execution markers added consistently.The
t.Parallel()calls are currently ineffective since both tests are immediately skipped, but this change aligns with the PR's goal of enabling parallel execution across all provider tests. When these tests are re-enabled, the parallel flag will already be in place.Consider either removing the skip to activate these tests or deferring the
t.Parallel()addition until they're ready to run.Also applies to: 117-117
framework/modelcatalog/main.go (1)
220-241: Consider consolidating duplicate OpenRouter and Vertex logic.The OpenRouter (lines 220-230) and Vertex (lines 232-241) blocks follow identical patterns. Consider extracting a helper function to reduce duplication.
Example consolidation:
// Helper function to check if provider should be inferred based on namespaced models func (mc *ModelCatalog) shouldInferProvider( targetProvider schemas.ModelProvider, currentProviders []schemas.ModelProvider, model string, ) bool { if slices.Contains(currentProviders, targetProvider) { return false } targetModels, ok := mc.modelPool[targetProvider] if !ok { return false } for _, provider := range currentProviders { if slices.Contains(targetModels, string(provider)+"/"+model) { return true } } return false }Then use:
if mc.shouldInferProvider(schemas.OpenRouter, providers, model) { providers = append(providers, schemas.OpenRouter) } if mc.shouldInferProvider(schemas.Vertex, providers, model) { providers = append(providers, schemas.Vertex) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
core/bifrost.go(1 hunks)core/bifrost_test.go(5 hunks)core/changelog.md(1 hunks)core/utils.go(1 hunks)core/version(1 hunks)framework/changelog.md(1 hunks)framework/modelcatalog/main.go(4 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)tests/core-providers/README.md(2 hunks)tests/core-providers/anthropic_test.go(1 hunks)tests/core-providers/azure_test.go(1 hunks)tests/core-providers/bedrock_test.go(1 hunks)tests/core-providers/cerebras_test.go(1 hunks)tests/core-providers/cohere_test.go(1 hunks)tests/core-providers/cross_provider_test.go(2 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(1 hunks)tests/core-providers/mistral_test.go(1 hunks)tests/core-providers/ollama_test.go(1 hunks)tests/core-providers/openai_test.go(1 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(1 hunks)tests/core-providers/scenarios/end_to_end_tool_calling.go(1 hunks)tests/core-providers/scenarios/transcription.go(1 hunks)tests/core-providers/sgl_test.go(1 hunks)tests/core-providers/vertex_test.go(1 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- plugins/governance/version
- plugins/jsonparser/changelog.md
- plugins/otel/version
- plugins/mocker/version
- plugins/logging/changelog.md
- plugins/semanticcache/version
- plugins/logging/version
- plugins/telemetry/version
- plugins/jsonparser/version
- plugins/governance/changelog.md
- core/version
- core/changelog.md
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/core-providers/mistral_test.go
- core/bifrost_test.go
- tests/core-providers/azure_test.go
- core/utils.go
- tests/core-providers/cohere_test.go
- tests/core-providers/groq_test.go
- tests/core-providers/bedrock_test.go
- tests/core-providers/openai_test.go
- core/bifrost.go
- tests/core-providers/README.md
- tests/core-providers/gemini_test.go
- tests/core-providers/anthropic_test.go
- tests/core-providers/cerebras_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
framework/modelcatalog/main.go (1)
core/schemas/bifrost.go (5)
OpenRouter(48-48)Vertex(40-40)Groq(43-43)Bedrock(38-38)ModelProvider(32-32)
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
tests/core-providers/scenarios/validation_presets.go (2)
ConversationExpectations(131-137)ModifyExpectationsForProvider(326-373)
🔇 Additional comments (15)
tests/core-providers/openrouter_test.go (1)
13-13: LGTM! Parallel execution enabled correctly.The
t.Parallel()call is correctly placed and enables this test to run concurrently with other provider tests, improving overall test suite performance. The test structure supports parallel execution with isolated client setup and proper cleanup.tests/core-providers/parasail_test.go (1)
13-13: LGTM! Parallel execution enabled correctly.The addition of
t.Parallel()is appropriate here. The test uses isolated resources (dedicated client and context) and has proper cleanup, making it safe for concurrent execution with other provider tests.transports/version (1)
1-1: LGTM! Version bump applied.Routine metadata update to reflect the release version.
plugins/mocker/changelog.md (1)
4-4: LGTM! Changelog entry updated.Correctly documents the core and framework version bumps.
plugins/maxim/version (1)
1-1: LGTM! Version bump applied.Routine metadata update for the plugin release.
framework/version (1)
1-1: LGTM! Framework version bumped.Metadata update reflects the framework enhancements in this release.
plugins/semanticcache/changelog.md (1)
4-4: LGTM! Changelog entry updated.Correctly documents the core and framework dependency version bumps.
plugins/otel/changelog.md (1)
4-4: LGTM! Changelog entry updated.Correctly documents the dependency version bumps.
plugins/telemetry/changelog.md (1)
4-4: LGTM! Changelog entry updated.Correctly documents the core and framework version bumps.
tests/core-providers/sgl_test.go (1)
13-13: LGTM! Parallel execution enabled.Adding
t.Parallel()will improve test suite performance by allowing this test to run concurrently with other provider tests. The test is properly isolated with its own client and context, making it safe for parallel execution.tests/core-providers/vertex_test.go (1)
13-13: LGTM! Parallel test execution enabled.The addition of
t.Parallel()allows this test to run concurrently with other parallel tests, improving test suite performance.tests/core-providers/ollama_test.go (1)
13-13: LGTM! Parallel test execution enabled.Consistent with the parallel test execution pattern applied across the provider test suite.
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
172-174: LGTM! More reliable test expectations.Removing "sunny" from required keywords while keeping it as a warning makes the test more robust. LLMs may paraphrase weather descriptions differently while still correctly incorporating the tool result data (location and temperature), which are the essential elements to validate.
framework/modelcatalog/main.go (1)
252-262: Review permissive substring matching in Bedrock provider inference.The Bedrock logic uses
strings.Contains(bedrockModel, model)which is more permissive than the exact matching in the Groq block (line 246). This could lead to false positives ifmodelis a substring of a different Bedrock model name.For example, if
model="opus"and a Bedrock model is"claude-3-opus", the condition would match even though "opus" alone isn't a valid model identifier.Consider whether stricter matching is needed:
// More precise matching - check for exact model or known prefix patterns if !slices.Contains(providers, schemas.Bedrock) && strings.Contains(model, "claude") { if bedrockModels, ok := mc.modelPool[schemas.Bedrock]; ok { for _, bedrockModel := range bedrockModels { // Match exact model or with version suffix (e.g., claude-3 matches claude-3-opus) if bedrockModel == model || strings.HasPrefix(bedrockModel, model+"-") { providers = append(providers, schemas.Bedrock) break } } } }tests/core-providers/scenarios/transcription.go (1)
206-206: No action required—the change is intentional and properly documented.The "verbose_json" format was deliberately replaced with "json" for whisper-1 provider compatibility, as evidenced by the comment at line 259. The comment at line 205 explains that response formats are tested while excluding "text" to avoid JSON parsing issues, justifying the choice of "json" only. The change is coordinated across the test suite with consistent rationale.
9782009 to
f34e7ee
Compare
Merge activity
|
## Summary Renamed `isRateLimitError` to `IsRateLimitErrorMessage` to make it a public function and enhanced the model catalog to better handle cross-provider model compatibility. ## Changes - Renamed `isRateLimitError` to `IsRateLimitErrorMessage` and updated all references - Enhanced `GetProvidersForModel` in the model catalog to handle special provider cases: - Added support for OpenRouter models - Added support for Vertex models - Added support for Groq models that use OpenAI naming - Added support for Bedrock models that use Anthropic naming - Added `RefineModelForProvider` function to handle model name translations - Added parallel test execution for all provider tests using `t.Parallel()` - Updated test documentation to explain parallel test execution benefits - Modified tool calling test expectations to be more reliable ## Type of change - [ ] Bug fix - [x] Feature - [x] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (Next.js) - [x] Docs ## How to test ```sh # Core/Transports go test ./tests/core-providers/ -v # Test parallel execution go test ./tests/core-providers/ -parallel 10 ``` ## Breaking changes - [ ] Yes - [x] No ## Security considerations No security implications. ## Checklist - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI)
2 participants
Summary
Renamed
isRateLimitErrortoIsRateLimitErrorMessageto make it a public function and enhanced the model catalog to better handle cross-provider model compatibility.Changes
isRateLimitErrortoIsRateLimitErrorMessageand updated all referencesGetProvidersForModelin the model catalog to handle special provider cases:RefineModelForProviderfunction to handle model name translationst.Parallel()Type of change
Affected areas
How to test
Breaking changes
Security considerations
No security implications.
Checklist