Move TrainerRank memory accounting into trainer_rank/_memory - #999
bradhilton wants to merge 2 commits into
Conversation
|
Schulman — no blocking findings in the reviewed scope. Reviewed head All 26 moved memory methods match the original after narrowly accounting for module qualification, self annotations and docstring indentation. Remaining method bodies and class state are preserved. The warm-profile and optimizer-counter fixes survive, runtime globals remain late-bound through Validation: five focused, framework-free CPU checks passed for method binding, exception identity, runtime-global rebinding, warm-profile values and source-fingerprint drift; independent AST comparisons and diff checks passed. No model, GPU, distributed execution, package build or full framework-import test was performed by this review. Expected limitation: module/frame/line metadata changes. Frozen diagnostics that authenticate the old layout must remain pinned or be deliberately updated; these checks do not certify their automatic adoption. This is a source/CPU correctness review, not a new performance or GPU qualification. |
|
Dalinar: memory/planner owner review. No blocking findings. Reviewed head What I checked
Validation
Low
Held planner-stack integration (my held drafts #963, #978, #981 and #986)
Limits: CPU tests only; no GPU smoke run. Behavioral equivalence comes from AST comparison and static reading. |
Extract the 26 memory estimation, profiling and admission accounting methods (1,229 lines) from TrainerRank into a new _memory module. Bodies are moved verbatim; TrainerRank binds the module functions in its class body so self._x(...) dispatch, class-level access and per-instance monkeypatches keep working, and module globals (torch, dist, os, sibling helpers) are still resolved through _impl so tests that patch _impl.torch and friends keep intercepting them. Left in _impl for now: the _backward_region execution wrappers and cache recovery episode (_try_cache_recovery, _cache_recovery_episode, _release_cached_memory_for_backward, _memory_error_with_reduction_note, _execute_split_plan_with_memory_tracking, _run_flat_plan_with_memory_tracking), which tests/unit/test_trainer_rank_backward_work.py re-executes from the _impl.py source.
The planner-miss replay reruns the memory estimators that now live in _memory.py, so include it in the source digest that guards replays against drifted estimator code.
f0a2d88 to
78f6481
Compare
Summary
First of three stacked extractions from
TrainerRank(src/art/trainer_rank/_impl.py, 9,890 lines, one class with 193 methods). This one moves the 26 memory-estimation, profiling and cache-recovery-accounting methods (1,229 lines) intosrc/art/trainer_rank/_memory.py.Pattern — the same one
_gdn_memory.pyand_planner_evidence.pyalready use in this package:_memory.pyholds module-level functionsdef _name(self: TrainerRank, ...)with verbatim bodies, and the class body binds them by assignment (_memory_check = _memory._memory_check;staticmethod(...)for the three statics). No collaborator object, no delegators, no__init__change. Because bodies still call siblings viaself., the ~100 per-instance monkeypatches in the test suite (rank._available_memory_bytes = …), the class-level calls (TrainerRank._split_required_memory(costs)) and theTrainerRank.__new__-built ranks all keep working, andco_nameis preserved for_planner_evidenceframe records. Names that tests patch on_impl(torch,dist,estimate_prefix_tree_packed_tokens,materialize_prefix_tree_layout,_HEAD_CHUNK_TOKENS,_language_model, …) are read as_impl.Xat call time so those patches still intercept._estimate_flat_forwardhas a def-time default= _impl.Unset, so_impl.pyimports_memoryimmediately beforeclass TrainerRank:(commented).Moved:
_split_required_memory,_split_memory_key,_record_split_memory_floor,_split_plan_memory_check,_head_workspace_bytes,_group_head_workspace_bytes,_plan_head_workspace_bytes,_plan_hybridep_growth_bytes,_checkpoint_moe_bytes_per_token,_moe_workspace_bytes,_checkpoint_memory_floor,_retained_memory_bytes,_estimate_flat_forward,_update_peak_memory_profile,_estimate_group_request_output_bytes,_memory_signature_from_requests,_slot_memory_shapes,_memory_check,_refresh_memory_check,_memory_check_required,_forward_memory_group,_estimate_required_memory_bytes_from_values,_gdn_segment_layer_bytes,_available_memory_bytes,_all_ranks_have_memory_profile,_update_memory_profile( = static).Deliberately left in
_impl.py: the_backward_region-wrapped execution/cache-recovery cluster (_try_cache_recovery,_cache_recovery_episode,_release_cached_memory_for_backward,_memory_error_with_reduction_note,_execute_split_plan_with_memory_tracking,_run_flat_plan_with_memory_tracking), becausetests/unit/test_trainer_rank_backward_work.py::actual_rankparses_impl.pysource and re-executes those by name._memory.pyis added to_planner_misses._SOURCE_NAMESso the planner-miss replay drift guard keeps fingerprinting the estimator it reruns (total fingerprinted bytes 552 KB < 1 MiB budget); the existing test asserts it.Equivalence and rebase
main's current method bodies (modulo_impl.qualification, theselfannotation, the dropped@staticmethod, and docstring re-indentation). The class body equalsmain's minus exactly these 26 methods plus the 26 bindings; module level equalsmain's plus the one pre-class import.mainafter this was authored and modified three of the moved methods (_update_peak_memory_profile,_estimate_required_memory_bytes_from_values,_update_memory_profile); those changes are ported into_memory.pyby re-running the same transformation onmain's bodies. No new_impl.qualifications were needed._memory._impl is _impl.Validation
tests/unit/test_trainer_rank_*.py(minusgpu_ci) at-n 4: 1,256 passed on the branch vs 1,257 on a freshmainbaseline; the symmetric difference is three known distributed/subprocess-timeout flakes that pass in isolation on both.main's newtest_trainer_rank_profile_warm.pypasses.ruff check,ruff format --checkclean;ty checkshows the same 6 pre-existing_checkpoint.pydiagnostics asmain._impl.Xtable complete; full unit suite on branch matches base. Its one finding (the_SOURCE_NAMESgap) is fixed here.Observable non-behavioral change:
_planner_evidence.failureframe records for frames inside moved functions now reportmodule: art.trainer_rank._memory(function/co_nameunchanged; nothing branches on the module string).Stack:
refactor/trainer-rank-planner(34 planner methods →_micro_batch_planner.py) andrefactor/trainer-rank-slots-optimizer(41 methods →_slots.py,_optimizer.py) follow on top of this. Combined,_impl.pygoes from 9,890 to ~5,640 lines.