Skip to content

Add structured event logging keyed by per-process identity - #1401

Merged
fzyzcjy merged 19 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity
Jul 10, 2026
Merged

Add structured event logging keyed by per-process identity#1401
fzyzcjy merged 19 commits into
mainfrom
tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity

Conversation

@fzyzcjy

@fzyzcjy fzyzcjy commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

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.

@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 structured EventLogger utility with corresponding unit tests, and refactors logging configuration across various entry points to support process-specific identities. The code review feedback highlights several critical improvements: the event_logger_context decorator needs to support asynchronous methods to prevent context loss; the assertion in the finally block of with_context should be removed to avoid context leaks on failure; configure_logger should be restored in TrainRayActor to prevent unconfigured logging in training processes; and file operations in read_events should be wrapped in error handling to avoid crashes from concurrent file deletions or permission issues.

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 +101 to +111
def decorator(method: Callable) -> Callable:
@functools.wraps(method)
def wrapper(*args: Any, **kwargs: Any) -> Any:
if not is_event_logger_initialized():
return method(*args, **kwargs)

ctx_value = ctx_fn(*args, **kwargs)
with get_event_logger().with_context(ctx_value):
return method(*args, **kwargs)

return wrapper

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.

critical

The event_logger_context decorator does not support asynchronous methods (coroutines). If a decorated method is asynchronous, calling it returns a coroutine object immediately, which exits the with_context block before the coroutine is actually executed or awaited. This causes the context to be completely lost during execution.

To fix this, use inspect.iscoroutinefunction to handle both synchronous and asynchronous methods appropriately.

    import inspect

    def decorator(method: Callable) -> Callable:
        if inspect.iscoroutinefunction(method):
            @functools.wraps(method)
            async def async_wrapper(*args: Any, **kwargs: Any) -> Any:
                if not is_event_logger_initialized():
                    return await method(*args, **kwargs)

                ctx_value = ctx_fn(*args, **kwargs)
                with get_event_logger().with_context(ctx_value):
                    return await method(*args, **kwargs)

            return async_wrapper

        @functools.wraps(method)
        def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
            if not is_event_logger_initialized():
                return method(*args, **kwargs)

            ctx_value = ctx_fn(*args, **kwargs)
            with get_event_logger().with_context(ctx_value):
                return method(*args, **kwargs)

        return sync_wrapper

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.

Perhaps just add an assertion assert not inspect.iscoroutinefunction(method) for now?

Comment on lines +48 to +50
finally:
assert self._context_var.get() == merged
self._context_var.reset(token)

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

The assert self._context_var.get() == merged statement in the finally block is dangerous. If the assertion fails, it raises an AssertionError which prevents the critical cleanup logic self._context_var.reset(token) from executing, leading to a context leak. Additionally, raising an exception in a finally block can mask or suppress any original exception raised inside the try block, making debugging extremely difficult. The assertion should be removed entirely to ensure that reset(token) is always executed.

Suggested change
finally:
assert self._context_var.get() == merged
self._context_var.reset(token)
finally:
self._context_var.reset(token)
References
  1. To prevent resource leaks (e.g., counters that are not decremented), use constructs like try...finally or a with statement to ensure cleanup logic is always executed, even in the case of exceptions or early returns.

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.

I think this makes sense and having an assertion in the finally block here is dangerous. Please fix.

Comment thread miles/ray/train_actor.py

class TrainRayActor(RayActor):
def __init__(self, world_size, rank, master_addr, master_port):
configure_logger()

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

Removing configure_logger() entirely from TrainRayActor means that the training actor processes will run with unconfigured logging (no custom format, no event logger initialization, etc.). Since args is not available in __init__, configure_logger should be called inside the init method of TrainRayActor once args is available, using a TrainProcessIdentity constructed from the actor's role and rank.

Comment on lines +125 to +140
for jsonl_path in jsonl_files:
with open(jsonl_path, encoding="utf-8") as f:
for line_num, raw_line in enumerate(f, start=1):
raw_line = raw_line.strip()
if not raw_line:
continue
try:
event = _event_adapter.validate_json(raw_line)
events.append(event)
except Exception:
logger.warning(
"Failed to parse event at %s:%d",
jsonl_path,
line_num,
exc_info=True,
)

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

open(jsonl_path, encoding="utf-8") is called directly without a try...except block. If any of the matched files are deleted concurrently, or if there are permission issues reading a specific file, the entire read_events function will crash. Wrapping the file opening and reading in a try...except block would make the function much more robust.

Suggested change
for jsonl_path in jsonl_files:
with open(jsonl_path, encoding="utf-8") as f:
for line_num, raw_line in enumerate(f, start=1):
raw_line = raw_line.strip()
if not raw_line:
continue
try:
event = _event_adapter.validate_json(raw_line)
events.append(event)
except Exception:
logger.warning(
"Failed to parse event at %s:%d",
jsonl_path,
line_num,
exc_info=True,
)
for jsonl_path in jsonl_files:
try:
with open(jsonl_path, encoding="utf-8") as f:
for line_num, raw_line in enumerate(f, start=1):
raw_line = raw_line.strip()
if not raw_line:
continue
try:
event = _event_adapter.validate_json(raw_line)
events.append(event)
except Exception:
logger.warning(
"Failed to parse event at %s:%d",
jsonl_path,
line_num,
exc_info=True,
)
except Exception:
logger.warning("Failed to open or read event file %s", jsonl_path, exc_info=True)

@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-models-keyed-by-per-process-identity branch from 66d94d8 to bfc35ed Compare June 23, 2026 07:46
@fzyzcjy
fzyzcjy requested a review from yushengsu-thu as a code owner June 23, 2026 07:46
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity branch from d86a598 to cbb5cc2 Compare June 23, 2026 07:46
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-models-keyed-by-per-process-identity branch from bfc35ed to 9bcfbe8 Compare June 23, 2026 09:25
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity branch from cbb5cc2 to 0172902 Compare June 23, 2026 09:25
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-models-keyed-by-per-process-identity branch from 9bcfbe8 to 7af9705 Compare June 23, 2026 13:29
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity branch from 0172902 to e1e2055 Compare June 23, 2026 13:29
Comment on lines +101 to +111
def decorator(method: Callable) -> Callable:
@functools.wraps(method)
def wrapper(*args: Any, **kwargs: Any) -> Any:
if not is_event_logger_initialized():
return method(*args, **kwargs)

ctx_value = ctx_fn(*args, **kwargs)
with get_event_logger().with_context(ctx_value):
return method(*args, **kwargs)

return wrapper

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.

Perhaps just add an assertion assert not inspect.iscoroutinefunction(method) for now?


class TestGetEventLoggerRaisesWhenNotSet:
def test_raises_runtime_error(self) -> None:
import miles.utils.event_logger.logger as mod

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.

Let's still move all imports to the top.

Comment on lines +48 to +50
finally:
assert self._context_var.get() == merged
self._context_var.reset(token)

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.

I think this makes sense and having an assertion in the finally block here is dangerous. Please fix.

fzyzcjy added a commit that referenced this pull request Jul 8, 2026
Review comment on #1401: decorating a coroutine would exit the
with_context scope before the coroutine runs, silently dropping the
context. Assert at decoration time until async support is needed.
fzyzcjy added a commit that referenced this pull request Jul 8, 2026
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-models-keyed-by-per-process-identity branch from 7af9705 to efa1920 Compare July 8, 2026 03:52
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity branch from e1e2055 to 158ae09 Compare July 8, 2026 03:52
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-models-keyed-by-per-process-identity branch from efa1920 to 448d7da Compare July 8, 2026 05:54
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity branch from 158ae09 to 43f487c Compare July 8, 2026 05:54
fzyzcjy added 8 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.
fzyzcjy added 10 commits July 10, 2026 10:04
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.
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.
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-models-keyed-by-per-process-identity branch from 448d7da to 3e85a86 Compare July 10, 2026 02:08
@fzyzcjy
fzyzcjy force-pushed the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity branch from 43f487c to dc11be3 Compare July 10, 2026 02:08
Base automatically changed from tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-models-keyed-by-per-process-identity to main July 10, 2026 03:12
…ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity
@fzyzcjy
fzyzcjy merged commit 7f7830e into main Jul 10, 2026
6 checks passed
@fzyzcjy
fzyzcjy deleted the tom/pr_chain/trainer_ft/dev_revert_reversed/add-structured-event-logging-keyed-by-per-process-identity branch July 10, 2026 03:13
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