Skip to content

ETRecord ser/de handling "None" outputs and more #3191

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

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

pytorchbot
Copy link
Collaborator

Summary:
For the ease of communication, let me assign nicknames to the files related to this diff:

  • File A: caffe2/torch/_export/serde/serialize.py
  • File B: executorch/exir/serde/serialize.py
  • File C: executorch/exir/serde/export_serialize.py

Recently, we noticed that error torch._export.serde.serialize.SerializeError: Unable to deserialize output node Argument(as_none=[]) (P1210590561) was thrown from File B when deserializing ETRecord. It's possible that the error has been there since the beginning, but we've just never tested that logic path.

In this diff, I made a fix on File B to resolve this particular issue. Also adding handling for "None" output case in sdk logic. Keep on reading if you don't think the code changes make sense:

I explored the history of file changes. In chronological order:

  1. D48258552, deserialize_graph_output() was copied from File A to File B, with some modifications made. The deserialize_graph_output() in File B overrides that in File A due to polymorphism.
  2. D52446586, File C was created by copying File A. As a result of this diff, the deserialize_graph_output() in File B now overrides that in File C.
  3. Also in D52446586, the deserialize_graph_output() in File A had some significant changes; File C got the new version of deserialize_graph_output(). But this diff didn't update the deserialize_graph_output() in File B.
  4. D55391674 added the handling for "None" outputs to File A.

This diff brings (parts of) File C up-to-date with File A, and make deserialize_graph_output() in File B properly overrides that in File A.

In the future, we should figure out how to keep File C and File A in sync. Recently, File C was broken because it didn't stay in sync with File A in D54855251 and had to be fixed by D55776877.

Differential Revision: D56091104

Summary:
Pull Request resolved: #3039

For the ease of communication, let me assign nicknames to the files related to this diff:
* File A: *caffe2/torch/_export/serde/serialize.py*
* File B: *executorch/exir/serde/serialize.py*
* File C: *executorch/exir/serde/export_serialize.py*

Recently, we noticed that error `torch._export.serde.serialize.SerializeError: Unable to deserialize output node Argument(as_none=[])` (P1210590561) was thrown from File B when deserializing ETRecord. It's possible that the error has been there since the beginning, but we've just never tested that logic path.

In this diff, I made a fix on File B to resolve this particular issue. Also adding handling for "None" output case in sdk logic. ***Keep on reading if you don't think the code changes make sense:***

I explored the history of file changes. In chronological order:
1. D48258552, `deserialize_graph_output()` was copied from File A to File B, with some modifications made. The `deserialize_graph_output()` in File B overrides that in File A due to polymorphism.
2. D52446586, File C was created by ***copying*** File A. As a result of this diff, the `deserialize_graph_output()` in File B now overrides that in File C.
3. Also in D52446586, the `deserialize_graph_output()` in File A had some significant changes; File C got the new version of `deserialize_graph_output()`. But this diff didn't update the `deserialize_graph_output()` in File B.
4. D55391674 added the handling for "None" outputs to File A.

This diff brings (parts of) File C up-to-date with File A, and make `deserialize_graph_output()` in File B properly overrides that in File A.

In the future, we should figure out how to keep File C and File A in sync. Recently, File C was broken because it didn't stay in sync with File A in D54855251 and had to be fixed by D55776877. There will be a design review session this Friday to discuss consolidating the serialization code for edge and export.

Reviewed By: tarun292

Differential Revision: D56091104

fbshipit-source-id: 20c75ddc610c3be7ab2bb62943419d3b8b2be079
(cherry picked from commit 89cfa73)
Copy link

pytorch-bot bot commented Apr 21, 2024

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit 25b05c2 with merge base d3326a2 (image):

NEW FAILURE - The following job has failed:

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 21, 2024
@guangy10 guangy10 merged commit a94459a into release/0.2 Apr 22, 2024
34 of 35 checks passed
@mergennachin mergennachin mentioned this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants