Repository navigation
Point stale scripts/train.py references at the extracted training modules - #179
Merged
Merged
Conversation
The predicate: no reference falsely claims the training loop, data pipeline, metrics wiring or checkpoint fence lives in scripts/train.py, and no citation points at a line number inside it. References that are still true are left alone -- the ~93 invocation lines, the CLI facts (positional TOML, load_config, --section.key overrides), the negative "not used by scripts/train.py" claims, and the CHANGELOG entries, which are historical statements about this change.
Two docs sections claimed the training loop does not pass the dataloader
to ckpt_mgr.save(), with code blocks showing "# no dataloader=...". That
is false, and was already false on main: the loop passes it at all four
save sites. A SIGTERM drill confirms it end to end -- the emergency
train state carries {'epoch': 7, 'batches_yielded': 14, 'sampler': {...}}
and the run resumes at epoch=7, skip_batches=14, mid-epoch at the exact
sample boundary.
Repointing these blocks at training/loop.py without touching the prose
would have aimed the reader straight at the file that refutes it, so the
sections are replaced rather than relabelled. Also fixes a sentence this
branch had broken mid-replacement and aligns the extra-dict snippet with
checkpoint_extra's actual variables.
… docs/train-entry-pointers
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
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.
Summary
Docs-and-comments only. Stacked on #178 — base is
refactor/train-entry-point, notmain.That PR moved the training loop, data pipeline, metrics wiring and checkpoint fence out of
scripts/train.pyintokempnerforge/training/. It updated the docs that describe those subsystems directly; this one sweeps the rest of the repo so nothing still tells a reader — or an agent skill — to look inscripts/train.pyfor code that is no longer there.The predicate, which is grep-checkable:
build_data_pipeline,build_phase_state,build_model,restore_checkpoint,run_training,run_training_loop,setup_distributed,freeze_meta_at_step.# scripts/train.pyabove a snippet) relabelled to the module the snippet now lives in — under a strict reading these were the most misleading, since they name a file the code is not in.scripts/train.py:85,line ~788,around line 508. Line numbers into a file this PR stack rewrote are guaranteed to rot.explain-architecture/SKILL.md, which told a reader the loop is inscripts/train.pyand to stop looking for anything else.scripts/train.py.One correction beyond pointers
Two sections claimed the training loop "does not currently pass the dataloader" to
ckpt_mgr.save(), with code blocks showing# no dataloader=...(docs/checkpointing/train-state.md,docs/data/stateful-dataloader.md). That is false, and was already false onorigin/main— pre-existing rot, not a regression from the base PR. The loop passesdataloader=at all four save sites, and a SIGTERM drill on the base branch confirms it end to end: the emergency train state carried{'epoch': 7, 'batches_yielded': 14, 'sampler': {...}}and the run resumed atepoch=7, skip_batches=14— mid-epoch, at the exact sample boundary.They are replaced rather than relabelled. Repointing the code blocks at
training/loop.pywould have satisfied the predicate while aiming the reader straight at the file where three lines of grep refute the surrounding prose.What is deliberately left alone
uv run python scripts/train.py <config.toml>is still exactly how you train; the CLI is unchanged. Verified by diffing the sorted invocation lines againstorigin/main, not just counting them — a compensating edit could hold the count steady while rewriting a line. They are byte-identical.--section.key=valueoverrides,load_config, "prints the exact--dataflags forscripts/train.py".scripts/train.py") — still true.CHANGELOG.md— its entries are historical statements about what each change did at the time, and rewriting them would falsify the record.Testing
Docs, comments and one test comment; no behavior touched.
uv run ruff check/ruff format --checkpass on the touched Python filesuv run sphinx-build -W --keep-going -b html docs docs/_build/html— build succeededorigin/main(93 of them);CHANGELOG.mdabsent from the diff