Skip to content

[data] fix KeyError on invalid role tag in ranking dataset converters#10601

Open
he-yufeng wants to merge 2 commits into
hiyouga:mainfrom
he-yufeng:fix/ranking-invalid-role-keyerror
Open

[data] fix KeyError on invalid role tag in ranking dataset converters#10601
he-yufeng wants to merge 2 commits into
hiyouga:mainfrom
he-yufeng:fix/ranking-invalid-role-keyerror

Conversation

@he-yufeng

Copy link
Copy Markdown

What does this PR do?

OpenAIDatasetConverter and SharegptDatasetConverter handle a pairwise (ranking) example whose chosen/rejected role tag is not an accepted tag by logging Invalid role tag and setting broken_data = True. But the if broken_data: skip guard is evaluated at the top of the if/elif chain, so flipping the flag inside the ranking branch is too late — the code then indexes tag_mapping[chosen[role_tag]] and raises KeyError, which crashes align_dataset() (dataset.map) instead of skipping the bad example as intended.

This skips such examples (prompt, response = []) — exactly what the top guard does for the cases it catches — so a misconfigured/edge ranking example is dropped with a warning rather than crashing data preparation. Both converters share the identical branch and are fixed together.

Before / after

# invalid role in a ranking example
example = {"conversations": [{"from": "human", "value": "..."}],
           "chosen": {"from": "not_a_role", "value": "..."},
           "rejected": {"from": "gpt", "value": "..."}}
# before: KeyError: 'not_a_role'   (crashes align_dataset)
# after:  {"_prompt": [], "_response": [], ...}   (skipped)

Verification

PYTHONPATH=src pytest tests/data/test_converter.py — added test_sharegpt_converter_ranking_skips_invalid_role; it fails with KeyError on the old code and passes after this change (3 passed total). ruff check and ruff format --check clean.

The OpenAI and ShareGPT dataset converters log "Invalid role tag" and set
broken_data=True for a pairwise example whose chosen/rejected role is not an
accepted tag. But the `if broken_data:` skip guard is evaluated at the top of the
if/elif chain, so setting it inside the ranking branch is too late: the code then
indexes tag_mapping[invalid_role] and raises KeyError, crashing align_dataset
instead of skipping the example. Skip such examples (prompt/response = []) as the
guard intends; add a converter test for the ranking invalid-role path.

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>

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

Copy link
Copy Markdown
Contributor

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 updates the dataset converter to safely handle invalid role tags in pairwise (ranking) datasets by skipping them (setting prompt and response to empty lists) instead of setting a flag and continuing, which previously caused crashes. A corresponding unit test has been added to verify this behavior. The reviewer pointed out a potential issue in OpenAIDatasetConverter where raw role tags are compared directly against standardized roles in accept_tags[-1], which will fail for custom role tags, and suggested using tag_mapping.get() to resolve this.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@@ -319,19 +319,19 @@ def __call__(self, example: dict[str, Any]) -> dict[str, Any]:
or rejected[self.dataset_attr.role_tag] not in accept_tags[-1]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In OpenAIDatasetConverter, accept_tags contains standardized roles (Role.ASSISTANT.value, Role.FUNCTION.value), whereas chosen and rejected contain raw role tags from the dataset (e.g., "gpt"). Comparing the raw role tags directly against accept_tags[-1] will always fail if custom role tags are used, causing valid examples to be incorrectly skipped as having an invalid role tag.

To fix this, we should look up the raw role tags in tag_mapping using .get() before comparing them to accept_tags[-1].

            if (
                tag_mapping.get(chosen[self.dataset_attr.role_tag]) not in accept_tags[-1]
                or tag_mapping.get(rejected[self.dataset_attr.role_tag]) not in accept_tags[-1]
            ):

The ranking-branch fix in the previous commit (`tag_mapping[chosen/rejected]`)
covered pairwise examples, but the OpenAI converter's main message loop had the
same gap: `tag_mapping[role]` on line 280 (now 284) is reached before the
validation loop at line 288 (now 292) has a chance to flag the bad role, so a
dataset entry with any unrecognised role tag in its message list raises
`KeyError` instead of being gracefully skipped.

Add an explicit `role not in tag_mapping` guard (matching the pattern already
used in the Sharegpt converter) that logs a warning and sets `broken_data`,
then extend the test suite with a case that drove the crash to confirm
RED → GREEN.

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
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.

1 participant