refactor: FSDP patch lifecycle, naming clarity, and helper relocation - #61
Open
Jackie2049 wants to merge 20 commits into
Open
Jackie2049 wants to merge 20 commits into
Jackie2049 wants to merge 20 commits into
Conversation
forward_backward_batch wrapper 中 ctx_cleanup()(含 _log_prefix_sharing_audit 打印 + ctx.store.close())被误关在 PREFIX_SHARING_DIAG_DUMP 条件块内, 导致正常训练(未开 diag dump)时: - audit 日志(store_count/reuse_hit/matches_expected/sharing_group)完全不输出 - KV store 不 close,多步训练存在 ContextVar/KV 内存累积风险 将 ctx_cleanup 调用移出 DIAG_DUMP 条件块,在 backward 完成后无条件执行 (保留 not forward_only 条件,与原设计一致)。DIAG_DUMP 块只保留 weight grad dump + per-layer grad hook 清理。 回归验证:4090 单卡/多卡 PS ON smoke 测试发现 audit 0 次输出,修复后预期 出现 [PS][audit] summary + 24 层 layer runtime。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move hot-path imports to module scope, read PS config via verl_utils, and rename the prefix_grouper helper to read_ps_config_from_prefix_grouper. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename build_prefix_sharing_micro_batch_fsdp to plan_and_trim_microbatch_fsdp with clearer micro_batch/ps_config args, and rewrite PrefixSharingConfig.validate error messages in English while keeping integrate_mode fallback semantics. Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename private `_trim_*` / `_extract_*` / `_clone_batch` helpers in `verl_utils` to public API without leading underscore. - Add `trim_redundant_prefix_in_dense_tensor` for the FSDP 2D mask-only trim path; return `kept_position_rows` directly from trim helpers so callers do not re-extract position rows. - Update FSDP, Megatron and setup patch callers to use the new helpers and import `is_nested_tensor` from `verl_utils` instead of `verl_mcore`. - Default `PackedBatchLayout.from_kept_position_rows(align_size=1)` so FSDP callers can omit the argument. - Add `test_verl_utils_trim.py` and update boundary test imports. Co-authored-by: Cursor <cursoragent@cursor.com>
…ion_rows Replace repeated `first.device` lookups with a single `device` local variable to improve readability. Co-authored-by: Cursor <cursoragent@cursor.com>
…for_prefix_sharing_fsdp - Rename `plan_and_trim_microbatch_fsdp` to `prepare_for_prefix_sharing_fsdp` to better reflect its responsibility (plan, trim, build layout, and construct runtime state). - Update all callers: verl_fsdp patch, integrations __init__, and FSDP unit tests. - Add STEP comments to clarify the prepare/execute phases in `verl_fsdp.py`. Co-authored-by: Cursor <cursoragent@cursor.com>
Add explicit STEP comments to mark the prepare and execute phases in `forward_prefix_sharing_fsdp_micro_batch` and the internal steps inside `prepare_for_prefix_sharing_fsdp`. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename `forward_prefix_sharing_fsdp_micro_batch` to `forward_prefix_sharing_micro_batch_fsdp` for naming consistency with `prepare_for_prefix_sharing_fsdp`. Update all callers, imports, and docs. Also clarify the STEP comments and variable names inside the forward helper. Co-authored-by: Cursor <cursoragent@cursor.com>
…_model Rename the `prefix_sharing_runtime` keyword parameter of `_call_fsdp_model` to `attention_runtime` to reflect that the helper is agnostic to the specific attention runtime implementation. The model input key remains `prefix_sharing_runtime` since that is the contract expected by the model forward / attention patch. Co-authored-by: Cursor <cursoragent@cursor.com>
Move function-local imports in `_forward_step_with_engine_prepare` to the module top level for consistency and to avoid repeated import overhead. Other functions in the same file are left unchanged as requested. Co-authored-by: Cursor <cursoragent@cursor.com>
In `_forward_step_with_engine_prepare`, rename: - `trimmed_micro_batch` → `micro_batch_modified` - `ps_state` → `prefix_sharing_runtime_state` These names match the standalone helper in `verl_fsdp.py` and make the relationship between the two paths clearer. Co-authored-by: Cursor <cursoragent@cursor.com>
Collapse the two multi-line `_read_runtime_value` calls in the `model_config` dict to single-line form for readability. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the `device_name` intermediate variable in `_forward_step_with_engine_prepare` and call `_read_device_name()` directly inside `torch.autocast()`. Co-authored-by: Cursor <cursoragent@cursor.com>
…gine_prepare Rename `forward_prefix_sharing_micro_batch_fsdp` to `forward_step_without_engine_prepare` to mirror the production path `_forward_step_with_engine_prepare` and make the dispatch logic clearer. Update all imports, callers, tests, and docs. Co-authored-by: Cursor <cursoragent@cursor.com>
…prepare Construct `model_config` once in `patched_forward_step`, reuse it for `ps_config.validate`, and pass it as a parameter to `_forward_step_with_engine_prepare` and `forward_step_without_engine_prepare`. This eliminates duplicate `_read_runtime_value` calls and aligns the two dispatch paths with the same helper API. Co-authored-by: Cursor <cursoragent@cursor.com>
Move restore_reuser_prefix_columns_2d, restore_via_2d_unfold_verl080 and their helpers from verl_mcore to verl_utils so that verl_mcore only contains Megatron-specific micro-batch building logic. Update all import sites and tests accordingly. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename the private context attribute from `_ps_ctx` to `_prefix_sharing_context` and the cleanup callback to `_cleanup_prefix_sharing_context` so the lifecycle is self-documenting. Add STEP comments to the FSDP attention patch and forward_step to make the prepare/execute/restore phases explicit. Reorganize imports and simplify local variable names while preserving the original behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…t lifecycle note Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR completes the second FSDP-focused refactor wave after the setup lifecycle consolidation. It makes the FSDP patch set easier to inspect, restores runtime cleanup after every training backward pass, removes avoidable work from the FSDP forward-step hot path, and relocates shared restore helpers so that framework-specific modules stay focused.
Design
verl_utils, while Megatron-specific micro-batch building stays inverl_mcore.Key Changes
PATCH_SETcomments and descriptions, documented each wrapper purpose, and added an integration assertion for the profiler then diagnostic-wrapper order.verl_mcoretoverl_utils: movedrestore_reuser_prefix_columns_2d,restore_via_2d_unfold_verl080and their helpers so thatverl_mcoreonly contains Megatron-specific micro-batch building logic. Updated all import sites (FSDP patch, Megatron patch, tests) accordingly._ps_ctxto_prefix_sharing_contextand the cleanup callback to_cleanup_prefix_sharing_context, added explicitSTEPcomments in the FSDP attention patch andforward_stepto mark the prepare/execute/restore phases, and simplified local variable names while preserving behavior.Test Results
Local CPU regression
Result: 290 passed, 31 skipped, 4 deselected.
4090 verl 0.8.0 FSDP environment
Result: 49 passed, 9 warnings.
Static checks
Result: passed.
git diff --checkalso passed.Remaining Items and Potential Impact
PatchHandlerollback ledger because it patches a registry entry rather than a normal module attribute. It should be moved into a reversible installer primitive in a later refactor.