Uh oh!
There was an error while loading. Please reload this page.
fix(data): drop the tokenizer.json ingest requirement for MLM (#184) - #195
Merged
Conversation
masked_language_modeling required a tokenizer.json at the dataset root, staged it as an ExtraFile, and displayed it in the local summary. This was a false gate: the ingestor never read it, the ingest.v1 schema never required it, and the training side deliberately stopped staging a dataset tokenizer in #805 (it diverged the vocab and broke weight averaging). The tokenizer is the collaborator's, supplied at model upload. - DiscoverText: remove the MLM tokenizer branch. An MLM dataset with just the text layout is now accepted; a stray tokenizer.json is ignored, not an error (it is simply not walked). - LocalLayout.ExtraFiles was populated solely by this branch, so remove it (struct field, FileCount, the stream.go packaging loop, and the data.go local-summary display) rather than leave dead, always-empty plumbing. - Fix the now-false "the ingestor reads it" doc comments. - Tests: replace TestDiscoverText_MLM_RequiresTokenizer with TestDiscoverText_MLM_NoTokenizer (accepted without; stray one ignored). RFC-0002 §12.11. CLI-only, low risk. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
saadqbal
approved these changes
Jul 9, 2026
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 freeto 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.
Summary
masked_language_modelingingestion required atokenizer.jsonat the dataset root, staged it, and showed it in the local summary. This was a false gate: the ingestor never read it, theingest.v1schema never required it, and the training side deliberately stopped staging a dataset tokenizer in #805 (a dataset-staged tokenizer diverged the vocab and broke weight averaging). The tokenizer is the collaborator's, supplied at model upload.Closes#184. Design-of-record: RFC-0002 §12.11 (#174).
What changed
internal/push/text.go—DiscoverTextno longer requires/stagestokenizer.jsonfor MLM. An MLM dataset with justlabels.csv+sequences/*.txtis accepted. Atokenizer.jsonleft in the directory is simply not walked, so it is ignored gracefully (never an error). Fixed the false "the ingestor reads it" doc comment.internal/push/walk.go— removedLocalLayout.ExtraFiles(struct field +FileCount) and its stale comments.internal/push/stream.go— removed the now-unreachableExtraFilespackaging loop; fixed thesortedKeyscomment.internal/cli/data.go— removed the deadtokenizerrow from the local-summary panel.internal/push/text_test.go— replacedTestDiscoverText_MLM_RequiresTokenizerwithTestDiscoverText_MLM_NoTokenizer(accepted without a tokenizer; a stray one is ignored, not counted, not an error). Dropped the deadExtraFilesassertion in the classification test.Design decision for the reviewer to ratify
ExtraFiles(a generic "single root-level file beyond labels.csv" staging map onLocalLayout) was populated solely by the MLM tokenizer branch — no other category used it, and its only documented example was the tokenizer. Rather than leave an always-empty map iterated in the stream hot path plus a dead summary branch, I removed the whole mechanism. If a future category needs root-level extras, re-adding it is trivial. If you would rather keep it as an extension point, I can instead keep the field and only drop the tokenizer branch — say the word.Out of scope / deferred: RFC-0002's own tables (
docs/rfcs/0002-data-ingest-flow.mdlines ~90/147/341/407) still describe the old "tokenizer.json required" behavior. I left the RFC untouched because it is under active revision on its own branch (docs/rfc-0002-data-ingest-flow); those tables should be reconciled there to avoid a merge collision. Also note: whether MLM should still requirelabels.csvat all (the RFC says MLM has "no labels") is a separate question, not touched here.Test plan
🤖 Generated with Claude Code
Note
Low Risk
Behavior change relaxes a false CLI gate and stops uploading an unused file; no auth or cluster security paths are touched, and existing MLM layouts without tokenizer.json become valid.
Overview
Masked language modeling no longer requires or uploads
tokenizer.jsonat the dataset root.DiscoverTextonly validateslabels.csvplus the text sidecar (texts/orsequences/); a leftovertokenizer.jsonin the folder is ignored (not staged, not counted, not an error), matching ingest behavior after #805.The
LocalLayout.ExtraFilesmechanism is removed end-to-end: struct field andFileCountinwalk.go, the tar packaging loop instream.go, and the tokenizer row in the ingest local summary (data.go). Tests now assert MLM works without a tokenizer and that stray tokenizers do not changeFileCount.Reviewed by Cursor Bugbot for commit f131a67. Bugbot is set up for automated code reviews on this repo. Configure here.