Skip to content

fix(template): replace rejected responses after the last user message - #10127

Open
Excelius-Wang wants to merge 3 commits into
modelscope:mainfrom
Excelius-Wang:fix/rejected-tool-round
Open

Excelius-Wang wants to merge 3 commits into
modelscope:mainfrom
Excelius-Wang:fix/rejected-tool-round

Conversation

@Excelius-Wang

Copy link
Copy Markdown
Contributor

Agent preference data using tool messages currently inherits chosen tool calls and results when building rejected_messages from rejected_response. Keep the documented replacement boundary at the last user message by adding an opt-out for tool boundaries to the existing helper. Its default behavior, including the last-round loss fix in #9923, remains unchanged.

Validation: 10 tests and 12 subtests passed, including an actual JSONL/OpenAI tool-call loading regression and the existing loss-scale tests. Local CPU encoding checks covered 48 official-example variants and 192 cases from three public preference datasets; all match explicit rejected-message references after the fix. Six two-step tiny-model DPO runs produced identical losses and trained weights for compact versus explicit representations on each dataset. Changed-file pre-commit checks and git diff --check passed.

assert len(messages) > 0, f'messages: {messages}'
idx = get_last_user_round(messages) + 1
# Replace the entire response trajectory, including tool calls and results.
idx = get_last_user_round(messages, include_tool=False) + 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The documentation need to change.
to declare that, the rejected_response needs to contain each rejected round after the last user field, including tool round, else user will not know to add negative tool rounds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the Chinese and English docs and added an example showing how to include tool calls and results in rejected_response. Thanks for pointing this out!

This branch has not been deployed

No deployments
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