fix(messages): preserve non-string tool-call args in Hermes ShareGPT round-trip - #4170
Conversation
…round-trip
HermesFunctionFormatter serialized tool calls with Python repr
(f"{tool_call_dict}") and parsed them back with
json.loads(text.replace("'", '"')). That blanket quote swap corrupts any
argument string containing an apostrophe (repr wraps such a value in double
quotes, so the inner ' becomes a stray ") and True/False/None are not valid
JSON, so json.loads raised, the tool call was dropped, and from_sharegpt
silently degraded the message to a plain BaseMessage with args lost. All-string
args round-trip fine, which is why the existing test missed this.
Emit tool calls and responses as JSON (json.dumps) and parse them as JSON,
falling back to ast.literal_eval for legacy single-quoted payloads so older
serialized data keeps round-tripping. Add regression tests for args with
apostrophes, booleans, None and floats, plus the legacy repr parse path.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
thanks @ebarkhordar for opening the issue and pr! lgtm |
|
Thanks for the quick turnaround on both the issue and the PR. Glad the JSON round-trip fix was useful; I'll keep an eye on the ShareGPT conversion path. |
Related Issue
Closes #4169
Description
HermesFunctionFormatterserialized tool calls by interpolating the dict into an f-string (Pythonrepr) and parsed them back withjson.loads(text.replace("'", '"')). That blanket quote swap corrupts any argument string containing an apostrophe (repr wraps such a value in double quotes, so the inner'becomes a stray"), andTrue/False/Noneare not valid JSON.json.loadsraised, the tool call was dropped, andfrom_sharegptsilently degraded the message to a plainBaseMessagewithargslost. All-string args round-trip fine, which is why the existing test missed it.Invariant:
to_sharegpt()->from_sharegpt()preservesfunc_nameandargsfor any JSON-representable arguments.Fix (single file):
format_tool_call/format_tool_responseemitjson.dumps(..., ensure_ascii=False)instead of interpolating the dict.extract_tool_calls/extract_tool_responseparse withjson.loads, falling back toast.literal_evalfor legacy single-quoted (Python-repr) payloads so previously serialized data keeps round-tripping.No dependency changes (
astandjsonare stdlib).What is the purpose of this pull request?
Verification
Verified in a clean
python:3.12-slimcontainer. The two new tests (test_hermes_sharegpt_roundtrip_preserves_non_string_args,test_hermes_extract_tool_calls_parses_legacy_repr_payload) fail onmasterand pass on this branch; the existing Hermes/ShareGPT tests stay green.On
master(source unchanged, new tests added):On this branch:
The one unrelated failure in
test/messages/test_func_message.py(test_roleplay_conversion_with_tools) is a missingOPENAI_API_KEYin the sandbox, not affected by this change.Checklist
uv lockneeded