-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: XML adapter's markers #8876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes issues with the XML adapter where it was mixing XML tags with ChatAdapter's bracketed completion markers, resulting in confusing prompts and incorrect model outputs.
- Overrides XMLAdapter methods to ensure consistent XML-only formatting throughout the prompt
- Removes the
<completed>
tag from completion requirements - Adds comprehensive tests to verify XML-only formatting behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
dspy/adapters/xml_adapter.py | Overrides format methods to use XML tags exclusively and removes completion markers |
tests/adapters/test_xml_adapter.py | Adds tests to verify XML input formatting and complete prompt structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Thanks @enitrat. I left some comments, can you take a look? |
@TomeHirata Do you agree that the edit: after further reading about the purpose of a completion marker, it seems that it's only useful for the The
As such, a |
Hey @TomeHirata, thanks for the answer. I've properly applied the suggestions and restricted the scope of the changes, and updated the PR summary to match the current changes implemented in this PR. Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can you take a look at my comment?
@TomeHirata should be all good now! |
Summary
Issue with the XMLAdapter
[[ ## completed ## ]]
) behavior, leading to prompts that mixed XML tags with ChatAdapter markers. This confused models and produced[[ ## completed ## ]]
instead of the expected XML-only output (Closes [Bug] XMLAdapter inserting wrong markers #8875)Cause of the issue
<answer>...</answer>
), but it only overrode parts of the inherited ChatAdapter class:format_field_with_value
(to XML), andparse
.format_field_structure
(system structure example) added[[ ## completed ## ]]
in ChatAdapter.format_assistant_message_content
appended[[ ## completed ## ]]
in ChatAdapter<completed>
tag instruction inuser_message_output_requirements
.As a result, the final prompt contained the ChatAdapter marker
[[ ## completed ## ]]
in both the system message structure example and the assistant message output.How I fixed it
Ensure XMLAdapter formats output fields and structure examples with XML tags only (no ChatAdapter markers).
Changes:
format_field_structure
(dspy/adapters/xml_adapter.py:24
) to format the system message structure example using XML tags only.format_assistant_message_content
(dspy/adapters/xml_adapter.py:44
) to format assistant messages with XML tags only (no[[ ## completed ## ]]
marker).<completed>
tag instruction fromuser_message_output_requirements
(dspy/adapters/xml_adapter.py:57
).[[ ## field ## ]]
format for now, as per discussions with maintainers.Tests added:
test_xml_adapter_full_prompt
asserts on the full prompt generated, ensuring there's no unwanted[[ ## completed ## ]]
markers in system or assistant messages, and no<completed>
tag instruction.<completed>
tag from mock responses.