Skip to content

Move FT modules into topic folders (mechanical) - #1596

Merged
fzyzcjy merged 1 commit into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical
Jul 10, 2026
Merged

Move FT modules into topic folders (mechanical)#1596
fzyzcjy merged 1 commit into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Group the fault-tolerance work into folders instead of scattering it
across miles/utils and megatron_utils:

  • miles/utils/ft_utils/: control_server, health_checker, heartbeat_utils,
    indep_dp, mini_ft_controller, process_group_utils
  • miles/utils/audit_utils/: event_analyzer, event_logger, witness,
    checksum_utils, process_identity
  • miles/utils/tracking_utils/: structured_log
  • miles/utils/test_utils/: clock, det_process_group
  • miles/backends/megatron_utils/ft/: checkpoint_transfer,
    in_memory_checkpoint, indep_dp, types

The tracking facade moves from tracking_utils/init.py to
tracking_utils/tracking.py (callers import the submodule) so that
importing tracking_utils.structured_log does not run a package
init that circularly imports the event logger.

Pure relocation: git-mv moves, empty package init files, dotted
import-path rewrites, and dot-depth fixes for the relative imports
that cross a moved path (plus isort reordering). No code changes.

Script: ft_restructure_transform.py (attached to the PR); reproduce via
verify_mechanical_refactor(base, target, transform) — byte-identical.

ci-sglang-pr: #28524

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request reorganizes the codebase structure by refactoring and moving various utility modules into more structured subdirectories. Specifically, fault tolerance (FT) utilities have been relocated to miles/utils/ft_utils/, auditing and witness utilities to miles/utils/audit_utils/, tracking and logging utilities to miles/utils/tracking_utils/, and certain test utilities (like clock and det_process_group) to miles/utils/test_utils/. Megatron-specific fault tolerance modules have also been moved under miles/backends/megatron_utils/ft/. All corresponding imports and test files have been updated to reflect these new paths. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from fe3ede2 to def7dff Compare July 8, 2026 05:53
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw branch from 470cbc8 to b444cac Compare July 8, 2026 06:00
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from def7dff to 2bc8c58 Compare July 8, 2026 06:00
@fzyzcjy

fzyzcjy commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Posted autonomously by Claude Code acting on the user's behalf. Triaged the stage-c-8-gpu-h100 failures by comparing against main's last green nightly and PR #1559's own CI history. Please push back if any conclusion is off.

CI status note (2026-07-08):

stage-c-8-gpu-h100 failures here are pre-existing on main, not introduced by this chain:

Fix: radixark/Megatron-LM#64 wires the existing (previously dead) get_dsa_module_spec_for_backend into the experimental-attention-variant dispatch. The fix is also merged into the ci-megatron-pr: #21 branch this PR pins, so rerunning the failed h100 jobs picks it up.

@fzyzcjy

fzyzcjy commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Posted autonomously by Claude Code acting on the user's behalf. Triaged the stage-c-4-gpu-h200 (0) failure from the job log and the last green nightly. Please push back if any conclusion is off.

stage-c-4-gpu-h200 (0) failure (https://github.com/radixark/miles/actions/runs/28921429924/job/85803894664):

  • Failing test: tests/e2e/megatron/test_qwen3_30B_A3B/test_int4_rollout.py — not an FT test; it runs here because run-ci-image forces --match-all-labels.
  • Fingerprint: train step 0 completed normally (ft op=train_step ... outcome=NORMAL), then one MegatronTrainRayActor process died hard: ray.exceptions.ActorUnavailableError: ... RpcError: RPC error: Socket closed rpc_code: 14 — an abrupt worker-process death, no Python traceback on the actor side.
  • The same suite (including this test) passed on main's last nightly (run 28879970610, 2026-07-08) with the same container image digest, and partitions (1)/(2) of this stage passed here.
  • Classification: suspected flake/infra (abrupt actor death after a healthy training step). Next step: rerun failed jobs once the run completes and compare.

@fzyzcjy

fzyzcjy commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Posted autonomously by Claude Code acting on the user's behalf. Follow-up on the stage-c-4-gpu-h200 (0) triage above after the rerun completed.

Rerun result: stage-c-4-gpu-h200 (0) passed on attempt 2 (same code, same image) — the test_int4_rollout.py abrupt actor death is confirmed a flake, as suspected.

@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw branch from b444cac to fbb7115 Compare July 8, 2026 12:45
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from 2bc8c58 to 26c7e0d Compare July 8, 2026 12:45
@fzyzcjy

fzyzcjy commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Posted autonomously by Claude Code acting on the user's behalf. After moving the FT e2e suite to stage-c-8-gpu-h100, the FT tests ran in CI for the first time and surfaced a dependency gap. Reporting the root cause; how to resolve is a maintainer call.

Two independent failures on stage-c-8-gpu-h100, different root causes:

1. GLM5 LoRA tests (test_glm5_1/2_*_lora_ci.py) — pre-existing, now fixed.
dsa experimental-attention spec gaps in Megatron + a stale Megatron-Bridge in the radixark/miles:dev image. Fixed by radixark/Megatron-LM#64 (merged into the pinned ci-megatron-pr: #21) plus a fresh dev image rebuild (the scheduled rebuild had been broken by a transient 429). These should pass on the next run against the fresh image.

2. FT e2e tests (test_trainer_ft_*) — chicken-and-egg dependency, needs a decision.
test_trainer_ft_realistic_gsm8k.py fails at init:

ModuleNotFoundError: No module named 'nvidia_resiliency_ext'
RuntimeError: nvidia_resiliency_ext is required for local checkpointing

Root cause: miles auto-sets non_persistent_ckpt_type="local" whenever train is in --ft-components (miles/utils/arguments.py), and Megatron requires nvidia_resiliency_ext for local checkpointing. So every FT test needs it. nvidia-resiliency-ext is declared in requirements.txt — but only on this chain (added by "Add the fault-tolerance dependency, CI label, and logger-config setup"); it is not on main. The dev image is built from main, so it lacks the module. These tests never ran in CI before (they were registered on the non-existent stage-c-8-gpu-h200 suite), so the gap was invisible until this suite move.

Consequence: the FT tests can only go green in CI against an image built from a requirements.txt that includes nvidia-resiliency-ext — i.e. after this chain lands on main (nightly rebuild), or via a custom image built from the chain and pinned with ci-image-tag: for verification.

@fzyzcjy

fzyzcjy commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Posted autonomously by Claude Code acting on the user's behalf. Explaining why the run-ci-ft-short / run-ci-ft-long labels were dropped from this PR.

Why the FT tests can't run in CI yet (labels removed).

The FT e2e tests were moved to the stage-c-8-gpu-h100 suite (they were previously registered on a non-existent stage-c-8-gpu-h200 suite, so they never actually ran in CI). Running them for the first time revealed a hard dependency gap:

  • Whenever train is in --ft-components, miles auto-sets non_persistent_ckpt_type="local" (miles/utils/arguments.py), and Megatron requires the nvidia_resiliency_ext module for local checkpointing. So every FT test needs it — test_trainer_ft_realistic_gsm8k.py crashes at init with RuntimeError: nvidia_resiliency_ext is required for local checkpointing.
  • nvidia-resiliency-ext is declared in requirements.txt, but only on this chain (added by the "Add the fault-tolerance dependency…" commit). It is not on main.
  • The radixark/miles:dev CI image bakes its Python deps at build time (COPY requirements.txt + pip install -r), built from main. Since main's requirements.txt has no nvidia-resiliency-ext, the image doesn't have the module — and the CI install step uses pip install -e . --no-deps, so it isn't pulled in at test time either.

Net: this is a build-order (chicken-and-egg) situation — the FT tests can only go green against an image built from a requirements.txt that includes nvidia-resiliency-ext, which happens once this chain lands on main and the nightly rebuilds dev. Forcing them to run now would only fail on the missing module, not on anything about the restructure.

So the run-ci-ft-* labels were removed from this PR (they were also being force-run by run-ci-image's --match-all-labels, which is why that label was dropped too). The restructure itself is verified by the mechanical byte-identical proof; run-ci-model-scripts is kept to confirm the GLM5 path (unrelated pre-existing dsa fix) is green on the freshly rebuilt image.

@fzyzcjy fzyzcjy added run-ci-image and removed run-ci-model-scripts Run model script smoke tests labels Jul 8, 2026
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw branch from fbb7115 to 713d01b Compare July 9, 2026 00:25
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from 26c7e0d to a4ba392 Compare July 9, 2026 00:25
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw branch from 713d01b to 2a07a3f Compare July 9, 2026 05:20
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from a4ba392 to 820ef58 Compare July 9, 2026 05:20
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw branch from 2a07a3f to d8a0036 Compare July 9, 2026 08:50
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from 820ef58 to d9abd9a Compare July 9, 2026 08:50
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw branch from d8a0036 to 73b6376 Compare July 9, 2026 12:23
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from d9abd9a to 7460c6f Compare July 9, 2026 12:23
@fzyzcjy

fzyzcjy commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

all green
image

@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw branch from 73b6376 to d46e663 Compare July 10, 2026 02:55
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from 7460c6f to 8bfd8a3 Compare July 10, 2026 02:55
Base automatically changed from tom/pr_chain/trainer_ft/dev_revert_reversed/remove-the-dead-pre-loop-prompt-assignment-in-split-train-data-by-dp-raw to main July 10, 2026 03:40
Group the fault-tolerance work into folders instead of scattering it
across miles/utils and megatron_utils:

- miles/utils/ft_utils/: control_server, health_checker, heartbeat_utils,
  indep_dp, mini_ft_controller, process_group_utils
- miles/utils/audit_utils/: event_analyzer, event_logger, witness,
  checksum_utils, process_identity
- miles/utils/tracking_utils/: structured_log
- miles/utils/test_utils/: clock, det_process_group
- miles/backends/megatron_utils/ft/: checkpoint_transfer,
  in_memory_checkpoint, indep_dp, types

The tracking facade moves from tracking_utils/__init__.py to
tracking_utils/tracking.py (callers import the submodule) so that
importing tracking_utils.structured_log does not run a package
__init__ that circularly imports the event logger.

Pure relocation: git-mv moves, empty package __init__ files, dotted
import-path rewrites, and dot-depth fixes for the relative imports
that cross a moved path (plus isort reordering). No code changes.

Script: ft_restructure_transform.py (attached to the PR); reproduce via
verify_mechanical_refactor(base, target, transform) — byte-identical.

ci-sglang-pr: #28524
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch from 32cbd0e to b02701b Compare July 10, 2026 03:43
@fzyzcjy
fzyzcjy merged commit 9a66b96 into main Jul 10, 2026
6 checks passed
@fzyzcjy
fzyzcjy deleted the tom/pr_chain/trainer_ft/dev_revert_reversed/move-ft-modules-into-topic-folders-mechanical branch July 10, 2026 03:44
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.

1 participant