Skip to content

Keep all turns and fix system prompt in ShareGPT conversations parsing#345

Merged
yghstill merged 1 commit into
Tencent:mainfrom
SuperMarioYL:fix/sharegpt-conversations-multiturn
Jun 20, 2026
Merged

Keep all turns and fix system prompt in ShareGPT conversations parsing#345
yghstill merged 1 commit into
Tencent:mainfrom
SuperMarioYL:fix/sharegpt-conversations-multiturn

Conversation

@SuperMarioYL

Copy link
Copy Markdown
Contributor

What

Fix ShareGPT conversations handling in TextDataset._prepare_messages
(angelslim/data/text_dataset.py). The branch had two correctness bugs:

  1. KeyError: 'system_prompt' — it guarded on data["system"] but then
    read data["system_prompt"]:

    if "system" in data and data["system"]:
        messages = [{"role": "system", "content": data["system_prompt"]}] + messages

    Any ShareGPT record that carries a system field crashes during data
    loading. (The sibling messages and input/output branches correctly
    guard and read system_prompt; only this branch was inconsistent.)

  2. Multi-turn conversations were truncated to the first two turns and roles
    were assigned positionally:

    share_gpt_data = data["conversations"]
    messages = [
        {"role": "user", "content": share_gpt_data[0]["value"]},
        {"role": "assistant", "content": share_gpt_data[1]["value"]},
    ]

    A standard multi-turn ShareGPT record ([{"from": "human", ...}, {"from": "gpt", ...}, {"from": "human", ...}, ...]) lost every turn after the second,
    and the actual from of each turn was ignored. This defeats the downstream
    loop, which already scans for the last assistant turn and treats everything
    before it as prompt context — i.e. the pipeline is built for multi-turn data.

How

  • Iterate over all turns, carrying each turn's role/content through the
    existing role-normalization loop (which already maps fromrole,
    valuecontent, and humanuser / gptassistant).
  • Prepend the optional dataset-level system prompt (data["system"], with
    data["system_prompt"] accepted as a fallback), deduplicated against a
    conversation that already begins with a system turn so it is not doubled.
  • Skip turns that carry no text payload, and harden the normalization loop so a
    turn missing a role no longer raises. The messages and input/output
    branches are left behavior-identical (the .get is a no-op for them).

Tests

tests/test_text_dataset_messages.py — CPU-only, no GPU / model weights / heavy
stack (the torch/transformers/datasets/pyarrow imports are stubbed;
_prepare_messages is pure-dict). Covers: system-field no-crash regression,
all-turns-preserved, leading-system-turn dedup, malformed-turn skip, and the
unchanged messages / input-output branches. The four conversations cases
fail on main and pass here; the two non-conversations cases pass on both.

Note on reuse

angelslim/compressor/speculative/train/data/data_utils.py already has a
convert_sharegpt_data helper with the same humanuser / gptassistant
mapping. It is intentionally not reused here because it lives in the
speculative.train.data subpackage and pulls transformers.image_utils and
training-only deps at import — importing it into the core data loader would add
a heavier cross-package dependency than the few lines here. It also indexes
role_mapping[message["from"]] directly, which raises on a system turn,
whereas the normalization in text_dataset.py handles system turns.

Reviewer check: does keeping all turns + prepending the system prompt match
the intended ShareGPT calibration format, or should the loader continue to
support only single-turn conversations? The downstream _load_data loop
(scanning for the last assistant turn) suggests multi-turn is intended.

TextDataset._prepare_messages only read the first two entries of a
ShareGPT-style "conversations" record and assigned roles positionally,
silently dropping every later turn of a multi-turn conversation and
mislabeling system-led ones. The branch also guarded on data["system"]
but read data["system_prompt"], raising KeyError on any record that
carries a system field.

Iterate over all turns, carry each turn's role/content through the
existing normalization, prepend the optional dataset-level system prompt
(deduplicated against a leading system turn), and skip turns with no text
payload. Harden the normalization loop so a turn missing a role no longer
raises. Add CPU-only unit tests covering the multi-turn, system-field,
dedup and malformed-turn cases plus the unchanged messages/input-output
branches.
@yghstill yghstill merged commit abf1d50 into Tencent:main Jun 20, 2026
4 checks passed
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