fix(dataprep): don't emit a degenerate chunk for empty text#7183
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where empty or whitespace-only text inputs would result in degenerate samples (such as lone-EOS documents or zero-length input IDs) that could break downstream trainers. It fixes this by returning an empty list directly when the token count is zero in smart_chunk_text, and adds a corresponding unit test to verify this behavior. The reviewer feedback suggests removing redundant trailing commas in single-line function signatures in the new test to comply with PEP 8, and expanding the test coverage to assert behavior for both return_tokenized=True and return_tokenized=False modes.
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.
| def __call__( | ||
| self, | ||
| text, | ||
| return_tensors = None, | ||
| add_special_tokens = False, | ||
| ): | ||
| token_ids = list(range(len(text.split()))) | ||
| if return_tensors == "pt": |
There was a problem hiding this comment.
According to PEP 8, trailing commas in single-line function signatures are redundant and should be avoided. They are typically only used when wrapping arguments/parameters across multiple lines.
| def __call__( | |
| self, | |
| text, | |
| return_tensors = None, | |
| add_special_tokens = False, | |
| ): | |
| token_ids = list(range(len(text.split()))) | |
| if return_tensors == "pt": | |
| def __call__(self, text, return_tensors = None, add_special_tokens = False): | |
| token_ids = list(range(len(text.split()))) | |
| if return_tensors == "pt": | |
| return {"input_ids": [token_ids]} | |
| return {"input_ids": token_ids} | |
| def decode(self, token_ids, skip_special_tokens = False): | |
| return " ".join(f"word_{i}" for i in token_ids) |
References
- PEP 8: Trailing commas in single-line function signatures are redundant and should be avoided. (link)
There was a problem hiding this comment.
Leaving this one as is. The repo formatter run by pre-commit.ci expands these signatures back to multi-line with a trailing comma (it just did so on the latest commit), so single-line signatures get auto-reverted. Keeping the enforced house style.
| token_ids, | ||
| skip_special_tokens = False, | ||
| ): | ||
| return " ".join(f"word_{i}" for i in token_ids) | ||
|
|
||
| for eos_token_id in (2, None): | ||
| loader = RawTextDataLoader(MockTokenizer(eos_token_id), chunk_size = 2048, stride = 512) | ||
| for text in ("", " \n\t "): |
There was a problem hiding this comment.
To ensure comprehensive test coverage, we should test both return_tokenized=True and return_tokenized=False modes for empty/whitespace-only inputs in both smart_chunk_text and chunk_text.
| token_ids, | |
| skip_special_tokens = False, | |
| ): | |
| return " ".join(f"word_{i}" for i in token_ids) | |
| for eos_token_id in (2, None): | |
| loader = RawTextDataLoader(MockTokenizer(eos_token_id), chunk_size = 2048, stride = 512) | |
| for text in ("", " \n\t "): | |
| for text in ("", " \n\t "): | |
| for return_tokenized in (True, False): | |
| assert loader.smart_chunk_text( | |
| text, chunk_size = 2048, stride = 512, return_tokenized = return_tokenized | |
| ) == [], f"empty input should yield no chunks (eos={eos_token_id}, text={text!r}, return_tokenized={return_tokenized})" | |
| assert loader.chunk_text(text, return_tokenized = return_tokenized) == [], ( | |
| f"chunk_text should yield no chunks for empty input " | |
| f"(eos={eos_token_id}, text={text!r}, return_tokenized={return_tokenized})" | |
| ) |
There was a problem hiding this comment.
Added. The test now loops over return_tokenized in (True, False) for both smart_chunk_text and chunk_text, and uses a whitespace-preserving fake tokenizer so it actually exercises the pre-tokenization guard.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3850faa3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Tokenizer returned a count; build a range | ||
| tokens = list(range(tokens)) | ||
|
|
||
| if len(tokens) == 0: |
There was a problem hiding this comment.
Reject whitespace before tokenization
For tokenizers that preserve whitespace as real tokens, such as byte-level BPE or SentencePiece tokenizers used by many HF models, whitespace-only input like " \n\t" can produce len(tokens) > 0, so this guard is skipped and chunk_text/load_from_files still emit the whitespace/EOS sample this change is trying to suppress. Please base the empty/whitespace check on the raw text, e.g. not text.strip(), before relying on token length.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Moved the guard to a raw-text check (not text or not text.strip()) before tokenization. Byte-level BPE and SentencePiece emit real tokens for spaces, newlines and tabs, so the len(tokens)==0 check only caught the truly empty string. Verified whitespace-only input now yields no chunks for both the gpt2 and llama tokenizers.
| # rather than a degenerate sample (a lone-EOS "document", or a | ||
| # zero-length ``input_ids`` when the tokenizer has no eos_token_id, | ||
| # which would break a downstream collator/trainer). | ||
| return [] |
There was a problem hiding this comment.
Preserve tokenized schema for skipped files
When every file passed to load_from_files(..., return_tokenized=True) tokenizes to zero tokens, this new return [] leaves all_chunks empty, and create_causal_dataset([]) falls through to the legacy text path because it cannot inspect chunks[0], producing an empty dataset with only a text column. That violates the requested tokenized mode and can break downstream consumers that expect input_ids/attention_mask/labels; return an empty tokenized dataset or raise when all files are skipped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. load_from_files now raises ValueError when every file is empty or whitespace-only, matching load_from_file, so return_tokenized mode never falls through to create_causal_dataset([]) and a text-only column. A mix of valid and empty files still returns only the valid rows with the tokenized schema.
ErenAta16
left a comment
There was a problem hiding this comment.
Clean, well-placed guard, added right after tokenization and before the single-chunk branch, so empty/whitespace-only text can't reach the code that would otherwise emit a degenerate lone-EOS chunk (or a zero-length input_ids when there's no eos_token_id). Test covers both eos_token_id set and None, and both a truly empty string and a whitespace-only one, through both smart_chunk_text and chunk_text. Low risk, small, correct. LGTM.
smart_chunk_text feeds empty / whitespace-only text (which tokenizes to zero tokens) into the single-chunk branch, which unconditionally returns one chunk. That yields a lone-EOS "document" (input_ids=[eos]) or, when the tokenizer has no eos_token_id, a zero-length input_ids=[] — an invalid sample that breaks a downstream collator/trainer. load_from_file already guards against this with a ValueError, but chunk_text, smart_chunk_text and load_from_files do not, so batch-loading a directory that contains an empty file silently injects garbage rows. Return no chunks when the tokenized text is empty, so empty inputs contribute nothing instead of a degenerate sample. load_from_file keeps its explicit ValueError (its guard runs first). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
8afb4eb to
d1c7dfb
Compare
Real BPE/SentencePiece tokenizers emit tokens for spaces and newlines, so the len(tokens)==0 check let whitespace-only documents through as a degenerate lone-EOS sample. Guard on text.strip() before tokenizing (mirroring load_from_file), and raise in load_from_files when every file is empty so return_tokenized mode never falls back to a text-column dataset. Test now uses a whitespace-preserving tokenizer and covers both return_tokenized modes.
72b1c79 to
2924cff
Compare
for more information, see https://pre-commit.ci
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
My approval on this missed the whitespace hole, and I want to name why rather than let it slide, since it's the second time this week I've made the same reviewing mistake. I wrote that the test "covers ... both a truly empty string and a whitespace-only one." It did pass a whitespace string, but the fake tokenizer at the time was token_ids = list(range(len(text.split())))and Same failure mode as a review I got wrong on huggingface/peft#3442 two days ago: I checked the production logic carefully, then took the test at face value instead of asking whether it could fail. A test whose fixture is written alongside the fix will tend to encode the fix's assumptions, so the question worth asking is not "does it pass" but "what would have to be true for this to fail". Codex asked that here and I didn't. The new version closes it properly, and the reason is this line: assert len(loader.tokenizer(" \n\t ")["input_ids"]) > 0That's the part that makes the rest non-vacuous. It pins the fake tokenizer as whitespace-preserving, so the subsequent Moving the guard before tokenization is also strictly better than fixing the token check, since it avoids tokenizing input that's going to be discarded. One small thing on loader.load_from_files([]) # ValueError: All files are empty or contain only whitespaceThat message is misleading for that input, and if not file_paths:
raise ValueError("No files provided")
if not all_chunks:
raise ValueError("All files are empty or contain only whitespace")Either way the exception type is unchanged, so it's purely about whether the message points at the right cause. |
Problem
RawTextDataLoader.smart_chunk_textsends empty / whitespace-only text (which tokenizes to zero tokens) into the single-chunk branch, which unconditionally emits one chunk:So an empty document becomes either a lone-EOS "sample" or, when the tokenizer has no
eos_token_id, a zero-lengthinput_ids— an invalid training example that will crash / NaN a downstream collator or trainer.load_from_filealready guards against this:but
chunk_text,smart_chunk_text, andload_from_filesdo not — so batch-loading a directory that happens to contain an empty/whitespace file silently injects garbage rows (load_from_files([good, empty])returns more rows thangoodalone).Fix
Return no chunks when the tokenized text is empty, so empty inputs contribute nothing instead of a degenerate sample.
load_from_filekeeps its explicitValueError(its guard runs beforesmart_chunk_text), and the multi-chunk path is untouched.Test plan
Added
test_smart_chunk_text_empty_input_returns_no_chunkstotests/test_raw_text.py, coveringsmart_chunk_textandchunk_texton""and whitespace, both with and without aneos_token_id.mainthe new test fails (empty input returns[{'input_ids': [2], ...}]); with this change it returns[].pytest tests/test_raw_text.py→ 3 passed (existing single/multi-chunk + no-eos tests still green).ruff checkclean (repo rule set).This follows up #7151, which fixed the single-chunk EOS-none tensor leak in the same function.
Authored with assistance from Claude Code (Anthropic); the bug, fix, and test were reviewed and verified locally by the contributor.