-
Notifications
You must be signed in to change notification settings - Fork 86
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
Unflatten traced module #954
Conversation
@@ -23,19 +23,21 @@ | |||
class ExampleCode(torch.nn.Module): |
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.
can you add another test case inspired by torchtrain, where module has a ModuleList like this
Transformer
self.layers = torch.nn.ModuleList()
for layer_id in range(model_args.n_layers):
self.layers.append(TransformerBlock(layer_id, model_args))
What I am wondering is, how will you deal with keeping the FQNs the same after splitting?
if you put layers[0] on one stage and layers[1] on next stage,
will second stage still have layers[1] as FQN or will it drop to layers[0]?
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.
Thanks. Good idea.
The second stage will have "layers.1" as the FQN.
"layers" is from the self.layers = torch.nn.ModuleList()
tier.
"1" corresponds to the attribute within the ModuleList.
Those two are preserved from the original model.
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.
@wconstab I added test_transformer.py
.
You can see the updated PR description for the split structure.
|
||
|
||
# Add an alias for convenience | ||
aten_pipe_split_alias = torch.ops.pippy._pipe_split.default |
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.
curious, whats the purpose of the additional .default
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.
@tugsbayasgalan wondering if you know the answer?
Due to export's unflatten bug: pytorch/pytorch#123122
Status of this branch: Good:
Bad:
I consider that the "Bad" items are not really blocking errors, and since the branch is needed by torchtrain pytorch/torchtitan#161, I am merging this branch into main as is. |
Description
_export_to_torch_ir
to the officialtorch.export.export
Purpose of this PR is to:
a.b.c
to submodules to specify their policies._export_to_torch_ir
per Export Team's plan.Test
Added
test_transformer.py
.We split the model into two stages. Each stages would preserve the
layers.<i>
structure as in the original model.Caveat:
I temporarily disabled multi-use parameter support (aka. shared paramters or tied parameters). So some real examples may break. Will add the support back in next PR.