fix(evaluations): use correct total_cost property in CostCalculator#6183
Conversation
The condition on line 28 checked costValues.total_price, but the property is total_cost everywhere else (models.json, evaluation runners, etc.). Since total_price is always undefined, the branch never executed and models that only define a flat total_cost rate fell through to the input_cost/output_cost path, producing NaN.
There was a problem hiding this comment.
Code Review
This pull request corrects a property name mismatch in the cost calculation logic by changing total_price to total_cost. The feedback suggests using a nullish check for both total_cost and totalTokens to properly handle free models and prevent potential NaN results.
| } | ||
|
|
||
| if (costValues.total_price > 0) { | ||
| if (costValues.total_cost > 0) { |
There was a problem hiding this comment.
The fix correctly addresses the property name mismatch. However, using > 0 will cause models with a zero cost (free models) to fall through to the else block, which may still result in NaN if input_cost or output_cost are missing. Additionally, checking for the existence of totalTokens prevents NaN results if that metric is missing. Using a nullish check (!= null) is consistent with the project's standard idiom for covering both null and undefined.
| if (costValues.total_cost > 0) { | |
| if (costValues.total_cost != null && totalTokens != null) { |
References
- In JavaScript/TypeScript, use loose equality (== null) as a standard idiom for a 'nullish' check that covers both null and undefined.
|
thank you! |
What happened
CostCalculator.tsline 28 checkscostValues.total_price, but every other part of the codebase (models.json, evaluation runners) usestotal_cost. Sincetotal_priceis alwaysundefined, the condition is never true and models that define a flattotal_costrate fall through to theinput_cost/output_costpath — which are also undefined for those models, producingNaNcosts.Fix
Change
total_price→total_coston line 28 to match the actual property name.