Skip to content

[export] Move PT2ArchiveWriter/Reader to torch/export #153795

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] Move PT2ArchiveWriter/Reader to torch/export #153795

wants to merge 1 commit into from

Conversation

angelayi
Copy link
Contributor

@angelayi angelayi commented May 18, 2025 •
edited by clee2000
Loading

Summary:
Before:
from sigmoid.core.package.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_sigmoid_package
After:
from torch.export.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_pt2_package

By merging the two PT2ArchiveReader/Writers, into using the native PytorchFileReader/Writer, the open source PT2 archive also changed to have an additional folder. However this PR still maintains support for loading an old PT2 archive which does not have the additional folder.

Before:

├── archive_format
├── byteorder
├── .data
│   ├── serialization_id
│   └── version
├── data
│   ├── aotinductor

After:

├── tmp
│   ├── archive_format
│   ├── byteorder
│   ├── .data
│   │   ├── serialization_id
│   │   └── version
│   ├── data
│   │   ├── aotinductor

Test Plan:
buck2 test //sigmoid/...
https://www.internalfb.com/intern/testinfra/testrun/5348024839248187

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov

@pytorch-bot PyTorch Bot
Copy link

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

🔗 Helpful Links

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

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

⏳ No Failures, 101 Pending

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

UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:

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: D74616598

@facebook-github-bot
Copy link
Contributor

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

@angelayi angelayi force-pushed the export-D74616598 branch from 8beb597 to 4ff51a7 Compare May 21, 2025 18:55
pytorch-bot bot pushed a commit that referenced this pull request May 21, 2025
@angelayi @facebook-github-bot
[export] Move PT2ArchiveWriter/Reader to torch/export (#153795)
4ff51a7 
Summary:
Pull Request resolved: #153795

Before:
`from sigmoid.core.package.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_sigmoid_package`
After:
`from torch.export.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_pt2_package`

Test Plan:
`buck2 test //sigmoid/...`
https://www.internalfb.com/intern/testinfra/testrun/5348024839248187

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

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

@angelayi angelayi force-pushed the export-D74616598 branch from 4ff51a7 to f6a0efe Compare May 21, 2025 22:26
angelayi added a commit to angelayi/pytorch that referenced this pull request May 21, 2025
@angelayi @facebook-github-bot
[export] Move PT2ArchiveWriter/Reader to torch/export (pytorch#153795)
f6a0efe 
Summary:
Pull Request resolved: pytorch#153795

Before:
`from sigmoid.core.package.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_sigmoid_package`
After:
`from torch.export.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_pt2_package`

Test Plan:
`buck2 test //sigmoid/...`
https://www.internalfb.com/intern/testinfra/testrun/5348024839248187

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

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

angelayi added a commit to angelayi/pytorch that referenced this pull request May 21, 2025
@angelayi @facebook-github-bot
[export] Move PT2ArchiveWriter/Reader to torch/export (pytorch#153795)
f1b21ad 
Summary:
Pull Request resolved: pytorch#153795

Before:
`from sigmoid.core.package.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_sigmoid_package`
After:
`from torch.export.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_pt2_package`

Test Plan:
`buck2 test //sigmoid/...`
https://www.internalfb.com/intern/testinfra/testrun/5348024839248187

Differential Revision: D74616598
@angelayi angelayi force-pushed the export-D74616598 branch from f6a0efe to f1b21ad Compare May 21, 2025 22:36
@angelayi angelayi requested review from desertfire and yushangdi May 21, 2025 23:29
@facebook-github-bot
Copy link
Contributor

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

@angelayi angelayi force-pushed the export-D74616598 branch from f1b21ad to 2d1b5f3 Compare May 21, 2025 23:32
angelayi added a commit to angelayi/pytorch that referenced this pull request May 21, 2025
@angelayi @facebook-github-bot
[export] Move PT2ArchiveWriter/Reader to torch/export (pytorch#153795)
2d1b5f3 
Summary:
Pull Request resolved: pytorch#153795

Before:
`from sigmoid.core.package.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_sigmoid_package`
After:
`from torch.export.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_pt2_package`

Test Plan:
`buck2 test //sigmoid/...`
https://www.internalfb.com/intern/testinfra/testrun/5348024839248187

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

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

@angelayi angelayi force-pushed the export-D74616598 branch from 2d1b5f3 to 4152fa7 Compare May 22, 2025 14:05
@angelayi angelayi requested a review from albanD as a code owner May 22, 2025 14:05
angelayi added a commit to angelayi/pytorch that referenced this pull request May 22, 2025
@angelayi @facebook-github-bot
[export] Move PT2ArchiveWriter/Reader to torch/export (pytorch#153795)
4152fa7 
Summary:
Pull Request resolved: pytorch#153795

Before:
`from sigmoid.core.package.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_sigmoid_package`
After:
`from torch.export.pt2_archive import PT2ArchiveWriter, PT2ArchiveReader, is_pt2_package`

Test Plan:
`buck2 test //sigmoid/...`
https://www.internalfb.com/intern/testinfra/testrun/5348024839248187

Differential Revision: D74616598
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: inductor-rocm / rocm-py3.10-inductor / test (inductor, 2, 2, linux.rocm.gpu.2)

Details for Dev Infra team Raised by workflow job

@angelayi
Copy link
Contributor Author

@pytorchbot merge -f "landed internally"

@pytorchmergebot
Copy link
Collaborator

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

@malfet
Copy link
Contributor

malfet commented May 23, 2025

@pytorchbot revert -m "Looks like it broke lots of tests, see https://hud.pytorch.org/hud/pytorch/pytorch/ec368a19036dfa42216b05216bc0400388aee50b/1?per_page=50&mergeEphemeralLF=true" -c nosignal

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@angelayi your PR has been successfully reverted.
⚠️ This PR might contain internal changes
cc: @pytorch/pytorch-dev-infra

pytorchmergebot added a commit that referenced this pull request May 23, 2025
@pytorchmergebot
Revert "[export] Move PT2ArchiveWriter/Reader to torch/export (#153795)"
4ff19ec 
This reverts commit 7e80f23.

Reverted #153795 on behalf of https://github.com/malfet due to Looks like it broke lots of tests, see https://hud.pytorch.org/hud/pytorch/pytorch/ec368a19036dfa42216b05216bc0400388aee50b/1?per_page=50&mergeEphemeralLF=true ([comment](#153795 (comment)))
@pytorchmergebot pytorchmergebot added Reverted ci-no-td Do not run TD on this PR labels May 23, 2025
@malfet
Copy link
Contributor

malfet commented May 23, 2025

I.e. looks like after this PR TestTorchbind::test_torchbind_aot_compile started to fail

@yushangdi
Copy link
Contributor

yushangdi commented May 23, 2025 •
edited
Loading

@malfet @angelayi I think we just need this simple fix in model_package_loader.cpp:
std::string const_directory = file_prefix + "data" + k_separator + "constants"; (D75308904). I can just land a forward fix? Looks like the internal Diff is not reverted yet.

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Comment with id 2905484431 not found

Details for Dev Infra team Raised by workflow job

@clee2000
Copy link
Contributor

@pytorchbot merge -f "will merge the forward fix after this"

@pytorchmergebot
Copy link
Collaborator

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

This was referenced May 29, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhxchen17 zhxchen17 zhxchen17 approved these changes

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

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

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

@desertfire desertfire Awaiting requested review from desertfire

@yushangdi yushangdi Awaiting requested review from yushangdi

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

Assignees
No one assigned
Labels
ci-no-td Do not run TD on this PR ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged module: inductor release notes: export release notes: inductor (aoti) Reverted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@angelayi @facebook-github-bot @pytorchmergebot @malfet @yushangdi @clee2000 @zhxchen17