fix(pretokenize): split_corpus lost every newline when given a file path - #10
Merged
Conversation
Reading a corpus from disk went through str.splitlines(), which drops line terminators, and through read_text, which rewrites \r\n to \n. The same text passed as a str or as an iterable of lines kept both. So a tokenizer trained from a file path never saw a line break, learned no merge spanning one, and could not represent indentation structure -- worst for the source-code corpora this project targets. It stayed invisible because split() itself is lossless on every input: the losslessness tests all passed while the corpus reaching the trainer was already damaged. Reads with newline="" and keepends=True, so a path, a literal str, and an iterable of lines now yield identical pre-tokens for identical text. Tests cover both, including CRLF, and assert the three input forms agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes split_corpus’s file-path handling so reading a corpus from disk preserves newline bytes and line terminators, making tokenization lossless and consistent with in-memory inputs (string / iterable-of-lines).
Changes:
- Read corpus files with
newline=""and split withsplitlines(keepends=True)to preserve\nand\r\n. - Expand
split_corpusdocstring to document the newline-loss bug and rationale. - Add tests covering LF/CRLF/no-trailing-newline/bare-newlines and asserting equivalence across input forms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/granule/pretokenize.py | Updates file-path reading to preserve newline bytes and keep line terminators when splitting. |
| tests/test_pretokenize_lossless.py | Adds regression tests for newline preservation and cross-input-form equivalence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
138
to
140
| When *corpus* is a ``str`` or :class:`~pathlib.Path` pointing to an | ||
| existing file it is read line by line. Otherwise a ``str`` is treated | ||
| as literal text and split directly. |
Comment on lines
+117
to
+120
| from_path = b"".join(split_corpus(corpus, pattern)) # type: ignore[arg-type] | ||
| from_lines = b"".join(split_corpus(text.splitlines(keepends=True), pattern)) # type: ignore[arg-type] | ||
| from_str = b"".join(split_corpus([text], pattern)) # type: ignore[arg-type] | ||
| assert from_path == from_lines == from_str == text.encode("utf-8") |
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
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.
The bug
split_corpusgiven a file path was not byte-lossless, in two ways that only affected that branch:str.splitlines()dropped every line terminator.read_textapplied universal-newline translation, rewriting\r\nto\n.The same text passed as a
stror as an iterable of lines kept both.A tokenizer trained through the file path never sees a line break, so it learns no merge spanning one and cannot represent indentation structure — worst for the source-code corpora this project targets.
Why it was invisible
splititself is lossless on every input, sotest_pretokenize_lossless.pypassed throughout while the corpus reaching the trainer was already damaged. The tests coveredsplit, neversplit_corpusfrom a path.The fix
Read with
newline=""and split withkeepends=True, so a path, a literalstr, and an iterable of lines produce identical pre-tokens for identical text. Callers wanting\r\nnormalized can do it themselves.New tests cover LF, CRLF, no-trailing-newline, and bare-newline corpora across all three patterns, plus an explicit assertion that the three input forms agree.
316 tests pass; ruff and mypy clean.