Skip to content

Add an event-log witness-tracing analysis rule - #1407

Merged
fzyzcjy merged 25 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule
Jul 10, 2026
Merged

Add an event-log witness-tracing analysis rule#1407
fzyzcjy merged 25 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

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.

@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 a new witness event analyzer rule and its associated tests to detect witness data mismatches and missing snapshots during training. The feedback focuses on optimizing performance by eliminating nested loop bottlenecks in mismatch detection, adding robust shape and length validation checks for advantage and witness ID lists, and utilizing in-place set operations to improve efficiency.

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.

Comment on lines +142 to +173
latest_attempt_witness_events = _filter_to_latest_attempt(
all_witness_events, group_key=lambda e: (e.rollout_id, e.source.cell_index)
)

for step_event in all_step_events:
rollout_id = step_event.rollout_id

for cell_index, cell_outcome in step_event.cell_outcomes.items():
if cell_outcome == "error":
continue
if not all(r == TrainStepOutcome.NORMAL for r in cell_outcome):
continue

witness_events_of_cell = [
e
for e in latest_attempt_witness_events
if e.rollout_id == rollout_id and e.source.cell_index == cell_index
]

if not witness_events_of_cell:
yield WitnessMissingSnapshotIssue(
rollout_id=rollout_id,
cell_index=cell_index,
description=f"Cell {cell_index} reported NORMAL for rollout {rollout_id} but no WitnessSnapshotParamEvent was found",
)
continue

zero_adv_excused_ids = _zero_adv_excused_ids_at(
zero_adv_witness_ids_by_rollout=zero_adv_witness_ids_by_rollout,
allocated_witness_ids_by_rollout=allocated_witness_ids_by_rollout,
rollout_id=rollout_id,
)

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

Optimize performance by eliminating two nested loop bottlenecks in _find_mismatches:

  1. Group latest_attempt_witness_events by (rollout_id, cell_index) beforehand to reduce the lookup from $O(W)$ to $O(1)$.
  2. Precompute the zero-advantage excused IDs for all unique rollout_ids in a single sorted pass to avoid the $O(N^2)$ complexity of calling _zero_adv_excused_ids_at repeatedly.
    latest_attempt_witness_events = _filter_to_latest_attempt(
        all_witness_events, group_key=lambda e: (e.rollout_id, e.source.cell_index)
    )
    witness_events_by_key = defaultdict(list)
    for e in latest_attempt_witness_events:
        witness_events_by_key[(e.rollout_id, e.source.cell_index)].append(e)

    # Precompute zero-advantage excused IDs for each rollout_id in a single pass to avoid O(N^2) complexity
    zero_adv_excused_ids_by_rollout_cache: dict[int, set[int]] = {}
    excused: set[int] = set()
    all_rids = sorted(set(zero_adv_witness_ids_by_rollout) | set(allocated_witness_ids_by_rollout))
    rid_idx = 0
    n_rids = len(all_rids)
    unique_rollout_ids = sorted({step_event.rollout_id for step_event in all_step_events})
    for r_id in unique_rollout_ids:
        while rid_idx < n_rids and all_rids[rid_idx] <= r_id:
            rid = all_rids[rid_idx]
            excused -= allocated_witness_ids_by_rollout.get(rid, set())
            excused |= zero_adv_witness_ids_by_rollout.get(rid, set())
            rid_idx += 1
        zero_adv_excused_ids_by_rollout_cache[r_id] = set(excused)

    for step_event in all_step_events:
        rollout_id = step_event.rollout_id

        for cell_index, cell_outcome in step_event.cell_outcomes.items():
            if cell_outcome == "error":
                continue
            if not all(r == TrainStepOutcome.NORMAL for r in cell_outcome):
                continue

            witness_events_of_cell = witness_events_by_key.get((rollout_id, cell_index), [])

            if not witness_events_of_cell:
                yield WitnessMissingSnapshotIssue(
                    rollout_id=rollout_id,
                    cell_index=cell_index,
                    description=f"Cell {cell_index} reported NORMAL for rollout {rollout_id} but no WitnessSnapshotParamEvent was found",
                )
                continue

            zero_adv_excused_ids = zero_adv_excused_ids_by_rollout_cache.get(rollout_id, set())

Comment on lines +109 to +111
for adv_tokens, wid_tokens in zip(event.advantages, event.witness_ids, strict=True):
if adv_tokens and all(v == 0.0 for v in adv_tokens):
result[event.rollout_id].add(wid_tokens[0])

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.

medium

When processing lists of sequence-level or token-level tensors (such as advantages and witness IDs), validate that the list lengths match up front, and perform per-sample shape checks to prevent silent mismatches or broadcast failures.

Suggested change
for adv_tokens, wid_tokens in zip(event.advantages, event.witness_ids, strict=True):
if adv_tokens and all(v == 0.0 for v in adv_tokens):
result[event.rollout_id].add(wid_tokens[0])
if len(event.advantages) != len(event.witness_ids):
raise ValueError("Length mismatch between advantages and witness_ids")
for adv_tokens, wid_tokens in zip(event.advantages, event.witness_ids, strict=True):
if len(adv_tokens) != len(wid_tokens):
raise ValueError("Shape mismatch in sequence-level tokens")
if adv_tokens and wid_tokens and all(v == 0.0 for v in adv_tokens):
result[event.rollout_id].add(wid_tokens[0])
References
  1. When processing lists of sequence-level or token-level tensors (such as advantages, student log probabilities, and teacher log probabilities) in RL or distillation pipelines, validate that the list lengths match up front, and perform per-sample shape checks to prevent silent mismatches or broadcast failures.

Comment on lines +128 to +130
for rollout_id in sorted(allocated_witness_ids_by_rollout.keys()):
running = running | allocated_witness_ids_by_rollout[rollout_id]
ans[rollout_id] = set(running)

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.

medium

Optimize set union by using the in-place operator |= instead of creating a new set object with | in each iteration of the loop.

Suggested change
for rollout_id in sorted(allocated_witness_ids_by_rollout.keys()):
running = running | allocated_witness_ids_by_rollout[rollout_id]
ans[rollout_id] = set(running)
for rollout_id in sorted(allocated_witness_ids_by_rollout.keys()):
running |= allocated_witness_ids_by_rollout[rollout_id]
ans[rollout_id] = set(running)

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.

Please fix.

Comment on lines +187 to +199
def _zero_adv_excused_ids_at(
*,
zero_adv_witness_ids_by_rollout: dict[int, set[int]],
allocated_witness_ids_by_rollout: dict[int, set[int]],
rollout_id: int,
) -> set[int]:
excused: set[int] = set()
for rid in sorted(set(zero_adv_witness_ids_by_rollout) | set(allocated_witness_ids_by_rollout)):
if rid > rollout_id:
break
excused -= allocated_witness_ids_by_rollout.get(rid, set())
excused |= zero_adv_witness_ids_by_rollout.get(rid, set())
return excused

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.

medium

Remove the _zero_adv_excused_ids_at helper function as it is no longer needed after precomputing the excused IDs in _find_mismatches.

@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-event-log-checksum-consistency-analysis-rules branch from d5bb6af to 359dadf 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-an-event-log-witness-tracing-analysis-rule branch from 941b7cd to 86bb3b5 Compare June 23, 2026 07:47
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-event-log-checksum-consistency-analysis-rules branch from 359dadf to a67be0b Compare June 23, 2026 09:26
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule branch from 86bb3b5 to 351ac0b Compare June 23, 2026 09:26
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-event-log-checksum-consistency-analysis-rules branch from a67be0b to d0ebbc6 Compare June 23, 2026 13:29
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule branch from 351ac0b to 8dfe785 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 with nits.

for event in _filter_to_latest_attempt(events, group_key=lambda e: e.rollout_id):
for adv_tokens, wid_tokens in zip(event.advantages, event.witness_ids, strict=True):
if adv_tokens and all(v == 0.0 for v in adv_tokens):
result[event.rollout_id].add(wid_tokens[0])

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.

Do you think it'd be safer to assert that every entry in wid_tokens is the same here?

fzyzcjy added a commit that referenced this pull request Jul 8, 2026
…(PR #1407)

Review comments on #1407: wid_tokens is built via torch.full with one
id per sample, so enforce that invariant before trusting wid_tokens[0];
also switch the running-union loop to |=.
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-event-log-checksum-consistency-analysis-rules branch from d0ebbc6 to 42acb06 Compare July 8, 2026 03:53
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule branch from 8dfe785 to 68e937b Compare July 8, 2026 03:53
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-event-log-checksum-consistency-analysis-rules branch from 42acb06 to 95c824d Compare July 8, 2026 05:55
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule branch from 68e937b to 634d3b5 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 15 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.
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-event-log-checksum-consistency-analysis-rules branch from 95c824d to 85f120b Compare July 10, 2026 02:09
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule branch from 634d3b5 to 5b5c797 Compare July 10, 2026 02:09
Base automatically changed from tom/pr_chain/trainer_ft/dev_revert_reversed/add-event-log-checksum-consistency-analysis-rules to main July 10, 2026 03:16
…ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule
@fzyzcjy
fzyzcjy merged commit 8b54af1 into main Jul 10, 2026
6 checks passed
@fzyzcjy
fzyzcjy deleted the tom/pr_chain/trainer_ft/dev_revert_reversed/add-an-event-log-witness-tracing-analysis-rule branch July 10, 2026 03:16
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