KEMBAR78
[export] fix unbacked range deserialization by pianpwk · Pull Request #158681 · pytorch/pytorch · GitHub
Skip to content

[export] fix unbacked range deserialization #158681

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[export] fix unbacked range deserialization #158681

wants to merge 1 commit into from

Conversation

@pianpwk
Copy link
Contributor

@pianpwk pianpwk commented Jul 18, 2025 •
edited
Loading

Fixes #151809, by reading shape assertion nodes into ShapeEnv, and deferring instantiation of node example values, to be done node-by-node.

Differential Revision: D78588406

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 18, 2025 •
edited
Loading

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit ff45ba1 with merge base 2a249f1 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78588406

@pianpwk pianpwk marked this pull request as draft July 18, 2025 21:42
facebook-github-bot pushed a commit that referenced this pull request Jul 21, 2025
@pianpwk @facebook-github-bot
[export] range deserialization (#158681)
81fd85d 
Summary:



Rollback Plan:

Differential Revision: D78588406
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78588406

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 21, 2025
@facebook-github-bot
Copy link
Contributor

@pianpwk has imported this pull request. If you are a Meta employee, you can view this in D78588406.

pianpwk added a commit that referenced this pull request Jul 21, 2025
@pianpwk @facebook-github-bot
[export] range deserialization (#158681)
1ba6924 
Summary: Pull Request resolved: #158681

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Differential Revision: D78588406

Pulled By: pianpwk
@pianpwk pianpwk force-pushed the export-D78588406 branch from 0756a90 to 1ba6924 Compare July 21, 2025 22:17
pianpwk added a commit that referenced this pull request Jul 21, 2025
@pianpwk @facebook-github-bot
[export] range deserialization (#158681)
072c365 
Summary: Pull Request resolved: #158681

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Differential Revision: D78588406

Pulled By: pianpwk
@pianpwk pianpwk force-pushed the export-D78588406 branch from 1ba6924 to 072c365 Compare July 21, 2025 22:18
facebook-github-bot pushed a commit that referenced this pull request Jul 21, 2025
@pianpwk @facebook-github-bot
[export] range deserialization (#158681)
905a61f 
Summary:
Fixes #151809, by reading shape assertion nodes into ShapeEnv, and deferring instantiation of node example values, to be done node-by-node.


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Differential Revision: D78588406

Pulled By: pianpwk
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78588406

@pianpwk pianpwk changed the title [export] range deserialization [export] fix unbacked range deserialization Jul 21, 2025
@pianpwk pianpwk marked this pull request as ready for review July 21, 2025 22:23
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78588406

pianpwk added a commit that referenced this pull request Jul 21, 2025
@pianpwk @facebook-github-bot
[export] range deserialization (#158681)
014e69d 
Summary: Pull Request resolved: #158681

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Differential Revision: D78588406

Pulled By: pianpwk
@pianpwk pianpwk force-pushed the export-D78588406 branch from 905a61f to 014e69d Compare July 21, 2025 22:23
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78588406

pianpwk added a commit that referenced this pull request Jul 21, 2025
@pianpwk @facebook-github-bot
[export] fix unbacked range deserialization (#158681)
c9fb16e 
Summary:
Fixes #151809, by reading shape assertion nodes into ShapeEnv, and deferring instantiation of node example values, to be done node-by-node.

Pull Request resolved: #158681

Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Differential Revision: D78588406

Pulled By: pianpwk
@pianpwk pianpwk force-pushed the export-D78588406 branch from 014e69d to c9fb16e Compare July 21, 2025 22:51
avikchaudhuri
avikchaudhuri approved these changes Jul 22, 2025
View reviewed changes
torch/_export/serde/serialize.py Outdated
meta_val = self.deserialize_tensor_meta(tensor_value)
log.debug("[deserialize_tensor_meta] %s (output): %s", name, meta_val)
self.serialized_name_to_meta[name] = meta_val
self.serialized_name_to_meta[name] = lambda v=tensor_value: self.deserialize_tensor_meta(v)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked Python semantics re: capture of tensor_value here? Hope it doesn't pick up values assigned later and instead does what you think it does...which is capture the value in this iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the default value here is for using the value at definition; doing lambda: self.deserialize_tensor_meta(tensor_value) was using the value at evaluation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Minor nit: could have used functools.partial as well I think for this purpose, the default value trick seems a bit obscure.

@pianpwk @facebook-github-bot
[export] fix unbacked range deserialization (#158681)
ff45ba1 
Summary:
Fixes #151809, by reading shape assertion nodes into ShapeEnv, and deferring instantiation of node example values, to be done node-by-node.


Test Plan:
Imported from GitHub, without a `Test Plan:` line.

Rollback Plan:

Reviewed By: avikchaudhuri

Differential Revision: D78588406

Pulled By: pianpwk
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D78588406

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorchmergebot
Copy link
Collaborator

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

@github-actions github-actions bot deleted the export-D78588406 branch August 23, 2025 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avikchaudhuri avikchaudhuri avikchaudhuri approved these changes

@ydwu4 ydwu4 ydwu4 approved these changes

@tugsbayasgalan tugsbayasgalan Awaiting requested review from tugsbayasgalan tugsbayasgalan is a code owner

@zhxchen17 zhxchen17 Awaiting requested review from zhxchen17 zhxchen17 is a code owner

@angelayi angelayi Awaiting requested review from angelayi angelayi is a code owner

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged release notes: export

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[export] deserialization for unbacked ranges is wrong

5 participants

@pianpwk @facebook-github-bot @pytorchmergebot @avikchaudhuri @ydwu4