fix mm input_tokens=0 when server return prompt_tokens#299
Merged
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the logic for determining input_tokens in the performance summarizer to prioritize existing service-returned values. The reviewer suggested refactoring the conditional logic to improve readability and reduce redundant dictionary lookups using a more idiomatic structure.
Comment on lines
+154
to
159
| if perf_data.get("input_tokens") is not None and perf_data.get("input_tokens") > 0: | ||
| pass # Prefer service-returned "prompt_tokens". | ||
| elif is_mm_prompt(perf_data["input"]): | ||
| perf_data["input_tokens"] = 0 # multi-modal input does not support input_tokens | ||
| elif "input_tokens" not in perf_data or perf_data.get("input_tokens") is None: | ||
| perf_data["input_tokens"] = len(tokenizer.encode(perf_data["input"])) # input_tokens is not provided, calculate it |
Contributor
There was a problem hiding this comment.
The logic for determining input_tokens can be simplified to improve readability and avoid redundant dictionary lookups. The current structure using a pass block followed by multiple elif conditions is less idiomatic. Additionally, the comment on line 155 refers to prompt_tokens while the code uses input_tokens, which might be slightly confusing for maintainers.
Suggested change
| if perf_data.get("input_tokens") is not None and perf_data.get("input_tokens") > 0: | |
| pass # Prefer service-returned "prompt_tokens". | |
| elif is_mm_prompt(perf_data["input"]): | |
| perf_data["input_tokens"] = 0 # multi-modal input does not support input_tokens | |
| elif "input_tokens" not in perf_data or perf_data.get("input_tokens") is None: | |
| perf_data["input_tokens"] = len(tokenizer.encode(perf_data["input"])) # input_tokens is not provided, calculate it | |
| input_tokens = perf_data.get("input_tokens") | |
| if input_tokens is None or input_tokens <= 0: | |
| if is_mm_prompt(perf_data["input"]): | |
| perf_data["input_tokens"] = 0 # multi-modal input does not support input_tokens | |
| elif input_tokens is None: | |
| # input_tokens is not provided, calculate it | |
| perf_data["input_tokens"] = len(tokenizer.encode(perf_data["input"])) | |
SJTUyh
approved these changes
May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for your contribution; we appreciate it a lot. The following instructions will make your pull request healthier and help you get feedback more easily. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.
感谢您的贡献,我们非常重视。以下说明将使您的拉取请求更健康,更易于获得反馈。如果您不理解某些项目,请不要担心,只需提交拉取请求并从维护人员那里寻求帮助即可。
PR Type / PR类型
Related Issue | 关联 Issue
Fixes #298
🔍 Motivation / 变更动机
见issue
📝 Modification / 修改内容
见issue
📐 Associated Test Results / 关联测试结果
Please provide links to the related test results, such as CI pipelines, test reports, etc.
请提供相关测试结果的链接,例如 CI 管道、测试报告等。
Does the modification introduce changes that break the backward compatibility of the downstream repositories? If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.
是否引入了会破坏下游存储库向后兼容性的更改?如果是,请描述它如何破坏兼容性,以及下游项目应该如何修改其代码以保持与此 PR 的兼容性。
If the modification introduces performance degradation, please describe the impact of the performance degradation and the expected performance improvement.
如果引入了性能下降,请描述性能下降的影响和预期的性能改进。
🌟 Use cases (Optional) / 使用案例(可选)
If this PR introduces a new feature, it is better to list some use cases here and update the documentation.
如果此拉取请求引入了新功能,最好在此处列出一些用例并更新文档。
✅ Checklist / 检查列表
Before PR:
After PR:
👥 Collaboration Info / 协作信息
🌟 Useful CI Command / 实用的CI命令
/gemini review/gemini summary/gemini help/readthedocs build