Skip to content

Revert "Add onnx export with verification"#6297

Closed
wyli wants to merge 1 commit intodevfrom
revert-6237-liqun/export-onnx
Closed

Revert "Add onnx export with verification"#6297
wyli wants to merge 1 commit intodevfrom
revert-6237-liqun/export-onnx

Conversation

@wyli
Copy link
Copy Markdown
Contributor

@wyli wyli commented Apr 5, 2023

Reverts #6237

@wyli wyli requested review from Nic-Ma and binliunls April 5, 2023 11:28
@ericspod
Copy link
Copy Markdown
Member

ericspod commented Apr 5, 2023

Hi @wyli, I see this and another PR reverting previous ones, what is the issue here?

@wyli wyli marked this pull request as draft April 5, 2023 13:41
@wyli
Copy link
Copy Markdown
Contributor Author

wyli commented Apr 5, 2023

Hi @wyli, I see this and another PR reverting previous ones, what is the issue here?

both PRs introduce integration errors...
#6294
#6296

and I don't know how to fix them properly. I'm trying to add some more premerge tests to avoid this in the future.

@ericspod
Copy link
Copy Markdown
Member

ericspod commented Apr 5, 2023

OK thanks @wyli, we can always re-revert these changes once we know the issue and we won't lose this work.

@binliunls
Copy link
Copy Markdown
Contributor

Hi @wyli , there is a DCO ci error currently. Could I directly commit on this PR and ignore this error?

Thanks,
Bin

@binliunls
Copy link
Copy Markdown
Contributor

Hi @liqunfu ,
I think the example_outputs is a legacy parameter of the torch.onnx.export in pytorch. Since you are the expert in onnx and wrote this unit test, could you please offer a help on this issue in the old version of pytorch?

Thank,
Bin

@Nic-Ma
Copy link
Copy Markdown
Contributor

Nic-Ma commented Apr 5, 2023

Hi @wyli @ericspod ,

I would suggest to avoid reverting this PR "Add onnx export with verification" if @liqunfu can help identify the integration test error you said.
Because this ONNX export feature is very important for our first milestone (TensorRT supported bundles) at April 17.

Thanks.

@wyli wyli closed this Apr 5, 2023
@wyli wyli deleted the revert-6237-liqun/export-onnx branch April 5, 2023 15:49
@liqunfu
Copy link
Copy Markdown
Contributor

liqunfu commented Apr 5, 2023

thanks @binliunls for pointing out the legacy of example_outputs!
I created a PR to support PyTorch version < 1.10. #6309

wyli pushed a commit that referenced this pull request Apr 6, 2023
…6309)

Fixes # .
#6297

### Description

PyTorch onnx exporter API has been changed since 1.10 to remove
example_outputs as a required input argument. Special handling is added
in this PR to support PyTorch version older than 1.10. Unit test is also
extended to covert this case.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: Liqun Fu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants