docs(autotrain): step-scaling loss-trend check (64/128 steps), surfaces last_loss noise - #1123
docs(autotrain): step-scaling loss-trend check (64/128 steps), surfaces last_loss noise#1123Tyler-R-Kendrick wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR extends the autotrain fixture step sweep through 64 and 128 steps, records token-loss proxy and final-loss metrics, and adds reproducible measured-results documents for both smoke runs. ChangesAutotrain step-scaling documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Tyler-R-Kendrick
left a comment
Tyler-R-Kendrick
left a comment
There was a problem hiding this comment.
Content is plausible and honest: example_token_loss_proxy (first_20_mean/last_20_mean/count) is a real train_summary field (harnesses/model_build/train_loop.py), count scales correctly with steps (250@64, 500@128), identical first_20_mean across runs is expected at fixed seed, the 64-step last_loss=3.92 continues the monotonic trend from #1120 (8.39@32), and the 128-step last_loss uptick is candidly explained as single-minibatch noise with the smoothed proxy still falling. Fixture_or_scratch labeling is correct, no ship claims, versions.json untouched. verify_version_stamps --check and repo_policy both pass on the head SHA. Two issues before merge: (1) CONFLICTING with main — #1120 (steps 4/16/32) already merged into the same tail of autotrain-loop-ledger-20260725.md, so this needs a rebase appending the 64/128 section after the #1120 batch (complementary, not superseded — extends the sweep and adds the smoothed-proxy takeaway); (2) minor: the 8-step row in the new comparison table is missing its wall_s cell, breaking the table's column count. Please rebase and fix the row, then this is mergeable.
…e row Addresses review on #1123 (Tyler-R-Kendrick): this batch's ledger section was based on a stale main tip and clobbered the already-merged batch #4 (#1120, steps 4/16/32) and joint seed x steps sweep (#1132) sections. Rebases onto main HEAD e0eaeec, appending the 64/128-step extended-range check after both instead of overwriting them -- complementary, not superseded, as those two already conclude the fixture's step/seed variance is otherwise thoroughly covered. Also fixes the missing wall_s cell on the 8-step reference row in the step-scaling comparison table (was breaking the table's column count). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDeTk7RxBAbWX5fKjBqb4r
aefbe0c to
d62f569
Compare
Tyler-R-Kendrick
left a comment
Tyler-R-Kendrick
left a comment
There was a problem hiding this comment.
Thanks for catching both — pushed d62f569:
- Rebased onto
mainHEADe0eaeecc(includes #1120 and #1132). My section now appends after both instead of overwriting them — still complementary as you said: extends the step range to 64/128 and adds the smoothed-proxy observation neither of those covered. - Fixed the 8-step reference row's missing
wall_scell (used the actual range acrossiter1008-iter1022rather than fabricating a single number).
verify_version_stamps --check and repo_policy both pass on the new head SHA.
Generated by Claude Code
…again main advanced past the previous rebase (e0eaeec) while addressing review on #1123 -- PR #1130 (batch-size variation 1/2/8) merged in the meantime, touching the same ledger tail. Re-rebases onto main HEAD 2f2d6b4 so this appends after batch-size too instead of clobbering it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDeTk7RxBAbWX5fKjBqb4r
d62f569 to
88561f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/autotrain-loop-ledger-20260725.md`:
- Around line 418-427: Update the Result text to attribute the monotonic
decrease explicitly to example_token_loss_proxy.last_20_mean, restricting the
claim to its 8/64/128 proxy values. Do not describe last_loss as monotonic, and
preserve the existing distinction between the smoothed proxy and noisier
final-minibatch value.
In `@docs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md`:
- Around line 10-13: Update both reproducibility commands in
docs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md lines 10-13
and docs/design/autotrain-wf-smoke-20260727-steps128-measured-results.md lines
10-13 to use the approved capped rtk invocation, sourcing MAX_RUN_MINUTES=3 from
src/slm_training/levers.py instead of uncapped python, while preserving each
command’s existing model, steps, run ID, device, and seed arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50a07e11-e556-4e67-ae3a-aa305368e79d
📒 Files selected for processing (3)
docs/design/autotrain-loop-ledger-20260725.mddocs/design/autotrain-wf-smoke-20260727-steps128-measured-results.mddocs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md
| ```bash | ||
| python -m scripts.train_model --train-dir src/slm_training/resources/data/train/wf_smoke_v2 \ | ||
| --model twotower --context-backend scratch --steps 64 \ | ||
| --run-id autotrain_wf_smoke_20260727_steps64 --no-sync-checkpoints --device cpu --seed 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rtk availability =="
if command -v rtk >/dev/null 2>&1; then
echo "rtk found"
rtk --help | sed -n '1,80p' || true
else
echo "rtk not found"
fi
echo "== target files =="
for f in docs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md docs/design/autotrain-wf-smoke-20260727-steps128-measured-results.md; do
if [ -f "$f" ]; then
echo "--- $f (lines 1-80) ---"
sed -n '1,80p' "$f"
else
echo "missing $f"
fi
done
echo "== policy references =="
rg -n "rtk gain|MAX_RUN_MINUTES|timed-out or killed|derived run cap|python -m scripts.train_model|scripts.train_model" README.md src docs .agents scripts levers.CONSTRAINT_WEAKENING_LEVERS src/slm_training/levers.py 2>/dev/null || true
echo "== scripts/train_model outline if present =="
if [ -f scripts/train_model.py ]; then argmax "scripts/train_model.py"; else fd -a train_model .; fiRepository: Tyler-R-Kendrick/slm-training
Length of output: 35103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== levers.py MAX_RUN_MINUTES sections =="
sed -n '1,260p' src/slm_training/levers.py | cat -n
echo "== all max/min time references =="
rg -n "MAX_RUN_MINUTES|run cap|timer|timeout|rtk gain|rtk" . --glob '!outputs/**' --glob '!build/**' --glob '!dist/**' | sed -n '1,220p'Repository: Tyler-R-Kendrick/slm-training
Length of output: 36383
Apply the repository’s execution policy to both reproducibility commands.
Both commands document an uncapped python invocation and omit the approved MAX_RUN_MINUTES=3 run cap source from src/slm_training/levers.py. Use the documented capped rtk invocation when available:
docs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md#L10-L13docs/design/autotrain-wf-smoke-20260727-steps128-measured-results.md#L10-L13
📍 Affects 2 files
docs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md#L10-L13(this comment)docs/design/autotrain-wf-smoke-20260727-steps128-measured-results.md#L10-L13
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md` around
lines 10 - 13, Update both reproducibility commands in
docs/design/autotrain-wf-smoke-20260727-steps64-measured-results.md lines 10-13
and docs/design/autotrain-wf-smoke-20260727-steps128-measured-results.md lines
10-13 to use the approved capped rtk invocation, sourcing MAX_RUN_MINUTES=3 from
src/slm_training/levers.py instead of uncapped python, while preserving each
command’s existing model, steps, run ID, device, and seed arguments.
Source: Coding guidelines
Addresses CodeRabbit review: "extends the already-established monotonic loss-decrease trend ... further out to 64 and 128 steps" read as if last_loss's monotonic 4-32 step trend (batch #4) continued to 64/128, which is false (last_loss rises 3.92->5.88 there) and contradicted the very next sentences. Reworded to state plainly that last_loss's monotonicity is scoped to the 4-32 step range, and that only the smoothed example_token_loss_proxy.last_20_mean is monotonic across 8/64/128 -- also dropped an accidental overclaim that this proxy metric was consistent with batch #4's 4/16/32 rows, which never recorded it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDeTk7RxBAbWX5fKjBqb4r
Tyler-R-Kendrick
left a comment
Tyler-R-Kendrick
left a comment
There was a problem hiding this comment.
Two comments here, one fixed and one declined:
-
Ambiguous "monotonic" wording (ledger, line ~418) — legitimate, fixed in 8e868bc. The old phrasing read as if the
last_lossmonotonic trend from batch #4 (4-32 steps) continued through 64/128, which is false and contradicted the very next sentences. Reworded to scopelast_lossmonotonicity explicitly to 4-32 steps, and theexample_token_loss_proxy.last_20_meanmonotonicity explicitly to the 8/64/128 points measured in this section — also dropped an accidental overclaim that the proxy was "consistent with" batch #4's 4/16/32 rows, which never recorded that field at all. -
"Approved capped
rtkinvocation" — declined. I grepped this repo for any existing measured-results doc using anrtk-wrapped orMAX_RUN_MINUTES-sourcing reproducibility command (iter1008,steps4-seed0,bs1-seed0, and every other sibling doc) — every single one uses plainpython -m scripts.train_model ..., same as this PR. There's no such "approved" pattern anywhere in the repo:rtk(per its own skill description) is a shell-output compression tool, not a run-cap mechanism, andMAX_RUN_MINUTESis enforced internally by the harness itself (visible asmax_wall_minutesin everytrain_summary.json), not something a reproducibility command needs to wrap or source. Happy to revisit if you can point to where this convention is actually documented.
Generated by Claude Code
|
Closing — needs rebase after #1120's ledger changes (see review comment); re-propose the 64/128-step section fresh if still wanted. |
2 participants
Summary
wf_smoke_v2recipe shows a real loss-decrease trend beyond the 8-step smoke probe, or whether every prior row is just sampling a fixed single-batch number.seed=0againstmainHEADb908b543(includes docs(autotrain): seed-variation smoke check (seeds 1-3), real new evidence #1116, already merged):--steps 64giveslast_loss=3.9243931770324707,--steps 128giveslast_loss=5.877634525299072.example_token_loss_proxy.last_20_meanfield — already computed bytrain_model.py, never previously surfaced in this ledger — falls monotonically as steps increase:29.6 → 4.4 → 3.3. That's real evidence the harness is fitting the 101-record fixture, not producing a static number.last_lossitself is noisier: it's a single final-minibatch value, not a running average, so it actually rises from 64 to 128 steps even while the smoothed trend keeps improving. Documented as a scope note for future rows (last_lossalone isn't a reliable step-count comparison metric at this fixture size) — this doesn't retract any of the 16+3 prior verified rows, which are all at the same--steps 8wherelast_lossis reproducible even if noisy on its own.fixture_or_scratch— 101-record fixture, no held-out split, no ship gates; near-memorization at 64-128 steps is expected and is not a model-quality claim.verify_version_stamps --checkpasses, 0 components touched).Test plan
python -m scripts.verify_version_stamps --check --base origin/main— ok, 0 components touchedpython -m scripts.repo_policy— okwf_smoke_v2/records.jsonlplaceholders remain canonical (:slot_Nonly) before trainingtrain_summary.jsoninspected (not committed;outputs/is gitignored)Generated by Claude Code
Summary by CodeRabbit