Conversation
github-actions
bot
added
the
Evaluation
There was a problem hiding this comment.
Pull request overview
This pull request aims to fix token usage calculation in the RAI service evaluator by converting token counts from strings to integers. The PR changes how prompt_tokens, completion_tokens, and total_tokens are handled in two locations within the _parse_eval_result method.
Changes:
- Convert token counts from strings to integers to align with codebase conventions
- Change default token values from empty strings to 0 (integer)
- Update total token calculation to use integers instead of string concatenation
| total_tokens = ( | ||
| str(int(prompt_tokens) + int(completion_tokens)) | ||
| int(prompt_tokens) + int(completion_tokens) | ||
| if prompt_tokens and completion_tokens | ||
| else "" | ||
| else 0 | ||
| ) |
There was a problem hiding this comment.
Same bug as lines 307-311. The condition on line 343 checks if both tokens are truthy, which incorrectly returns 0 when either value is 0. Since the variables are already integers, the calculation should be unconditional:
total_tokens = prompt_tokens + completion_tokens| prompt_tokens = int(metrics.get("promptTokens", 0)) if metrics.get("promptTokens") else 0 | ||
| completion_tokens = int(metrics.get("completionTokens", 0)) if metrics.get("completionTokens") else 0 |
There was a problem hiding this comment.
The logic for converting prompt_tokens and completion_tokens is redundant. The code first converts to int, then checks if the value is truthy and converts to int again. Consider simplifying to:
prompt_tokens = int(metrics.get("promptTokens", 0) or 0)
completion_tokens = int(metrics.get("completionTokens", 0) or 0)This handles both string and None values correctly while avoiding double conversion.
| prompt_tokens = int(metrics.get("promptTokens", 0)) if metrics.get("promptTokens") else 0 | |
| completion_tokens = int(metrics.get("completionTokens", 0)) if metrics.get("completionTokens") else 0 | |
| prompt_tokens = int(metrics.get("promptTokens", 0) or 0) | |
| completion_tokens = int(metrics.get("completionTokens", 0) or 0) |
| total_tokens = ( | ||
| str(int(prompt_tokens) + int(completion_tokens)) | ||
| int(prompt_tokens) + int(completion_tokens) | ||
| if prompt_tokens and completion_tokens | ||
| else "" | ||
| else 0 | ||
| ) |
There was a problem hiding this comment.
The condition on line 309 checks if both prompt_tokens and completion_tokens are truthy, which means if either value is 0, total_tokens will be set to 0 instead of the correct sum. Since prompt_tokens and completion_tokens are already guaranteed to be integers (defaulting to 0), this condition is incorrect. The calculation should be unconditional:
total_tokens = prompt_tokens + completion_tokensThis bug causes total_tokens to be 0 when either prompt_tokens or completion_tokens is 0, even though the other might have a non-zero value.
| parsed_result[f"{self._eval_metric.value}_prompt_tokens"] = int(prompt_tokens) if prompt_tokens else 0 | ||
| parsed_result[f"{self._eval_metric.value}_completion_tokens"] = int(completion_tokens) if completion_tokens else 0 |
There was a problem hiding this comment.
The int() conversion here is redundant since prompt_tokens and completion_tokens are already integers from lines 302-303. The variables can be used directly:
parsed_result[f"{self._eval_metric.value}_prompt_tokens"] = prompt_tokens
parsed_result[f"{self._eval_metric.value}_completion_tokens"] = completion_tokensAdditionally, the condition if prompt_tokens else 0 is unnecessary since prompt_tokens is already guaranteed to be an integer.
| prompt_tokens = int(metrics.get("promptTokens", 0)) if metrics.get("promptTokens") else 0 | ||
| completion_tokens = int(metrics.get("completionTokens", 0)) if metrics.get("completionTokens") else 0 |
There was a problem hiding this comment.
Same redundancy issue as lines 302-303. Consider simplifying to:
prompt_tokens = int(metrics.get("promptTokens", 0) or 0)
completion_tokens = int(metrics.get("completionTokens", 0) or 0)| prompt_tokens = int(metrics.get("promptTokens", 0)) if metrics.get("promptTokens") else 0 | |
| completion_tokens = int(metrics.get("completionTokens", 0)) if metrics.get("completionTokens") else 0 | |
| prompt_tokens = int(metrics.get("promptTokens", 0) or 0) | |
| completion_tokens = int(metrics.get("completionTokens", 0) or 0) |
1 participant
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines