[export] Serialize getattr nodes #107924

angelayi · 2023-08-25T02:42:16Z

Turns out some graphs will result in getattr nodes...so let's serialize them

pytorch-bot · 2023-08-25T02:42:18Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/107924

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit be3d23e with merge base b4c6c4d ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

zhxchen17

Overall I think it's reasonable to have get_attr in the graph. In fact I forgot this corner case before.

torch/_export/serde/serialize.py

gmagogsfm · 2023-08-25T08:11:10Z

torch/_export/serde/schema.py

@@ -250,6 +250,7 @@ class GraphModule:
    # TODO(zhxchen17) Merge call_spec into call graph.
    call_spec: CallSpec
    module_call_graph: List[ModuleCallEntry]
+    constants: str


Do we need to bump schema version?

I was thinking no because we haven't published things yet :P but I would bump it if a schema change was made after today.

OK so you're going to store attributes that are tensors here. Didn't we have a general place to store tensors by reference in the IR as well?

facebook-github-bot · 2023-08-25T16:06:50Z

@angelayi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

avikchaudhuri

Looks good, few questions.

avikchaudhuri · 2023-08-25T16:58:09Z

torch/_export/serde/serialize.py

+                elif isinstance(attr, torch.fx.GraphModule):
+                    with self.save_graph_state():
+                        graph = self.serialize_graph(attr)
+                    return Argument.create(as_graph=GraphArgument(name=arg.target, graph=graph))


I wonder if in the future it would make sense to store graphs by name as well.

I think either way is fine, but graph arguments are more consistent with fx.GraphModule and other existing format (e.g. onnx).

torch/_export/serde/serialize.py

avikchaudhuri · 2023-08-25T17:08:54Z

torch/_export/serde/schema.py

@@ -250,6 +250,7 @@ class GraphModule:
    # TODO(zhxchen17) Merge call_spec into call graph.
    call_spec: CallSpec
    module_call_graph: List[ModuleCallEntry]
+    constants: str


OK so you're going to store attributes that are tensors here. Didn't we have a general place to store tensors by reference in the IR as well?

torch/_export/serde/schema.py

torch/_export/serde/serialize.py

torch/_export/serde/schema.py

SherlockNoMad

Sorry, I would need to block this diff.
Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.

cc @suo, @muchulee8

gmagogsfm · 2023-08-25T18:57:47Z

Sorry, I would need to block this diff. Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.

cc @suo, @muchulee8

The serialization format is private implementation detail hidden behind torch.export.save/load API, so we have freedom to evolve it.

Since there is no support for serializing or deserializing constant tensors like these yet, this change doesn't make the situation any worse than what it is today (no support at all).

Can we land this solution (torch.save-based) now so that torch.export.save() doesn't silently lose constant tensors (a severe bug) today to make 2.1 branchcut? Then we can immediately evolve it in a BC-compatible way.

SherlockNoMad · 2023-08-25T19:08:53Z

Sorry, I would need to block this diff. Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.
cc @suo, @muchulee8

Sorry, I would need to block this diff. Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.
cc @suo, @muchulee8

The serialization format is private implementation detail hidden behind torch.export.save/load API, so we have freedom to evolve it.

Since there is no support for serializing or deserializing constant tensors like these yet, this change doesn't make the situation any worse than what it is today (no support at all).

Can we land this solution (torch.save-based) now so that torch.export.save() doesn't silently lose constant tensors (a severe bug) today to make 2.1 branchcut? Then we can immediately evolve it in a BC-compatible way.

Ok, but we need to make it clear that we DOES NOT have any BC guarantee on this exported version of the weights! My concern is that once this weight format is leaked into production service, we will be tied to it, which becomes very hard to change!

Notice that this is a different problem from the state_dict serialization! Weights in state_dict serialization and deserialization is not specific in the PT2 IR. But this constant tensor serialization/deserialization is part of the IR spec.

gmagogsfm · 2023-08-25T19:25:26Z

Sorry, I would need to block this diff. Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.
cc @suo, @muchulee8

Sorry, I would need to block this diff. Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.
cc @suo, @muchulee8

The serialization format is private implementation detail hidden behind torch.export.save/load API, so we have freedom to evolve it.
Since there is no support for serializing or deserializing constant tensors like these yet, this change doesn't make the situation any worse than what it is today (no support at all).
Can we land this solution (torch.save-based) now so that torch.export.save() doesn't silently lose constant tensors (a severe bug) today to make 2.1 branchcut? Then we can immediately evolve it in a BC-compatible way.

Ok, but we need to make it clear that we DOES NOT have any BC guarantee on this exported version of the weights! My concern is that once this weight format is leaked into production service, we will be tied to it, which becomes very hard to change!

Notice that this is a different problem from the state_dict serialization! Weights in state_dict serialization and deserialization is not specific in the PT2 IR. But this constant tensor serialization/deserialization is part of the IR spec.

Yeah, we made some scary warnings in the pytorch doc

pytorch/torch/export/__init__.py

Line 1120 in c68d0a7

.. warning::

SherlockNoMad · 2023-08-25T19:42:38Z

Sorry, I would need to block this diff. Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.
cc @suo, @muchulee8

Sorry, I would need to block this diff. Let's consider Tensor serialization holistically with AOTInductor, and Model Processing workstream.
cc @suo, @muchulee8

The serialization format is private implementation detail hidden behind torch.export.save/load API, so we have freedom to evolve it.
Since there is no support for serializing or deserializing constant tensors like these yet, this change doesn't make the situation any worse than what it is today (no support at all).
Can we land this solution (torch.save-based) now so that torch.export.save() doesn't silently lose constant tensors (a severe bug) today to make 2.1 branchcut? Then we can immediately evolve it in a BC-compatible way.

Ok, but we need to make it clear that we DOES NOT have any BC guarantee on this exported version of the weights! My concern is that once this weight format is leaked into production service, we will be tied to it, which becomes very hard to change!
Notice that this is a different problem from the state_dict serialization! Weights in state_dict serialization and deserialization is not specific in the PT2 IR. But this constant tensor serialization/deserialization is part of the IR spec.

Yeah, we made some scary warnings in the pytorch doc

pytorch/torch/export/__init__.py

Line 1120 in c68d0a7

.. warning::

Ok, I removed the block. Let's come up with a proper solution soon.

gmagogsfm · 2023-08-25T20:01:24Z

Ok, I removed the block. Let's come up with a proper solution soon.

Yep, let's do it next week
@angelayi

SherlockNoMad · 2023-08-25T21:25:13Z

Another plausible solution is to lift such constant weights as graph inputs, so they can be handle in the same way as weights/buffers.

discussed in comment, agreed to proceed

comment resolved, reviewer out of work hour, need to land soon

angelayi · 2023-08-25T23:25:43Z

@pytorchbot merge

pytorchmergebot · 2023-08-25T23:28:10Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2023-08-25T23:28:14Z

Merge failed

Reason: 1 jobs have failed, first few of them are: Meta Internal-Only Changes Check

Details for Dev Infra team

Raised by workflow job

angelayi · 2023-08-26T02:39:49Z

@pytorchbot merge -f "only failure is Meta Internal-Only Changes Check, which is an infra failure"

pytorchmergebot · 2023-08-26T02:41:42Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Turns out some graphs will result in getattr nodes...so let's serialize them Pull Request resolved: #107924 Approved by: https://github.com/zhxchen17, https://github.com/avikchaudhuri

angelayi requested review from gmagogsfm, zhxchen17 and avikchaudhuri August 25, 2023 02:42

github-actions bot added the module: export label Aug 25, 2023

angelayi added the release notes: export label Aug 25, 2023

zhxchen17 requested changes Aug 25, 2023

View reviewed changes

torch/_export/serde/serialize.py Outdated Show resolved Hide resolved

angelayi requested a review from zhxchen17 August 25, 2023 05:32

gmagogsfm reviewed Aug 25, 2023

View reviewed changes

angelayi added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 25, 2023

angelayi requested a review from gmagogsfm August 25, 2023 15:49

angelayi added 2 commits August 25, 2023 09:34

[export] Serialize getattr nodes

91130b2

add constants to schema

18025e7

angelayi force-pushed the angelayi/getattr branch from 9ff84ed to 18025e7 Compare August 25, 2023 16:34

avikchaudhuri approved these changes Aug 25, 2023

View reviewed changes

zhxchen17 previously requested changes Aug 25, 2023

View reviewed changes

torch/_export/serde/schema.py Outdated Show resolved Hide resolved

torch/_export/serde/serialize.py Outdated Show resolved Hide resolved

SherlockNoMad reviewed Aug 25, 2023

View reviewed changes

torch/_export/serde/schema.py Outdated Show resolved Hide resolved

SherlockNoMad previously requested changes Aug 25, 2023

View reviewed changes

SherlockNoMad self-requested a review August 25, 2023 19:40

serialize to Dict[str, bytes]

edf6c91

angelayi requested a review from zhxchen17 August 25, 2023 21:13

idk what that line was for

be3d23e

pytorchmergebot added the merging label Aug 25, 2023

pytorchmergebot removed the merging label Aug 25, 2023

zhxchen17 approved these changes Aug 26, 2023

View reviewed changes

pytorchmergebot added the merging label Aug 26, 2023

pytorchmergebot added Merged and removed merging labels Aug 26, 2023

pytorchmergebot closed this in 4e9d7f8 Aug 26, 2023

github-actions bot deleted the angelayi/getattr branch February 24, 2025 02:06

[export] Serialize getattr nodes #107924

[export] Serialize getattr nodes #107924

Uh oh!

Conversation

angelayi commented Aug 25, 2023

Uh oh!

pytorch-bot bot commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/107924

✅ No Failures

Uh oh!

zhxchen17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmagogsfm Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

angelayi Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avikchaudhuri Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 25, 2023

Uh oh!

avikchaudhuri left a comment

Choose a reason for hiding this comment

Uh oh!

avikchaudhuri Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

zhxchen17 Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avikchaudhuri Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SherlockNoMad left a comment

Choose a reason for hiding this comment

Uh oh!

gmagogsfm commented Aug 25, 2023

Uh oh!

SherlockNoMad commented Aug 25, 2023

Uh oh!

gmagogsfm commented Aug 25, 2023

Uh oh!

SherlockNoMad commented Aug 25, 2023

Uh oh!

gmagogsfm commented Aug 25, 2023

Uh oh!

SherlockNoMad commented Aug 25, 2023

Uh oh!

angelayi commented Aug 25, 2023

Uh oh!

pytorchmergebot commented Aug 25, 2023

Merge started

Uh oh!

pytorchmergebot commented Aug 25, 2023

Merge failed

Uh oh!

angelayi commented Aug 26, 2023

Uh oh!

pytorchmergebot commented Aug 26, 2023

Merge started

Uh oh!

Uh oh!

pytorch-bot bot commented Aug 25, 2023 •

edited

Loading

angelayi Aug 25, 2023 •

edited

Loading