feat: add model pricing data to Bifrost and model responses - #745
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Warning Rate limit exceeded@Pratham-Mishra04 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a public method to read configured model providers, implements pagination for model listings, refactors pricing data shape and loading, adds a ModelCatalog accessor to fetch pricing per model, enriches listModels responses with pricing, tightens speech input validation, and updates docs/changelogs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPHandler as listModels Handler
participant Bifrost
participant ModelCatalog
Client->>HTTPHandler: GET /v1/models
HTTPHandler->>Bifrost: GetConfiguredProviders()
Bifrost-->>HTTPHandler: []ModelProvider
HTTPHandler->>Bifrost: ListModels(providers)
Bifrost-->>HTTPHandler: BifrostListModelsResponse
alt Pricing manager configured
loop per model
HTTPHandler->>ModelCatalog: GetPricingEntryForModel(modelID, provider)
ModelCatalog-->>HTTPHandler: *PricingEntry / nil
HTTPHandler->>HTTPHandler: attach Pricing to response item (if present)
end
end
HTTPHandler-->>Client: ListModelsResponse (with optional Pricing)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
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: 5
🧹 Nitpick comments (1)
core/bifrost.go (1)
376-378: Avoid duplicate AddPricing calls.AddPricing already runs in ListModelsRequest; calling it again in ListAllModels is redundant.
Apply this diff:
- // Add pricing data to the response - response.AddPricing(bifrost.GetPricingDataForModel)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/bifrost.go(5 hunks)core/schemas/models.go(4 hunks)framework/modelcatalog/main.go(4 hunks)framework/modelcatalog/sync.go(3 hunks)framework/modelcatalog/utils.go(3 hunks)transports/bifrost-http/lib/config.go(2 hunks)transports/bifrost-http/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
transports/bifrost-http/server.go (2)
framework/modelcatalog/main.go (1)
Config(26-30)transports/bifrost-http/lib/config.go (1)
Config(139-169)
transports/bifrost-http/lib/config.go (1)
core/schemas/models.go (1)
DataSheetPricingEntry(200-229)
core/bifrost.go (2)
core/schemas/models.go (1)
DataSheetPricingEntry(200-229)core/schemas/bifrost.go (1)
ModelProvider(32-32)
framework/modelcatalog/utils.go (1)
core/schemas/models.go (1)
DataSheetPricingEntry(200-229)
core/schemas/models.go (2)
core/schemas/bifrost.go (1)
ModelProvider(32-32)core/schemas/provider.go (1)
Provider(199-226)
framework/modelcatalog/sync.go (2)
framework/modelcatalog/main.go (1)
ModelCatalog(32-56)core/schemas/models.go (1)
DataSheetPricingEntry(200-229)
framework/modelcatalog/main.go (1)
core/schemas/models.go (1)
DataSheetPricingEntry(200-229)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
transports/bifrost-http/server.go (1)
732-734: Bootstrap pricing propagation looks good.This correctly seeds the client with pricing after catalog load.
core/schemas/models.go (1)
118-131: Pricing attachment logic looks solid.Sets DataSheetPricingEntry irrespective of preexisting Pricing object.
transports/bifrost-http/lib/config.go (1)
920-924: Review comment references non-existent methodGetBifrostClient()The data race concern is valid—both callbacks at lines 467–468 and 921–922 use unsynchronized direct reads of
config.clientwhileSetBifrostClient(line 1523) writes withmuMCPlock protection. However, the suggested diff cannot be applied as written becauseGetBifrostClient()does not exist in the codebase.To properly fix the data race, either:
- Create a thread-safe
GetBifrostClient()getter method that acquires themuMCPlock before returning the client, then apply the suggested diff, OR- Modify both callbacks to acquire the mutex directly, similar to how
AddMCPClientusesc.muMCP.Lock()for client field access.
Pratham-Mishra04
changed the title
951ed2f to
49db44b
Compare
ba5078f to
9782009
Compare
49db44b to
b31416f
Compare
9782009 to
f34e7ee
Compare
b31416f to
8fc37ed
Compare
8fc37ed to
7e6b09b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
framework/modelcatalog/sync.go (1)
131-134: Invoke the pricing callback with normalized data.Passing the raw map from the download keeps provider-prefixed keys, so downstream consumers (e.g.
Bifrost.SetPricingData) see inconsistent identifiers and produce incorrect keys. Call the accessor that already returns our canonicalized map instead.Apply this diff:
- if mc.pricingSyncCallback != nil { - mc.pricingSyncCallback(pricingData) + if mc.pricingSyncCallback != nil { + mc.pricingSyncCallback(mc.GetPricingData())core/bifrost.go (1)
838-840: Normalize incoming pricing keys before storing.Once the catalog starts returning provider-qualified keys, this loop will emit
provider/provider/model, corrupting lookups. Strip the provider prefix from the incoming key before composing the final storage key.Apply this diff:
func (bifrost *Bifrost) SetPricingData(pricingData map[string]schemas.DataSheetPricingEntry) { - for model, pricing := range pricingData { - bifrost.pricingData.Store(pricing.Provider+"/"+model, pricing) + for model, pricing := range pricingData { + normalized := model + prefix := pricing.Provider + "/" + if strings.HasPrefix(normalized, prefix) { + normalized = strings.TrimPrefix(normalized, prefix) + } + bifrost.pricingData.Store(pricing.Provider+"/"+normalized, pricing) } }framework/modelcatalog/main.go (1)
169-171: Fix pricing map key collisions across providers.Keying the export map by
modelalone overwrites entries whenever two providers expose the same model name (e.g. OpenAI vs Azure wrappers), so callers receive whichever entry happened to be processed last. Use a provider-qualified key (matching what the client expects) to preserve every variant.Apply this diff:
- model, _, _ := splitKey(key) - pricingData[model] = convertTableModelPricingToPricingData(pricing) + model, provider, _ := splitKey(key) + composite := provider + "/" + model + pricingData[composite] = convertTableModelPricingToPricingData(pricing)
🧹 Nitpick comments (1)
framework/modelcatalog/utils.go (1)
14-17: Defend against malformed pricing keys.
splitKeypanics on any key that lacks two delimiters (e.g. legacy rows, manual config edits). Add a length check and fall back safely to avoid taking down the pricing sync.Apply this diff:
func splitKey(key string) (string, string, string) { parts := strings.Split(key, "|") - return parts[0], parts[1], parts[2] + if len(parts) < 3 { + return key, "", "" + } + return parts[0], parts[1], parts[2] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/bifrost.go(5 hunks)core/schemas/models.go(4 hunks)framework/modelcatalog/main.go(4 hunks)framework/modelcatalog/sync.go(3 hunks)framework/modelcatalog/utils.go(3 hunks)transports/bifrost-http/lib/config.go(2 hunks)transports/bifrost-http/server/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/lib/config.go
🧰 Additional context used
🧬 Code graph analysis (6)
transports/bifrost-http/server/server.go (2)
framework/modelcatalog/main.go (1)
Config(26-30)transports/bifrost-http/lib/config.go (1)
Config(139-169)
framework/modelcatalog/sync.go (2)
framework/modelcatalog/main.go (1)
ModelCatalog(32-56)core/schemas/models.go (1)
DataSheetPricingEntry(200-229)
framework/modelcatalog/utils.go (1)
core/schemas/models.go (1)
DataSheetPricingEntry(200-229)
core/schemas/models.go (2)
core/schemas/bifrost.go (1)
ModelProvider(32-32)core/schemas/provider.go (1)
Provider(199-226)
framework/modelcatalog/main.go (1)
core/schemas/models.go (1)
DataSheetPricingEntry(200-229)
core/bifrost.go (2)
core/schemas/models.go (1)
DataSheetPricingEntry(200-229)core/schemas/bifrost.go (1)
ModelProvider(32-32)
🔇 Additional comments (4)
core/schemas/models.go (4)
48-108: LGTM! Well-implemented pagination with cursor validation.The pagination logic correctly handles edge cases including nil responses, invalid cursors, empty pages, and boundary conditions. The use of opaque tokens with LastID validation ensures cursor integrity.
110-110: LGTM! Clean function type definition.The type signature appropriately uses a pointer return to allow for optional pricing data.
136-136: LGTM! Field addition follows established patterns.The new
DeploymentNamefield is appropriately optional with a pointer type and correct JSON tags, consistent with other fields in the struct.
199-229: LGTM! Comprehensive and well-structured pricing data model.The
DataSheetPricingEntrystruct appropriately distinguishes required fields (basic token pricing, provider, mode) from optional fields (media, character-based, 128k+, cache/batch pricing). The naming is consistent and descriptive, and the use of pointers for optional fields is correct.
Pratham-Mishra04
changed the title
7e6b09b to
d990c56
Compare
d990c56 to
d2152f4
Compare
b0c0f3c to
05c00fe
Compare
Merge activity
|
05c00fe to
0a199a8
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
## Summary Added model pricing data support to the Bifrost core, enabling automatic pricing information in model listings and providing a mechanism to sync pricing data from external sources. ## Changes - Added a `pricingData` field to the Bifrost struct to store pricing information for models - Created new methods to set and retrieve pricing data for models - Added a `DataSheetPricingEntry` struct to represent detailed pricing information - Enhanced `BifrostListModelsResponse` with an `AddPricing` method to include pricing in model listings - Integrated pricing data with the model catalog framework - Added a callback mechanism to update pricing data when synced from external sources - Added `DeploymentName` field to the Model struct ## Type of change - [x] Feature - [x] Refactor ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) ## How to test ```sh # Core/Transports go version go test ./... # Test model listing with pricing data curl -X GET "http://localhost:8000/v1/models" -H "Authorization: Bearer $BIFROST_API_KEY" ``` ## Breaking changes - [x] No ## Related issues Enhances model listing capabilities with pricing information ## Security considerations No security implications as this only adds informational data to model responses. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI)
1 participant
Summary
Added model pricing data support to the Bifrost core, enabling automatic pricing information in model listings and providing a mechanism to sync pricing data from external sources.
Changes
pricingDatafield to the Bifrost struct to store pricing information for modelsDataSheetPricingEntrystruct to represent detailed pricing informationBifrostListModelsResponsewith anAddPricingmethod to include pricing in model listingsDeploymentNamefield to the Model structType of change
Affected areas
How to test
Breaking changes
Related issues
Enhances model listing capabilities with pricing information
Security considerations
No security implications as this only adds informational data to model responses.
Checklist