Skip to content

Preserve configured gradient clipping in accuracy mode - #14

Open
zrr1999 wants to merge 8 commits into
PFCCLab:mainfrom
zrr1999:fix/restore-accuracy-mode-20260916
Open

zrr1999 wants to merge 8 commits into
PFCCLab:mainfrom
zrr1999:fix/restore-accuracy-mode-20260916

Conversation

@zrr1999

@zrr1999 zrr1999 commented Sep 16, 2026 •

Copy link
Copy Markdown

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

PR information

Preserve the caller's clip_grad during accuracy-mode SFT initialization. A requested positive threshold previously became zero before optimizer construction, silently disabling gradient clipping. Require companion Megatron reproducible-norm support when accuracy mode requests positive clipping, so an incompatible wheel fails explicitly.

Retain all existing configuration names and model-specific switches, including native_unfused_adamw. No switch renaming, model-path rewrite, or comparison-tolerance change is included. Related: #12, #13 and PaddleFleet #1961.

Sync the accuracy CI image and PaddleFleet ops wheel download to CUDA 12.9, matching Megatron-LM commit 8aa1d8a. This changes exactly the image tag and the ops wheel CUDA directory. YAML parsing and all eight embedded shell blocks passed syntax checks. Torch installation settings remain controlled by the shared Fleet setup script. The new CI run must pass before merge.

Experiment results

Seven focused tests passed: real SFT construction preserves thresholds 0, 0.25 and 1 in both modes; an incompatible Megatron clipping implementation fails explicitly. Flake8, isort, YAPF and diff checks passed. The companion Megatron tests passed 14 cases on each of two GPU ranks. The previous head dbf01b04 passed GPU precision CI. The new CI-only head requires its own native results.

Paired local validation uses Megatron 6b4771e50, Swift d3d0b1a87, and Fleet d889f0a1: the original GLM52 CI profile passed 100 bitwise-identical loss steps and 187 identical canonical checkpoint tensors; the acceptance-profile native entrypoints separately passed 100 steps with strict loss, provenance and checkpoint oracles. Both sides also match their prior local baselines. The original two-rank GLM45 10-step regression with clip_grad=1.0 passed all 40 raw per-token/final-loss hash records and both same-side baseline comparisons.

These runs use local Paddle 3.4.0.post20260808+733f3454aa0 and Torch 2.12.1+cu129; they do not establish remote CI equivalence. Evidence: experiments/ops/restore-pair-20260916/{ci-terminal,terminal}.json and experiments/ops/restore-pair-regression-20260916/glm45-pair-r0/{protocol,source-binding,comparison}.json.

Companion PR: PFCCLab/Megatron-LM#21.

The GPU unit workflow now uses a checkout unique to the run and attempt. Docker receives a read-only source mount and runs the existing tests in a private copy; cleanup removes only the new checkout. Old root-owned workspace files no longer require a privileged recursive repair. CI-script edits trigger GPU tests, and hosted lint uses pinned Node 24 actions. The legacy GPU host uses native Git for the exact event commit because its glibc cannot run Node 24.

Four actual filesystem regressions, Actionlint, wrapper ShellCheck, shell syntax checks, full-repository pre-commit and actual commit hooks pass. Failures retain their exit codes and private copies are removed; host source and Git metadata remain unchanged. Only Git commit signing was disabled. Current native GPU unit/accuracy and NPU results remain required, with no test or comparison threshold reduced.

Git LFS is initialized only in the disposable checkout. If the binary is missing, the job verifies the official v3.8.0 archive SHA-256 before using a temporary binary, then fetches, materializes and checks the LFS objects. The current GPU unit job fails closed because the host has Git 1.8.3.1, while Git LFS requires Git >= 2.0. An earlier diagnostic run also established that the required /mnt/modelscope/ci_env.sh is absent. A compatible, correctly configured runner is required; these are infrastructure blockers, not passing unit tests. The NPU job remains required.

Latest-head a4736be3 model-accuracy alignment passed (job 111320729675): all recorded ranks and steps match for per-token and final loss MD5. The GPU unit runner and NPU requirements above remain open.

Signed-off-by: Zhan Rongrui <me@zrr.dev>
Apply the equivalent of PFCCLab/Megatron-LM commit 8aa1d8a30b1983debbae1bdea1a080d7eebe01d8 to the Swift accuracy workflow.

Signed-off-by: Zhan Rongrui <me@zrr.dev>

@morirun morirun 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.

温和核对:

  1. 去掉 accuracy 模式下强制 clip_grad = 0 的意图清楚;get_optimizer_and_scheduler 对 get_reproducible_grad_norm_bins 的硬依赖需要配对的 Megatron-Core(见 PFCCLab/Megatron-LM#21)先落地或可探测,否则本地/CI 会直接 ValueError。
  2. 当前 CI:lint 红(yapf / trailing-whitespace / end-of-file-fixer / double-quote-string-fixer);其中部分 hook 改到了本 PR 未触及的文件(如 scripts/dependence/build.sh、swift/megatron/trainers/trainer.py),建议在分支上跑一遍 pre-commit run --all-files 再推,避免基线漂移干扰。
  3. unittest 日志里是 runner sudo/tty 基础设施失败,未必是本 diff 逻辑问题;alignment_model_accuracy 镜像从 cu130 改到 cu129 请确认与 ops wheel 路径一致且为有意变更。

不代推;合入与否由维护者决定。

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.

2 participants