Skip to content
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

Experimental prototype for converting torch.jit.trace modules to export #124449

Closed

Conversation

tugsbayasgalan
Copy link
Contributor

@tugsbayasgalan tugsbayasgalan commented Apr 19, 2024

Stack from ghstack (oldest at bottom):

Differential Revision: D56440613

We want to do this for following reasons:

  1. There is current limitation in export tracing for torch.jit.trace d modules that cannot be easily upstreamed
  2. We need to run internal CI regularly to understand feature gaps and continuously track them
  3. Multiple people will be working on this prototype so it is better to have a checked in version so we don't always run into merge conflicts.

cc @ezyang @msaroufim @bdhirsh @anijain2305 @chauhang @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng

Copy link

pytorch-bot bot commented Apr 19, 2024

🔗 Helpful Links

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

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

✅ You can merge normally! (6 Unrelated Failures)

As of commit 6828070 with merge base 92eb173 (image):

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

UNSTABLE - The following jobs failed but were likely due to flakiness present on trunk and has been marked as unstable:

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

@tugsbayasgalan
Copy link
Contributor Author

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

3 similar comments
@tugsbayasgalan
Copy link
Contributor Author

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

@tugsbayasgalan
Copy link
Contributor Author

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

@tugsbayasgalan
Copy link
Contributor Author

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

angelayi pushed a commit that referenced this pull request Apr 25, 2024
…gnature

ghstack-source-id: 3aaf2a3f3ba4638aef5ce2101e8b22a8a57f0bc8
Pull Request resolved: #124449
angelayi pushed a commit that referenced this pull request Apr 25, 2024
…gnature

ghstack-source-id: 3aaf2a3f3ba4638aef5ce2101e8b22a8a57f0bc8
Pull Request resolved: #124449
@facebook-github-bot
Copy link
Contributor

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

tugsbayasgalan added a commit that referenced this pull request May 15, 2024
…gnature

Pull Request resolved: #124449

@imported-using-ghimport

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613/)
ghstack-source-id: d823a86cff475c1785f140140b49f548a8b921f9

Some update

Pull Request resolved: #124450

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

@imported-using-ghimport

Differential Revision: [D56440611](https://our.internmc.facebook.com/intern/diff/D56440611/)
ghstack-source-id: d823a86cff475c1785f140140b49f548a8b921f9

Make CI less noisy

Pull Request resolved: #124664

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

@imported-using-ghimport

Differential Revision: [D56440612](https://our.internmc.facebook.com/intern/diff/D56440612/)
ghstack-source-id: d823a86cff475c1785f140140b49f548a8b921f9
@tugsbayasgalan tugsbayasgalan changed the title Hacks to work around the fact that ScriptMethod does not have code/signature Experimental prototype for converting torch.jit.trace modules to export May 15, 2024
…les to export"

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613)

cc ezyang msaroufim bdhirsh anijain2305 chauhang voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng

[ghstack-poisoned]
tugsbayasgalan added a commit that referenced this pull request May 15, 2024
…gnature

Pull Request resolved: #124449

@imported-using-ghimport

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613/)
ghstack-source-id: 39182d5d313da5d4d0b6b30fa8e94d974499276b

Some update

Pull Request resolved: #124450

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

@imported-using-ghimport

Differential Revision: [D56440611](https://our.internmc.facebook.com/intern/diff/D56440611/)
ghstack-source-id: 39182d5d313da5d4d0b6b30fa8e94d974499276b

Make CI less noisy

Pull Request resolved: #124664

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

@imported-using-ghimport

Differential Revision: [D56440612](https://our.internmc.facebook.com/intern/diff/D56440612/)
ghstack-source-id: 39182d5d313da5d4d0b6b30fa8e94d974499276b
…les to export"

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613)

cc ezyang msaroufim bdhirsh anijain2305 chauhang voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng

[ghstack-poisoned]
tugsbayasgalan added a commit that referenced this pull request May 15, 2024
…gnature

Pull Request resolved: #124449

@imported-using-ghimport

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613/)
ghstack-source-id: 44bafb981fef6340a542a5abf8c27fe186a91c8b

Some update

Pull Request resolved: #124450

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

@imported-using-ghimport

Differential Revision: [D56440611](https://our.internmc.facebook.com/intern/diff/D56440611/)
ghstack-source-id: 44bafb981fef6340a542a5abf8c27fe186a91c8b

Make CI less noisy

Pull Request resolved: #124664

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

@imported-using-ghimport

Differential Revision: [D56440612](https://our.internmc.facebook.com/intern/diff/D56440612/)
ghstack-source-id: 44bafb981fef6340a542a5abf8c27fe186a91c8b
…les to export"


Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613)

We want to do this for following reasons:
1. There is current limitation in export tracing for torch.jit.trace d modules that cannot be easily upstreamed
2. We need to run internal CI regularly to understand feature gaps and continuously track them
3. Multiple people will be working on this prototype so it is better to have a checked in version so we don't always run into merge conflicts. 

cc ezyang msaroufim bdhirsh anijain2305 chauhang voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng

[ghstack-poisoned]
tugsbayasgalan added a commit that referenced this pull request May 15, 2024
…gnature

Pull Request resolved: #124449

@imported-using-ghimport

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613/)
ghstack-source-id: 550e93f774b6f6aa665eee31ff42c15c80907cfb

Some update

Pull Request resolved: #124450

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

@imported-using-ghimport

Differential Revision: [D56440611](https://our.internmc.facebook.com/intern/diff/D56440611/)
ghstack-source-id: 550e93f774b6f6aa665eee31ff42c15c80907cfb

Make CI less noisy

Pull Request resolved: #124664

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

@imported-using-ghimport

Differential Revision: [D56440612](https://our.internmc.facebook.com/intern/diff/D56440612/)
ghstack-source-id: 550e93f774b6f6aa665eee31ff42c15c80907cfb
@tugsbayasgalan
Copy link
Contributor Author

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

…les to export"


Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613)

We want to do this for following reasons:
1. There is current limitation in export tracing for torch.jit.trace d modules that cannot be easily upstreamed
2. We need to run internal CI regularly to understand feature gaps and continuously track them
3. Multiple people will be working on this prototype so it is better to have a checked in version so we don't always run into merge conflicts. 

cc ezyang msaroufim bdhirsh anijain2305 chauhang voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng

[ghstack-poisoned]
tugsbayasgalan added a commit that referenced this pull request May 15, 2024
…gnature

Pull Request resolved: #124449

@imported-using-ghimport

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613/)
ghstack-source-id: c5a0b72a295f371bdb1e0e55601e867c5ec41962

Some update

Pull Request resolved: #124450

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

@imported-using-ghimport

Differential Revision: [D56440611](https://our.internmc.facebook.com/intern/diff/D56440611/)
ghstack-source-id: c5a0b72a295f371bdb1e0e55601e867c5ec41962

Make CI less noisy

Pull Request resolved: #124664

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

@imported-using-ghimport

Differential Revision: [D56440612](https://our.internmc.facebook.com/intern/diff/D56440612/)
ghstack-source-id: c5a0b72a295f371bdb1e0e55601e867c5ec41962
@tugsbayasgalan
Copy link
Contributor Author

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

@@ -153,11 +148,22 @@ def make_fake_inputs(nn_module, args, kwargs, dynamic_shapes):
len(constraints) == 0
), "Found constraints when tracing with a toplevel tracing context."
fake_mode = context.fake_mode
else:
elif not _is_torch_jit_trace:
Copy link
Contributor

@angelayi angelayi May 16, 2024

Choose a reason for hiding this comment

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

can we leave a comment like "HACK" for all the workarounds made in this PR

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 16, 2024
…les to export"


Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613)

We want to do this for following reasons:
1. There is current limitation in export tracing for torch.jit.trace d modules that cannot be easily upstreamed
2. We need to run internal CI regularly to understand feature gaps and continuously track them
3. Multiple people will be working on this prototype so it is better to have a checked in version so we don't always run into merge conflicts. 

cc ezyang msaroufim bdhirsh anijain2305 chauhang voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng

[ghstack-poisoned]
tugsbayasgalan added a commit that referenced this pull request May 16, 2024
…gnature

Pull Request resolved: #124449

@imported-using-ghimport

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613/)
ghstack-source-id: c056aa3cce08a23bf3271fd5a27f8c94baf109fa

Some update

Pull Request resolved: #124450

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

@imported-using-ghimport

Differential Revision: [D56440611](https://our.internmc.facebook.com/intern/diff/D56440611/)
ghstack-source-id: c056aa3cce08a23bf3271fd5a27f8c94baf109fa

Make CI less noisy

Pull Request resolved: #124664

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

@imported-using-ghimport

Differential Revision: [D56440612](https://our.internmc.facebook.com/intern/diff/D56440612/)
ghstack-source-id: c056aa3cce08a23bf3271fd5a27f8c94baf109fa
@tugsbayasgalan
Copy link
Contributor Author

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

)
else:
# FIXME(ycao): This is a hack to get around missing signature from ScriptMethod
msg = "dummy constraint violation message"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if it's better to just have msg = "" here, analogous to other zero-d out things in this mode. It might even be better to have a top-level dict of all the things we're zero-ing out, something like _torch_jit_trace_hacks = {"msg": "", "original_signature": None, ...} etc. so that we have a good account of all the FIXMEs.

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but 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

ZelboK pushed a commit to ZelboK/pytorch that referenced this pull request May 19, 2024
…rt (pytorch#124449)

Differential Revision: [D56440613](https://our.internmc.facebook.com/intern/diff/D56440613)

We want to do this for following reasons:
1. There is current limitation in export tracing for torch.jit.trace d modules that cannot be easily upstreamed
2. We need to run internal CI regularly to understand feature gaps and continuously track them
3. Multiple people will be working on this prototype so it is better to have a checked in version so we don't always run into merge conflicts.

Pull Request resolved: pytorch#124449
Approved by: https://github.com/angelayi, https://github.com/avikchaudhuri
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants