feat(models): implement dynamic pricing tiers for models #514
Conversation
|
Claude finished @steebchen's task —— View job PR Review: Dynamic Pricing Tiers Implementation ✅Completed comprehensive code review of the dynamic pricing tiers implementation. Todo List:
Review Summary:✅ Strengths:
📝 Recommendations:
The core implementation is solid but needs the fallback issue addressed before merging. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
Good Implementation: The dynamic pricing tiers feature is well-designed and properly integrated. The code structure is clean and the test coverage demonstrates the functionality works as intended.
Critical Issue Found: The main concern is the missing fallback pricing logic in costs.ts. When no tier matches the context size, the code falls back to inputPrice/outputPrice properties that no longer exist on models using pricingTiers, resulting in zero costs.
Recommendations:
- Fix the fallback logic in
costs.tsto handle cases where context size doesn't match any tier - Enhance test coverage with edge cases (boundary values, exceeding max tier, etc.)
- Consider adding validation to ensure pricing tiers are properly configured and cover the full context range
The implementation concept is solid, but the fallback handling needs to be addressed before merging to prevent incorrect cost calculations.
| if (applicableTier) { | ||
| inputPrice = applicableTier.inputPrice; | ||
| outputPrice = applicableTier.outputPrice; | ||
| } |
There was a problem hiding this comment.
Critical Issue: When no pricing tier matches the context size, the code falls back to providerInfo.inputPrice and providerInfo.outputPrice, but these were removed from the qwen3-coder-plus model definition. This means inputPrice and outputPrice will be undefined, defaulting to 0 in the calculation, resulting in zero costs.
Suggestion: Add explicit error handling or a default fallback tier:
if (applicableTier) {
inputPrice = applicableTier.inputPrice;
outputPrice = applicableTier.outputPrice;
} else {
// Log warning and either throw error or use highest tier as fallback
console.warn(`No pricing tier found for context size ${totalContextSize} for model ${model}`);
// Option 1: Use highest tier as fallback
const highestTier = providerInfo.pricingTiers[providerInfo.pricingTiers.length - 1];
inputPrice = highestTier.inputPrice;
outputPrice = highestTier.outputPrice;
}| expect(largeResult.inputCost).toBeCloseTo(1.8); // 300000 * (6/1e6) | ||
| expect(largeResult.outputCost).toBeCloseTo(6.0); // 100000 * (60/1e6) | ||
| expect(largeResult.totalCost).toBeCloseTo(7.8); // 1.8 + 6.0 | ||
| }); |
There was a problem hiding this comment.
Missing Test Coverage: Consider adding edge case tests:
- Context size exceeding maximum tier: What happens with 1,500,000 tokens (beyond the 1M limit)?
- Context size at tier boundaries: Test exactly 32000, 32001, 128000, 128001, etc.
- Zero or negative context size: Edge case handling
- Empty pricing tiers array: Defensive programming test
Example additional test:
it("should handle context size exceeding maximum tier", () => {
const result = calculateCosts("qwen3-coder-plus", "alibaba", 1500000, 100000, null);
// Should either use highest tier or handle gracefully
expect(result.inputCost).not.toBe(0); // Shouldn't be zero due to missing fallback
});| }, | ||
| { | ||
| minContextSize: 256001, | ||
| maxContextSize: 1000000, |
There was a problem hiding this comment.
Potential Gap: The highest tier ends at 1,000,000 tokens, which matches the model's contextSize, but what happens if the actual usage exceeds this? Consider:
- Adding an overflow tier for contexts > 1M tokens
- Or explicitly documenting that contexts > 1M should be rejected upstream
The current implementation could result in zero costs for contexts exceeding 1M tokens due to the missing fallback in the cost calculation logic.
Added support for pricing tiers based on context size for models. Updated cost calculation logic to account for dynamic pricing. Introduced unit tests to validate tier-based cost computation.
steebchen
force-pushed
the
feat/models-dynamic-pricing
branch
from
3c352e6 to
056eda9
Compare
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes introduce tiered pricing for the "qwen3-coder-plus" model under the Alibaba provider, allowing input and output token costs to vary based on context size. This is implemented by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant calculateCosts
participant ProviderModelMapping
Caller->>calculateCosts: Call with providerInfo, promptTokens, completionTokens
calculateCosts->>ProviderModelMapping: Check for pricingTiers
alt pricingTiers exist
calculateCosts->>calculateCosts: Sum promptTokens + completionTokens as contextSize
calculateCosts->>ProviderModelMapping: Find tier matching contextSize
calculateCosts->>calculateCosts: Use tier's inputPrice/outputPrice
else no pricingTiers
calculateCosts->>calculateCosts: Use default inputPrice/outputPrice
end
calculateCosts->>Caller: Return calculated costs
Estimated code review effort2 (~15 minutes) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This pull request implements dynamic pricing tiers for models based on context size, replacing the previous flat pricing structure. The change allows models to have different pricing rates depending on the total token count (prompt + completion tokens), enabling more granular cost calculations.
- Added
PricingTierinterface andpricingTiersfield to model definitions - Updated cost calculation logic to select appropriate pricing tier based on total context size
- Implemented comprehensive unit tests to validate tier-based pricing calculations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/models/src/models.ts | Added PricingTier interface and pricingTiers field to ProviderModelMapping |
| packages/models/src/models/alibaba.ts | Migrated qwen3-coder-plus model from flat pricing to tiered pricing structure |
| apps/gateway/src/lib/costs.ts | Updated calculateCosts function to support pricing tier selection based on context size |
| apps/gateway/src/lib/costs.spec.ts | Added unit tests validating pricing tier calculations for different context sizes |
| const totalContextSize = | ||
| calculatedPromptTokens + calculatedCompletionTokens; | ||
|
|
||
| // Find the appropriate pricing tier based on context size | ||
| const applicableTier = providerInfo.pricingTiers.find( | ||
| (tier) => | ||
| totalContextSize >= tier.minContextSize && | ||
| totalContextSize <= tier.maxContextSize, |
There was a problem hiding this comment.
The term 'totalContextSize' is misleading. This variable represents the sum of prompt and completion tokens, not the actual context size of the model. Consider renaming to 'totalTokens' or 'totalTokenCount' to accurately reflect what it represents.
| const totalContextSize = | |
| calculatedPromptTokens + calculatedCompletionTokens; | |
| // Find the appropriate pricing tier based on context size | |
| const applicableTier = providerInfo.pricingTiers.find( | |
| (tier) => | |
| totalContextSize >= tier.minContextSize && | |
| totalContextSize <= tier.maxContextSize, | |
| const totalTokens = | |
| calculatedPromptTokens + calculatedCompletionTokens; | |
| // Find the appropriate pricing tier based on context size | |
| const applicableTier = providerInfo.pricingTiers.find( | |
| (tier) => | |
| totalTokens >= tier.minContextSize && | |
| totalTokens <= tier.maxContextSize, |
| calculatedPromptTokens + calculatedCompletionTokens; | ||
|
|
||
| // Find the appropriate pricing tier based on context size | ||
| const applicableTier = providerInfo.pricingTiers.find( |
There was a problem hiding this comment.
The pricing tier selection logic may fail when the total token count exceeds all defined tier ranges or falls into gaps between tiers. There's no fallback mechanism if no tier matches the token count, which could result in zero pricing when inputPrice and outputPrice are undefined.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Added support for pricing tiers based on context size for models. Updated cost calculation logic to account for dynamic pricing. Introduced unit tests to validate tier-based cost computation.
Summary by CodeRabbit
New Features
Bug Fixes
Tests