Repository navigation
refactor(trainer): share RL/SFT loop setup, resume step, trace and step logging - #3851
Closed
faresobeid wants to merge 1 commit into
Closed
faresobeid wants to merge 1 commit into
faresobeid wants to merge 1 commit into
Conversation
faresobeid
marked this pull request as ready for review
October 4, 2026 12:32
This was referenced Oct 4, 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 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
The RL and SFT train loops repeated the same setup and teardown code. This PR moves the shared parts into plain helpers in
trainer/utils.py. The micro-step loops stay separate, and the gradient-manager calls are unchanged.setup_trainer(config, seq_len=None): torch distributed, CPU optimizer threads (full offload), matmul precision,resolve_ep,get_parallel_dims. Replacessetup_full_cpu_optimizer_offload.setup_torch_distributedstays becausetools/convert_dcp_to_bf16.pyuses it.resolve_resume_step(resume, ckpt_dir): the checkpoint step to resume from (same rules as before).start_trace(trace_path)/save_trace(prof, trace_path): the torch profiler start and export. I used two functions, not a context manager, so the long loop bodies are not re-indented (that would conflict with every open PR on these files).log_step_metrics(step, metrics, ...): adds the shared perf / optim / checkpoint-disk metrics and logs the step in onemonitors.logcall.setup_optimizer(config.optim, model, parallel_dims, config.model): reads the offload settings and builds the full-offload dtype policy itself. Both loops made the same 10-line call before.Not done: CP rank/group and the final checkpoint stay inline. Each one is 2 to 4 lines that differ a little between RL and SFT, so a helper would not save code.
Behavior change
Metric keys and values are the same. The only change: each trainer step is now one row in
metrics.jsonland onewandb.logcall, not 5 rows (RL) or 6 to 7 rows (SFT). The dashboard reads metrics by key, so it is not affected. Also, if a monitor fails to log a step, the whole step row is lost now, not just one group of metrics.What I ran
pytest tests/unit/orchestrator tests/unit/test_configs.py -m "not gpu": 291 passed.pytest tests/unit/train -m "not gpu" --ignore=tests/unit/train/models: 98 passed. 1 failed:test_cpu_adam.py::test_native_cpu_adamw_matches_fused_torch_and_preserves_gradientsneedsninjato build the C++ extension, and this machine does not have it. That test does not touch code changed here.ruff checkandruff format --checkon the changed files: clean.resolve_resume_stepagainst the old inline code for all resume combinations, and it runs a smoke call of the newsetup_optimizersignature (AdamW, with and withoutoptim_cpu_offload).GPU checks to do
trace_pathset: check that the trace file is written and the W&B /metrics.jsonlkeys are unchanged.--resumefrom a checkpoint: check that it resumes at the right step.model.full_offload, e.g. the GLM-5.3 setup) for one or two steps: check that the optimizer is built with the dtype policy and that NUMA binding / thread setup still logs.Conflicts
rl/train.py): textual conflict next to the newlog_step_metricscall and in the imports. Easy to resolve.resolve_latest_ckpt_stepimport inrl/train.py): textual conflict. This PR removes that import.optim/__init__.pyimports, SFT resume load): merges cleanly, but it touches the same files.rl/train.py): merge cleanly with this branch, but they touch the same file.refactor/trainer-gradpath) conflicts inoptim/__init__.py,utils.pyand both loops; refactor(trainer): add iter_moe_blocks for the MoE layer lookup #3843 (refactor/trainer-moe) conflicts in both loops;refactor/trainer-vlmconflicts inrl/train.py.Note
Low Risk
Refactor with equivalent setup and metric semantics; the main operational change is atomic per-step logging (a failed log drops the whole step row).
Overview
Pulls duplicated RL/SFT trainer bootstrap and per-step logging into
trainer/utils.py, without changing the main training loops.setup_trainerreplaces inline distributed init, full-offload NUMA/thread setup, matmul precision,resolve_ep, andget_parallel_dims.resolve_resume_step,start_trace/save_trace, andlog_step_metricscentralize resume resolution, Chrome trace export, and monitoring.setup_optimizernow takes(optim config, model, parallel_dims, model_config)and reads CPU/full offload settings plus the full-offload dtype policy viaget_full_offload_dtype_policyinternally.Behavior: metric keys/values stay the same, but each step emits one
monitors.logrow (perf, optim, disk, and loop-specific metrics together) instead of several separate writes per step.Reviewed by Cursor Bugbot for commit f889b72. Bugbot is set up for automated code reviews on this repo. Configure here.