Skip to content

Conversation

@CharlieFRuan
Copy link
Collaborator

@CharlieFRuan CharlieFRuan commented Aug 31, 2025

Prior to this PR, in the re-tokenize chat history codepath, we did not use self.custom_chat_template in some places (only used it in the final tokenization). Though currently only Qwen3 enters this codepath, which would not be affected, it is still good to apply the custom_chat_template for consistency.

@CharlieFRuan CharlieFRuan changed the title [Fix][Generator] Add chat_template in each step retokenization [Fix][Generator] Use custom_chat_template in each step retokenization Aug 31, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly applies the custom_chat_template in the re-tokenization step for consistency. While the change is good in isolation, it highlights a critical bug in the agent_loop logic. The initial prompt length is calculated using the default chat template, but the token IDs from which the prompt is later extracted are generated using the custom chat template. This mismatch will lead to incorrect prompt data. I've added a critical review comment with a detailed explanation of the issue and a suggested fix.

@CharlieFRuan
Copy link
Collaborator Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses an inconsistency by ensuring custom_chat_template is used during the re-tokenization of chat history. The changes in agent_loop and _get_next_input_ids_by_retokenizing_chat_history correctly apply the custom template when needed. The fix is sound and improves code consistency. I have provided one suggestion to enhance the code style in agent_loop.

@SumanthRH SumanthRH self-assigned this Sep 2, 2025
@SumanthRH SumanthRH merged commit 9c592e9 into NovaSky-AI:main Sep 2, 2025
3 checks passed
@CharlieFRuan CharlieFRuan deleted the fix-0831-custom-chat branch September 8, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants