Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces _TensorViewCodec to encode and decode PyTorch tensors sharing the same underlying storage, optimizing memory usage during checkpoint transfers. It also includes comprehensive unit tests to verify correctness across various scenarios. The feedback suggests two important improvements: first, to include the device in the deduplication key to prevent storage ID collisions across different devices; second, to slice the untyped storage to a multiple of the target element size during decoding to avoid potential RuntimeErrors when reinterpreting the dtype.
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.
| storage_id_by_ptr: dict[int, int] = {} | ||
| unique_storages: list[torch.Tensor] = [] | ||
| view_metas: list[dict] = [] | ||
| for t in tensors: | ||
| storage = t.untyped_storage() | ||
| ptr = storage.data_ptr() | ||
| if ptr not in storage_id_by_ptr: | ||
| storage_id_by_ptr[ptr] = len(unique_storages) | ||
| # Wrap full storage as uint8 tensor (no copy, shares memory). | ||
| unique_storages.append(torch.tensor(storage, dtype=torch.uint8, device=t.device)) | ||
| view_metas.append( | ||
| { | ||
| "storage_id": storage_id_by_ptr[ptr], | ||
| "dtype": t.dtype, | ||
| "shape": tuple(t.shape), | ||
| "stride": tuple(t.stride()), | ||
| "storage_offset": t.storage_offset(), | ||
| } | ||
| ) |
There was a problem hiding this comment.
Using only storage.data_ptr() as the key in storage_id_by_ptr can lead to collisions if the input list contains tensors from different devices (e.g., CPU and GPU, or multiple GPUs). Since raw memory addresses are independent across devices, they can easily overlap. To prevent incorrect storage deduplication, use a tuple of (t.device, ptr) as the dictionary key.
| storage_id_by_ptr: dict[int, int] = {} | |
| unique_storages: list[torch.Tensor] = [] | |
| view_metas: list[dict] = [] | |
| for t in tensors: | |
| storage = t.untyped_storage() | |
| ptr = storage.data_ptr() | |
| if ptr not in storage_id_by_ptr: | |
| storage_id_by_ptr[ptr] = len(unique_storages) | |
| # Wrap full storage as uint8 tensor (no copy, shares memory). | |
| unique_storages.append(torch.tensor(storage, dtype=torch.uint8, device=t.device)) | |
| view_metas.append( | |
| { | |
| "storage_id": storage_id_by_ptr[ptr], | |
| "dtype": t.dtype, | |
| "shape": tuple(t.shape), | |
| "stride": tuple(t.stride()), | |
| "storage_offset": t.storage_offset(), | |
| } | |
| ) | |
| storage_id_by_ptr: dict[tuple[torch.device, int], int] = {} | |
| unique_storages: list[torch.Tensor] = [] | |
| view_metas: list[dict] = [] | |
| for t in tensors: | |
| storage = t.untyped_storage() | |
| ptr = storage.data_ptr() | |
| key = (t.device, ptr) | |
| if key not in storage_id_by_ptr: | |
| storage_id_by_ptr[key] = len(unique_storages) | |
| # Wrap full storage as uint8 tensor (no copy, shares memory). | |
| unique_storages.append(torch.tensor(storage, dtype=torch.uint8, device=t.device)) | |
| view_metas.append( | |
| { | |
| "storage_id": storage_id_by_ptr[key], | |
| "dtype": t.dtype, | |
| "shape": tuple(t.shape), | |
| "stride": tuple(t.stride()), | |
| "storage_offset": t.storage_offset(), | |
| } | |
| ) |
There was a problem hiding this comment.
I think this concern is legitimate and worth addressing.
| for vm in view_metas: | ||
| storage_t = unique_storages[vm["storage_id"]] # uint8 view of received storage | ||
| # Reinterpret bytes as the original dtype, then apply stride/offset. | ||
| dtype_view = storage_t.view(vm["dtype"]) | ||
| view = torch.as_strided( | ||
| dtype_view, | ||
| size=vm["shape"], | ||
| stride=vm["stride"], | ||
| storage_offset=vm["storage_offset"], | ||
| ) | ||
| tensors.append(view) |
There was a problem hiding this comment.
If the untyped storage size is not a multiple of the element size of vm["dtype"] (which can happen with mixed-dtype storages or padded storages), calling storage_t.view(vm["dtype"]) will raise a RuntimeError. To prevent this, slice storage_t to a multiple of the element size before reinterpreting its dtype.
| for vm in view_metas: | |
| storage_t = unique_storages[vm["storage_id"]] # uint8 view of received storage | |
| # Reinterpret bytes as the original dtype, then apply stride/offset. | |
| dtype_view = storage_t.view(vm["dtype"]) | |
| view = torch.as_strided( | |
| dtype_view, | |
| size=vm["shape"], | |
| stride=vm["stride"], | |
| storage_offset=vm["storage_offset"], | |
| ) | |
| tensors.append(view) | |
| for vm in view_metas: | |
| storage_t = unique_storages[vm["storage_id"]] # uint8 view of received storage | |
| # Reinterpret bytes as the original dtype, then apply stride/offset. | |
| element_size = torch.tensor([], dtype=vm["dtype"]).element_size() | |
| num_bytes = (storage_t.numel() // element_size) * element_size | |
| dtype_view = storage_t[:num_bytes].view(vm["dtype"]) | |
| view = torch.as_strided( | |
| dtype_view, | |
| size=vm["shape"], | |
| stride=vm["stride"], | |
| storage_offset=vm["storage_offset"], | |
| ) | |
| tensors.append(view) |
10b39c7 to
99ca854
Compare
615139e to
7b7d21b
Compare
99ca854 to
4828875
Compare
7b7d21b to
04cfe0f
Compare
4828875 to
58be551
Compare
04cfe0f to
dea367a
Compare
Shi-Dong
left a comment
There was a problem hiding this comment.
Please address the Gemini concern on duplicated dict keys.
| storage_id_by_ptr: dict[int, int] = {} | ||
| unique_storages: list[torch.Tensor] = [] | ||
| view_metas: list[dict] = [] | ||
| for t in tensors: | ||
| storage = t.untyped_storage() | ||
| ptr = storage.data_ptr() | ||
| if ptr not in storage_id_by_ptr: | ||
| storage_id_by_ptr[ptr] = len(unique_storages) | ||
| # Wrap full storage as uint8 tensor (no copy, shares memory). | ||
| unique_storages.append(torch.tensor(storage, dtype=torch.uint8, device=t.device)) | ||
| view_metas.append( | ||
| { | ||
| "storage_id": storage_id_by_ptr[ptr], | ||
| "dtype": t.dtype, | ||
| "shape": tuple(t.shape), | ||
| "stride": tuple(t.stride()), | ||
| "storage_offset": t.storage_offset(), | ||
| } | ||
| ) |
There was a problem hiding this comment.
I think this concern is legitimate and worth addressing.
Review comment on #1413: raw data_ptr values can collide across devices, which would alias unrelated storages during checkpoint transfer. Include the device in the dedup key and cover mixed-device encoding with tests.
58be551 to
aa46de3
Compare
dea367a to
09e58f1
Compare
aa46de3 to
51bc303
Compare
09e58f1 to
342d174
Compare
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.
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.
Add the metric comparison helpers used by fault-tolerance tests to compare logged training metrics offline. - miles/utils/test_utils/comparisons/metrics.py.
Add reusable reconfiguration assertions used by fault-tolerance tests to verify cell reconfigure / healing behaviour, with unit tests. - miles/utils/test_utils/reconfigure_assertions.py and tests.
Move the GroupInfo dataclass out of training_utils/parallel.py into a shared process_group_utils module that also provides multi-process-group helpers (GeneralPGUtil / MultiPGUtil) for collectives over single or hierarchical process groups, used by the cross-replica / effective-DP code paths. - process_group_utils.py: GroupInfo + GeneralPGUtil / MultiPGUtil (+ tests). - training_utils/parallel.py: import GroupInfo from the shared module. - megatron / fsdp parallel.py: import GroupInfo from the shared module. - distributed_utils.py: use GeneralPGUtil for masked-whiten all-reduce.
Introduce `_TensorViewCodec` (in `checkpoint_transfer.py`), which encodes a list of tensors as (unique_storages, view_metas) by deduping shared underlying storages (e.g. Megatron distributed-optimizer grad buckets) and reconstructs the original views via `as_strided`. Comprehensively unit-tested by `TestTensorViewCodec`. Consumed by the peer checkpoint transfer added next.
51bc303 to
932660e
Compare
342d174 to
69c50b9
Compare
…ft/dev_revert_reversed/add-tensorviewcodec-for-storage-deduplicated-tensor-serialization
Introduce
_TensorViewCodec(incheckpoint_transfer.py), which encodes a listof tensors as (unique_storages, view_metas) by deduping shared underlying
storages (e.g. Megatron distributed-optimizer grad buckets) and reconstructs the
original views via
as_strided. Comprehensively unit-tested byTestTensorViewCodec. Consumed by the peer checkpoint transfer added next.