Skip to content

Add dump and inference-engine-checksum comparison helpers for FT tests - #1409

Merged
fzyzcjy merged 27 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests
Jul 10, 2026
Merged

Add dump and inference-engine-checksum comparison helpers for FT tests#1409
fzyzcjy merged 27 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Add comparison helpers used by fault-tolerance tests to compare dumped tensors and
inference-engine checksums offline (generic comparators, dump comparison, and an
inference-engine checksum comparison built on the event-analyzer checksum rule).

  • miles/utils/test_utils/comparisons/{comparators,dumps,inference_engine_checksums}.py and tests.

@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 introduces test comparison utilities, including a runner for the tensor comparator, a helper to compare leaf dump directories, and a utility to compare inference engine weight checksums, along with their respective unit tests. The review feedback highlights two key issues: first, a bug in run_comparator where the 'rank' key is incorrectly omitted from skip_keys when grouping_skip_keys is provided; second, a potential assertion failure in compare_inference_engine_checksums during fault-tolerance tests with retries due to duplicate rollout events, which should be resolved by only keeping the latest event per rollout.

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.

# 'rank' the comparator gets `baseline_load_failed` for every tensor and fails with rc=1.
# Callers may pass extra keys (e.g. no_failure skips 'dp'/'edp' too). (Grouping is a
# comparator-matching detail, not a pass/fail threshold.)
skip_keys: list[str] = list(grouping_skip_keys) if grouping_skip_keys is not None else ["rank"]

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.

high

There is a bug in how skip_keys is constructed. If grouping_skip_keys is provided, "rank" is completely omitted from the list. According to the comments, "rank" must always be skipped to avoid baseline_load_failed errors when comparing FT and non-FT runs. We should ensure "rank" is always included in skip_keys regardless of whether grouping_skip_keys is provided.

Suggested change
skip_keys: list[str] = list(grouping_skip_keys) if grouping_skip_keys is not None else ["rank"]
skip_keys: list[str] = ["rank"] + (list(grouping_skip_keys) if grouping_skip_keys is not None else [])

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.

This looks like a bug.

Comment on lines +15 to +56
# Each side's engines must already agree internally (same invariant as the production rule), so
# one representative engine per rollout then proves baseline == target regardless of engine count.
assert not inference_engine_weight_checksum_consistency.check(
baseline
), "Baseline engines disagree with each other"
assert not inference_engine_weight_checksum_consistency.check(target), "Target engines disagree with each other"

baseline_by_rollout = _checksums_by_rollout_id(baseline)
target_by_rollout = _checksums_by_rollout_id(target)
assert baseline_by_rollout.keys() == target_by_rollout.keys(), (
f"Engine checksum rollout_id sets differ: "
f"baseline={sorted(baseline_by_rollout)} "
f"vs target={sorted(target_by_rollout)}"
)

mismatches: list[ChecksumMismatchIssue] = []
for rollout_id in sorted(baseline_by_rollout):
mismatches += list(
compare_flat_dicts(
a=baseline_by_rollout[rollout_id],
b=target_by_rollout[rollout_id],
label_a=f"baseline/rollout_{rollout_id}",
label_b=f"target/rollout_{rollout_id}",
)
)
assert not mismatches, "Engine weight checksum baseline-vs-target mismatch:\n" + "\n".join(
f" - {m.label_a} vs {m.label_b} key {m.key}: {m.value_a} != {m.value_b}" for m in mismatches
)
print(f"Engine weight checksum comparison passed: {len(baseline_by_rollout)} rollout(s) compared")


def _checksums_by_rollout_id(events: list[InferenceEngineWeightChecksumEvent]) -> dict[int, dict[str, str]]:
by_rollout: dict[int, dict[str, str]] = {}
for event in events:
if event.rollout_id is None:
continue
assert (
event.rollout_id not in by_rollout
), f"Duplicate InferenceEngineWeightChecksumEvent for rollout {event.rollout_id}"
assert event.engine_checksums, f"No engine checksums for rollout {event.rollout_id}"
by_rollout[event.rollout_id] = event.engine_checksums[0]
return by_rollout

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.

high

In fault-tolerance (FT) tests, rollouts can be retried or re-run after a failure, which appends duplicate InferenceEngineWeightChecksumEvents for the same rollout_id to the event log. The current implementation asserts that there are no duplicate events for any rollout_id (lines 51-53), which will cause the comparison helper to fail during any test run that involves a retry. Additionally, running the internal consistency check on all events (including aborted/failed attempts) can lead to false positives. We should filter the events to only keep the latest (successful) attempt for each rollout_id before performing the consistency check and the baseline-vs-target comparison.

    baseline_by_rollout = _latest_events_by_rollout(baseline)
    target_by_rollout = _latest_events_by_rollout(target)

    # Each side's engines must already agree internally (same invariant as the production rule), so
    # one representative engine per rollout then proves baseline == target regardless of engine count.
    assert not inference_engine_weight_checksum_consistency.check(
        list(baseline_by_rollout.values())
    ), "Baseline engines disagree with each other"
    assert not inference_engine_weight_checksum_consistency.check(
        list(target_by_rollout.values())
    ), "Target engines disagree with each other"

    assert baseline_by_rollout.keys() == target_by_rollout.keys(), (
        f"Engine checksum rollout_id sets differ: "
        f"baseline={sorted(baseline_by_rollout)} "
        f"vs target={sorted(target_by_rollout)}"
    )

    mismatches: list[ChecksumMismatchIssue] = []
    for rollout_id in sorted(baseline_by_rollout):
        mismatches += list(
            compare_flat_dicts(
                a=baseline_by_rollout[rollout_id].engine_checksums[0],
                b=target_by_rollout[rollout_id].engine_checksums[0],
                label_a=f"baseline/rollout_{rollout_id}",
                label_b=f"target/rollout_{rollout_id}",
            )
        )
    assert not mismatches, "Engine weight checksum baseline-vs-target mismatch:\n" + "\n".join(
        f"  - {m.label_a} vs {m.label_b} key {m.key}: {m.value_a} != {m.value_b}" for m in mismatches
    )
    print(f"Engine weight checksum comparison passed: {len(baseline_by_rollout)} rollout(s) compared")


def _latest_events_by_rollout(
    events: list[InferenceEngineWeightChecksumEvent],
) -> dict[int, InferenceEngineWeightChecksumEvent]:
    by_rollout: dict[int, InferenceEngineWeightChecksumEvent] = {}
    for event in events:
        if event.rollout_id is None:
            continue
        assert event.engine_checksums, f"No engine checksums for rollout {event.rollout_id}"
        by_rollout[event.rollout_id] = event
    return by_rollout

@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-the-event-log-analyzer-that-applies-analysis-rules branch from 9cde1c9 to a7b69d4 Compare June 23, 2026 07:47
@fzyzcjy
fzyzcjy requested a review from yushengsu-thu as a code owner June 23, 2026 07:47
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests branch from 7a8fac2 to 19ae254 Compare June 23, 2026 07:47
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-the-event-log-analyzer-that-applies-analysis-rules branch from a7b69d4 to 1eff480 Compare June 23, 2026 09:26
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests branch from 19ae254 to bf5b210 Compare June 23, 2026 09:26
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-the-event-log-analyzer-that-applies-analysis-rules branch from 1eff480 to 9938a50 Compare June 23, 2026 13:30
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests branch from bf5b210 to 12dce7d Compare June 23, 2026 13:30

@Shi-Dong Shi-Dong 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.

LGTM

fzyzcjy added a commit that referenced this pull request Jul 8, 2026
Review comment on #1409: 'rank' must always be skipped when grouping
dump bundles (absolute rank IDs differ between FT and non-FT runs).
The only caller passing custom keys already includes it; enforce the
invariant instead of silently dropping it.
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-the-event-log-analyzer-that-applies-analysis-rules branch from 9938a50 to 7d94295 Compare July 8, 2026 03:53
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests branch from 12dce7d to 7b20e3b Compare July 8, 2026 03:53
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-the-event-log-analyzer-that-applies-analysis-rules branch from 7d94295 to ed55414 Compare July 8, 2026 05:55
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests branch from 7b20e3b to 387780b Compare July 8, 2026 05:55
fzyzcjy added 9 commits July 10, 2026 10:04
Add a `deterministic_random` reward that hashes the sample tokens + response to
produce a stable pseudo-random 0/1 reward, used for reproducible
fault-tolerance / CI tests.

- rm_hub/__init__.py (+ test).
Add `inplace_modify_args`, a context manager that temporarily overrides args
attributes and restores them on exit (asserting they weren't clobbered), used to
scope per-attempt argument overrides in the fault-tolerant trainer.

- argparse_utils.py (+ test).
Small shared-utility additions used by the fault-tolerant trainer: hash
non-contiguous tensors safely (reshape before viewing as bytes), an
`enable_experimental_ft_trainer` env flag, forward NCCL_DEBUG/NCCL_DEBUG_FILE to
worker environments, and a `filter_keys` helper.

- ci_utils.py / environ.py / external_utils/command_utils.py / misc.py.
Thread the original backend through ReloadableProcessGroup so that, when a
process group is rebuilt (e.g. after a reconfigure/heal), it is recreated with
the same backend instead of hard-coding NCCL.

- reloadable_process_group.py: carry `backend` in the reload group info.
Add small foundation utilities used across the fault-tolerance trainer: a strict
pydantic base model, a retry helper, a tensor checksum helper, a per-cell
megatron world-size computation, the TrainStepOutcome enum, and the IndepDPInfo
dataclass describing a cell's independent-DP identity.

- pydantic_utils.py / retry_utils.py / checksum_utils.py / megatron_args_utils.py
  / types.py / indep_dp.py and tests.
Add a `log_structured` helper that emits logfmt-style key/value log lines, used
by the fault-tolerance components for greppable structured logs.

- structured_log.py (+ test).
Add a small `Clock` interface (`RealClock` plus a controllable fake clock) so
time-dependent fault-tolerance code (health checks, heartbeats) can be driven
deterministically in tests.

- miles/utils/clock.py and tests.
Add a fault-injector test utility used to deterministically exercise
fault-tolerance code paths.

- miles/utils/test_utils/fault_injector.py.
Add the shared data models for the fault-tolerance control server (e.g. the
`TriState` health value), used by the health checker and later by the HTTP
control server.

- miles/utils/control_server/models.py.
fzyzcjy added 17 commits July 10, 2026 10:04
Add the periodic health checker (debounced TriState status driven by a Clock) and
heartbeat utilities used to monitor train-cell liveness.

- miles/utils/health_checker.py, miles/utils/heartbeat_utils.py and tests.
Add the nvidia-resiliency-ext dependency, the "ft" CI test label, the FT test
fixtures in the rollout conftest, and route startup logging through
configure_logger_raw. The fault-tolerance CLI arguments themselves now live with
the features that consume them (distributed across the per-feature commits).
Unconditionally disconnect-then-reconnect the model-update process group when
(re)connecting rollout engines, guarding the destroy against a missing group, so
a reconfigured/healed engine set can rebuild the NCCL group from scratch.

- broadcast.py: drop the "only disconnect if group exists" short-circuit; guard
  `destroy_process_group` against None.
Expose an `inject_fault` Ray method on TrainRayActor (in its own concurrency
group) that triggers a configured failure mode via the fault injector, so
fault-tolerance tests can crash/hang specific actors on demand.

- train_actor.py: `inject_fault` RPC.
Extract the DP split into a witness-aware split_train_data_by_dp_raw
helper (with unit tests; the key list also carries seq_witness_ids) and
use it to split the training data on the actor side when
delay_split_train_data_by_dp is set, deferring the DP split from the
rollout side to actor-side processing. split_train_data_by_dp stays a
thin wrapper that ray.puts each partition.

- miles/ray/rollout/train_data_conversion.py (+ tests), miles/utils/data.py, actor_group.py, rollout_manager.py.
Add an opt-in deterministic NCCL process-group backend (`--debug-deterministic-collective`)
that folds order-sensitive SUM/AVG reductions into a fixed order so training
collectives are bit-reproducible, registering it as the training world's
distributed backend and requiring synchronous grad sync.

- det_process_group.py (+ GPU test, dist test helper).
- train_actor.py: register the backend and select it when enabled.
- initialize.py: assert synchronous grad reduce under the deterministic backend.
Add a per-process identity helper that uniquely keys each training process, used
to attribute structured fault-tolerance events to their originating process.

- miles/utils/process_identity.py and tests.
Add the structured event models (Event / EventBase hierarchy) for the
fault-tolerance event log, each tagged with the originating ProcessIdentity.

- miles/utils/event_logger/models.py and tests.
Add the structured event logger that records typed events keyed by per-process
identity, wire it through the logging helper and CLI argument, and start it from
the train entrypoints.

- miles/utils/event_logger/logger.py, logging_utils.py, arguments.py and entrypoint wiring, with tests.
Add snapshot/restore for the structured event log so the event history survives
cell restarts during fault-tolerant training.

- miles/utils/event_logger/checkpoint.py and tests.
Add the `MetricEvent` model (a discriminated-union member) and emit every
tracking metric into the structured event log: `tracking_utils.log` now forwards
`{metrics}` to `get_event_logger().log(MetricEvent, ...)` when the event logger
is initialized.
Add the witness id allocator and `WitnessInfo` carrier used to assign and track
witness ids for fault-tolerance verification.

- miles/utils/witness/allocator.py and tests.
Thread witness ids through the model by injecting witness parameters, so the
event log can later verify they propagate correctly.

- miles/utils/witness/module.py, model_provider.py and tests.
Add the first event-analyzer rules that replay the structured event log and flag
weight-checksum inconsistencies: a `checksum_compare` helper (flatten nested
dicts, diff flat checksum maps) plus two rules built on it — cross-replica weight
checksum consistency and inference-engine weight checksum consistency — with unit
tests.

- miles/utils/event_analyzer/rules/{checksum_compare,cross_replica_weight_checksum,inference_engine_weight_checksum_consistency}.py and tests.
Add the witness-tracing rule for the event analyzer: it follows witness ids
through the replayed event log to verify they are propagated correctly across the
training pipeline, with unit tests.

- miles/utils/event_analyzer/rules/witness.py and tests.
Add the analyzer that replays the structured event log and applies the analysis
rules (checksum-consistency and witness tracing) to verify fault-tolerance
behaviour offline, with unit tests.

- miles/utils/event_analyzer/analyzer.py and tests.
Add comparison helpers used by fault-tolerance tests to compare dumped tensors and
inference-engine checksums offline (generic comparators, dump comparison, and an
inference-engine checksum comparison built on the event-analyzer checksum rule).

- miles/utils/test_utils/comparisons/{comparators,dumps,inference_engine_checksums}.py and tests.
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-the-event-log-analyzer-that-applies-analysis-rules branch from ed55414 to 493ef68 Compare July 10, 2026 02:09
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests branch from 387780b to 81b95c4 Compare July 10, 2026 02:11
Base automatically changed from tom/pr_chain/trainer_ft/dev_revert_reversed/add-the-event-log-analyzer-that-applies-analysis-rules to main July 10, 2026 03:17
…ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests
@fzyzcjy
fzyzcjy merged commit 8e56cbe into main Jul 10, 2026
6 checks passed
@fzyzcjy
fzyzcjy deleted the tom/pr_chain/trainer_ft/dev_revert_reversed/add-dump-and-inference-engine-checksum-comparison-helpers-for-ft-tests branch July 10, 2026 03:17
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.

3 participants