Skip to content

Handle Data Gym vocab files without trailing newline - #598

Open
tandede wants to merge 1 commit into
openai:mainfrom
tandede:fix-data-gym-final-merge
Open

Handle Data Gym vocab files without trailing newline#598
tandede wants to merge 1 commit into
openai:mainfrom
tandede:fix-data-gym-final-merge

Conversation

@tandede

@tandede tandede commented Aug 18, 2026

Copy link
Copy Markdown

Summary

  • parse Data Gym vocab.bpe files correctly when the final line has no trailing newline
  • add a regression test covering a final merge at end-of-file

Why

The parser previously split on \n and discarded the final list element unconditionally. That element is empty only when the file ends with a newline. For an otherwise valid file without one, the final BPE merge was silently dropped, causing the subsequent encoder consistency check to fail.

Using splitlines() preserves the final merge regardless of whether the file has a trailing newline while continuing to omit the terminal line boundary itself.

Validation

  • python -m pytest tests/test_load.py -q (1 passed)
  • python -m pytest tests --import-mode=append --hypothesis-seed=1 (34 passed)
  • cargo test (2 passed)
  • cargo fmt --check
  • ruff check --ignore PLC0206 tiktoken/load.py tests/test_load.py
  • ruff format --check tests/test_load.py

@tandede
tandede marked this pull request as ready for review August 19, 2026 11:38
@tandede
tandede force-pushed the fix-data-gym-final-merge branch from 92b419d to fa09801 Compare August 21, 2026 11:20
@tandede

tandede commented Aug 30, 2026

Copy link
Copy Markdown
Author

Hi maintainers, I’m following up on this PR in case it was missed. It addresses Data Gym vocabulary files that do not end with a trailing newline and is ready for review. I’d be happy to make any requested adjustments. Thank you!

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks right. splitlines()[1:] keeps the last merge whether or not the file ends in \n, while still dropping the header and not creating a fake empty merge for the normal trailing-newline case. The regression hits the actual failure by requiring a b to survive as rank 256. No issue from me.

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.

3 participants