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

[ONNX] Run old unittests with fx exporter #96479

Closed
wants to merge 9 commits into from

Conversation

BowenBao
Copy link
Collaborator

@BowenBao BowenBao commented Mar 10, 2023

@pytorch-bot
Copy link

pytorch-bot bot commented Mar 10, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit 465452b:
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Mar 10, 2023
BowenBao added a commit that referenced this pull request Mar 10, 2023
ghstack-source-id: 43dd979fa8f318498a056e66b3305d80bdd39f04
Pull Request resolved: #96479
BowenBao added a commit that referenced this pull request Mar 10, 2023
ghstack-source-id: 8142e854df36e3be6f56683dc1428722d5580f3e
Pull Request resolved: #96479
@justinchuby
Copy link
Collaborator

Love this!

@titaiwangms
Copy link
Collaborator

This is great!!!

@BowenBao BowenBao marked this pull request as ready for review March 15, 2023 00:46
@BowenBao BowenBao requested a review from abock as a code owner March 15, 2023 00:46
BowenBao added a commit to BowenBao/pytorch that referenced this pull request Mar 15, 2023
ghstack-source-id: 91869fa81df3600ea20d60d36fde119dabc2676a
Pull Request resolved: pytorch#96479

fix lint for unittest

amend unittest
script_model = model if is_model_script else torch.jit.script(model)
_run_test(
script_model,
scripting_remained_onnx_input_idx,
flatten=False,
ignore_none=False,
)
if not is_model_script and not self.is_script:
elif not is_model_script and not self.is_script and not self.is_fx:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move the elif self.is_fx on top of this and make this else? This is the tracing cases, right? I guess you think this is more explicit, but it actually seems more complicated. But not blocking though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be just me, but adding a comment would be better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleared the logic a bit.

@titaiwangms
Copy link
Collaborator

For those tests with dynamic_axes, I think we didn't test them properly. At least we should give additional test sets with different shape to prove it work. So it would be great if we can set additional test sets into fx test like in https://github.com/pytorch/pytorch/pull/96350/files. It's not blocking, if it's out of this PR scope, I can do it too. Also, I think it's worth to put the auto testing code up here? I figure we might need to use it a lot.

@BowenBao
Copy link
Collaborator Author

@titaiwangms I'll leave most for future PRs. I didn't include the auto script as it still requires a bit of manual process. I will consider adding as follow up when it's more mature.

titaiwangms added a commit that referenced this pull request Mar 22, 2023
…mbolic shape to ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
titaiwangms added a commit that referenced this pull request Mar 22, 2023
…ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
titaiwangms added a commit that referenced this pull request Mar 24, 2023
…mbolic shape to ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
titaiwangms added a commit that referenced this pull request Mar 24, 2023
…ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
titaiwangms added a commit that referenced this pull request Mar 24, 2023
…mbolic shape to ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
titaiwangms added a commit that referenced this pull request Mar 24, 2023
…ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
titaiwangms added a commit that referenced this pull request Mar 24, 2023
…mbolic shape to ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
titaiwangms added a commit that referenced this pull request Mar 24, 2023
…ONNX"


~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py

[ghstack-poisoned]
pytorchmergebot pushed a commit that referenced this pull request Mar 24, 2023
~~Need microsoft/onnxscript#484

Support dynamic export on fx-ONNX exporter. Essentially, we set inputs size and nodes all dynamic in torchscript, and leverage on `aten::sym_size` to catch dynamic size between each Op.

1. Add `dynamic_axes` switch between symbolic tracing (dynamic sizes) and fake mode (static). Set it to default True, as most of our tests are happy with sumbolic tracing. Except GPT2 stays with fake mode with error: microsoft/onnxscript#523
2. Add test_fx_dynamic_onnruntime.py to test on some addhoc we have from old exporter. This can be removed once tests are integrated with #96479
3. Since `aten::sym_size` are operated with built-in function, a built-in function mapping is added to support SymFloat/SymInt. (FIXME: #97201). sym_size output value is also fx.Node, and can be found in `fx_name_to_onnxscipt_value`, so it's operation stays the same as other ONNX ops in ONNX graph.
4. Fully deprecated FakeTensorProp as make_fx() should provide all node meta info.
5. Put complicated fx.Node related ArgumentType into _type_utils.py
Pull Request resolved: #96350
Approved by: https://github.com/wschin, https://github.com/justinchuby
@kit1980
Copy link
Member

kit1980 commented Apr 26, 2023

Should we merge this?

@BowenBao
Copy link
Collaborator Author

@kit1980 not in a hurry but will eventually do it. I'll convert it to draft as things must have been outdated for a while.

@BowenBao BowenBao marked this pull request as draft April 27, 2023 00:37
@github-actions
Copy link

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Jun 26, 2023
@github-actions github-actions bot closed this Jul 26, 2023
@facebook-github-bot facebook-github-bot deleted the gh/BowenBao/215/head branch August 25, 2023 14:16
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