-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Replace lazy MFSDP context initialization with an explicit scope #6190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
wujingyue
wants to merge
6
commits into
NVIDIA:main
Choose a base branch
from
wujingyue:fully-shard-context
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
2fc9b25
Add eager MFSDP construction context
wujingyue eb86c56
Refine MFSDP context ownership
wujingyue eb4b0f3
Simplify MFSDP context finalization
wujingyue c9b1302
Clarify MFSDP subtree collection helper
wujingyue 1c5cf1e
Simplify MFSDP overlap tests
wujingyue 2c0c385
Keep FsdpContext internal
wujingyue File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,36 +28,76 @@ | |
|
|
||
|
|
||
| class FsdpContext: | ||
| """Runtime stream and prefetch state shared by one FSDP subtree.""" | ||
| """Runtime stream and prefetch state shared by FSDP roots constructed together.""" | ||
|
|
||
| allgather_stream: torch.cuda.Stream | ||
| reduce_scatter_stream: torch.cuda.Stream | ||
| # HFSDP/HSDP need explicit last-microbatch state. First-microbatch state is | ||
| # unnecessary because it can be detected when ``model_weight``, after syncing | ||
| # from ``main_weight``, has placements different from ``Placements.optimizer``. | ||
| is_last_microbatch: bool | ||
| root_module: "FsdpModule" | ||
| # Static orders used to drive all-gather prefetch. We may want to switch to | ||
| # capturing runtime order if static module order proves too fragile. Each | ||
| # FsdpModule tracks its own materialized state via ``FsdpModule._unshard_event``. | ||
| forward_order: IndexedOrder["FsdpModule"] | ||
| backward_order: IndexedOrder["FsdpModule"] | ||
|
|
||
| def __init__(self, device: torch.device, root_module: "FsdpModule") -> None: | ||
| """Create rank-local runtime state for a root FSDP subtree. | ||
| def __init__(self, device: torch.device) -> None: | ||
| """Create rank-local runtime state for FSDP modules on ``device``. | ||
|
|
||
| Args: | ||
| device: Device on which this context schedules communication. | ||
| root_module: Outermost module that owns this context. | ||
| """ | ||
| self.root_module = root_module | ||
| self.is_last_microbatch = True | ||
| self.forward_order = IndexedOrder() | ||
| self.backward_order = IndexedOrder() | ||
| self._registered_modules: list[FsdpModule] = [] | ||
| self._is_finalized = False | ||
| with torch.cuda.device(device): | ||
| self.allgather_stream = torch.cuda.Stream() | ||
| self.reduce_scatter_stream = torch.cuda.Stream() | ||
|
|
||
| def register_module(self, module: "FsdpModule") -> None: | ||
| """Register a module constructed in this context.""" | ||
| if self._is_finalized: | ||
| raise RuntimeError("Cannot register an FSDP module after its context is finalized.") | ||
| self._registered_modules.append(module) | ||
|
|
||
| def finalize(self) -> None: | ||
| """Finalize roots, names, and cross-root prefetch orders.""" | ||
| if self._is_finalized: | ||
| raise RuntimeError("FSDP context is already finalized.") | ||
|
|
||
| visited: set[FsdpModule] = set() | ||
| root_modules: list[FsdpModule] = [] | ||
| for module in reversed(self._registered_modules): | ||
| if module in visited: | ||
| continue | ||
| root_modules.append(module) | ||
| _collect_fsdp_modules_under(cast(nn.Module, module), visited) | ||
| root_modules.reverse() | ||
| for root_module in root_modules: | ||
| root_module._is_root = True | ||
|
Comment on lines
+71
to
+80
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to myself: there's an easy way to compute root_modules without assuming the order of registered_modules |
||
|
|
||
| for root_module in root_modules: | ||
| for name, module in cast(nn.Module, root_module).named_modules(): | ||
| if not isinstance(module, FsdpModule): | ||
| continue | ||
| module._name = name | ||
| self.forward_order.append(module) | ||
|
|
||
| for root_module in reversed(root_modules): | ||
| _collect_backward_order(cast(nn.Module, root_module), self.backward_order) | ||
|
|
||
| self._is_finalized = True | ||
|
|
||
| def ensure_finalized(self) -> None: | ||
| """Raise if construction has not completed for this context.""" | ||
| if not self._is_finalized: | ||
| raise RuntimeError( | ||
| "FSDP context is not finalized. Exit fully_shard_context before running forward." | ||
| ) | ||
|
|
||
| def current_stream(self) -> torch.cuda.Stream: | ||
| """Current stream on this context's device.""" | ||
| return torch.cuda.current_stream(self.allgather_stream.device) | ||
|
|
@@ -84,7 +124,8 @@ class FsdpModule: | |
| # Root uses "" and None means uninitialized. | ||
| _name: str | None | ||
| _parameter_groups: tuple[FsdpParameterGroup, ...] | ||
| _context: FsdpContext | None | ||
| _context: FsdpContext | ||
| _is_root: bool | ||
| _ready_grad_parameters: set[nn.Parameter] | ||
| _num_trainable_parameters: int | ||
| # Event recorded after this FsdpModule's full parameters are materialized. | ||
|
|
@@ -94,13 +135,15 @@ class FsdpModule: | |
|
|
||
| def __init__( | ||
| self, | ||
| context: FsdpContext, | ||
| mesh: DeviceMesh, | ||
| placements: Placements, | ||
| mixed_precision_policy: MixedPrecisionPolicy, | ||
| use_symm_mem: bool = False, | ||
| ) -> None: | ||
| """Initialize FSDP runtime state on an already-constructed module.""" | ||
| self._context = None | ||
| self._context = context | ||
| self._is_root = False | ||
| self._name = None | ||
| self._unshard_event = None | ||
| owned_parameters = _collect_owned_parameters(self) | ||
|
|
@@ -124,60 +167,11 @@ def __init__( | |
| len(group.sharded_parameters) for group in self._parameter_groups if group.requires_grad | ||
| ) | ||
| self._register_hooks() | ||
|
|
||
| def _lazy_init_context(self) -> None: | ||
| """Initialize one shared runtime context for this FSDP root subtree. | ||
|
|
||
| MFSDP v2 requires users to apply ``fully_shard`` bottom-up, so child FSDP | ||
| modules are constructed before their eventual root module is constructed. | ||
| This method resolves the root lazily on the first forward through the | ||
| outermost FSDP module and shares that one context with every FSDP | ||
| descendant. | ||
|
|
||
| Alternatives considered: | ||
| - Eagerly initialize contexts during ``fully_shard``. When a parent is | ||
| sharded, we could create a new root context and reassign it to all | ||
| descendant FSDP modules. This creates transient child contexts that are | ||
| never used if the parent is later sharded, and each parent shard must | ||
| walk its descendants again, making nested sharding quadratic. | ||
| - Store an ``is_root`` field on each FSDP module. ``fully_shard`` could | ||
| mark newly sharded modules as roots and clear that flag on descendant | ||
| FSDP modules when a parent is sharded. This avoids creating unused | ||
| contexts but moves root tracking onto every FSDP module, adding | ||
| per-module state that must stay consistent with the final sharded | ||
| module hierarchy. | ||
| """ | ||
| if self._context is not None: | ||
| return | ||
|
|
||
| root_module = cast(nn.Module, self) | ||
| first_parameter = next(root_module.parameters(), None) | ||
| if first_parameter is None: | ||
| raise RuntimeError("FSDP root module requires at least one parameter in its subtree.") | ||
|
|
||
| context = FsdpContext(device=first_parameter.device, root_module=self) | ||
| # named_modules() yields FsdpModules in registration order, which is the static | ||
| # forward execution order used to prefetch the next FsdpModule's all-gather. | ||
| for submodule_name, submodule in root_module.named_modules(): | ||
| if not isinstance(submodule, FsdpModule): | ||
| continue | ||
| if submodule._context is not None: | ||
| raise RuntimeError( | ||
| "FSDP context is already initialized for a descendant module. " | ||
| "Run forward through the root FSDP module first." | ||
| ) | ||
| submodule._context = context | ||
| submodule._name = submodule_name | ||
| context.forward_order.append(submodule) | ||
|
|
||
| # Backward starts from the root pre-backward hook before visiting child | ||
| # subtrees in reverse module order. | ||
| _collect_backward_order(root_module, context.backward_order) | ||
| context.register_module(self) | ||
|
|
||
| @property | ||
| def context(self) -> FsdpContext: | ||
| """Return the initialized runtime context.""" | ||
| assert self._context is not None | ||
| """Return the FSDP context.""" | ||
| return self._context | ||
|
|
||
| @property | ||
|
|
@@ -189,8 +183,8 @@ def name(self) -> str: | |
| return name | ||
|
|
||
| def is_root(self) -> bool: | ||
| """Return whether this module is the outermost FsdpModule in its context.""" | ||
| return self.context.root_module is self | ||
| """Return whether this module is an outermost FsdpModule in its context.""" | ||
| return self._is_root | ||
|
|
||
| def _register_hooks(self) -> None: | ||
| module = cast(nn.Module, self) | ||
|
|
@@ -227,10 +221,10 @@ def pre_forward(self) -> None: | |
| While this FsdpModule computes, we issue the next FsdpModule's all-gather | ||
| on the comm stream, so ``AG_{i+1}`` is launched before ``F_i`` finishes. | ||
| """ | ||
| self._lazy_init_context() | ||
| context = self.context | ||
| context.ensure_finalized() | ||
| torch.cuda.nvtx.range_push(self._nvtx_label("forward")) | ||
| self._ready_grad_parameters.clear() | ||
| context = self.context | ||
| allgather_stream = context.allgather_stream | ||
| current_stream = context.current_stream() | ||
|
|
||
|
|
@@ -348,8 +342,19 @@ def _nvtx_label(self, phase: Literal["forward", "backward"]) -> str: | |
| return f"MFSDP {name} {phase}" | ||
|
|
||
|
|
||
| def _collect_fsdp_modules_under(module: nn.Module, modules: set["FsdpModule"]) -> None: | ||
| """Collect FSDP modules reachable from ``module``.""" | ||
| if isinstance(module, FsdpModule): | ||
| if module in modules: | ||
| return | ||
| modules.add(module) | ||
|
|
||
| for child in module.children(): | ||
| _collect_fsdp_modules_under(child, modules) | ||
|
|
||
|
|
||
| def _collect_backward_order(module: nn.Module, order: IndexedOrder["FsdpModule"]) -> None: | ||
| """Collect FsdpModules in static backward prefetch order.""" | ||
| """Collect one root's static backward prefetch order.""" | ||
| if isinstance(module, FsdpModule): | ||
| order.append(module) | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of silent misbehavior:
Maybe worth adding a guard or UT that detects when an FsdpModule being registered already belongs to another context, and raises explicitly.
Also, If we require all
fully_shardcalls to live under a single continuous fully_shard_context, does that make it harder to align with the FSDP2 API? (I recall the plan was to achieve compatibility via a wrapper)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Good idea.