fix(data): apply the null-content contract to legacy chat turns - #5742
Open
pruprakash wants to merge 1 commit into
Open
fix(data): apply the null-content contract to legacy chat turns#5742pruprakash wants to merge 1 commit into
pruprakash wants to merge 1 commit into
Conversation
normalize_chat_conversation converted null content to an empty string only on the OpenAI role/content branch. A legacy ShareGPT from/value turn with a null value kept content=None, and the chat template rendered the literal string None into the training text. Adds unit tests covering the change (red-green verified). Signed-off-by: Pruthviraj Prakash <pruprakash@nvidia.com>
pruprakash
added
bug
Contributor
|
LGTM — the null value to empty-string conversion for the legacy sharegpt schema correctly mirrors the existing OpenAI null-content handling, and the two new unit tests cover both the direct conversion and cross-schema equivalence. Suggested test cases
No perf tests impacted. |
Contributor
Author
|
/ok to test 82a8ee1 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do ?
Fixes ShareGPT chat SFT rows with a null
valuetraining the model on the literal stringNone, by applying the documented null-content contract to the legacyfrom/valuebranch ofnormalize_chat_conversation.Changelog
src/megatron/bridge/data/conversation_processing.py: legacyfrom/valuebranch converts a nullvalueto""; docstring widened to both schemas.tests/unit_tests/data/test_conversation_processing.py: 2 tests over the patched branch and null/empty equivalence.Additional Information
conversation_processing.py:606-611appliesNone->""thencontinues, so the legacy branch at:612-615copiedvalueintocontentunconverted. Jinja stringifiesNoneinstead of raising, so it reaches the training text silently.46c42552([4/4] fix(data): align thinking-model chat templates and masks #5541); the legacy branch (0845a7dd, fix(data): normalize ShareGPT role aliases #5078) never had it.nvcr.io/nvidian/nemo:nightlyon EOS at base4a3f2c2e. RED (job 5901189): 2 failed oncontent=Nonevs''. GREEN (job 5901199): 2 passed, file 110 passed.tests/unit_tests/data/796 -> 798 passed, same 2 pre-existing unrelated failures.raiseat:616also rejects an assistant turn withtool_callsand nocontentkey, making DeepSeek-V4 tool-call-only support unreachable frombuild_direct_hf_sft_split. Separate root cause, and the remedy is a maintainer call — reported separately, not fixed here.