Skip to content

[ONNX] Add symbolic function for XSoftmax op for exporting to ONNX.#14013

Merged
LysandreJik merged 3 commits intohuggingface:masterfrom
fatcat-z:add_xsoftmax_symbolic
Oct 26, 2021
Merged

[ONNX] Add symbolic function for XSoftmax op for exporting to ONNX.#14013
LysandreJik merged 3 commits intohuggingface:masterfrom
fatcat-z:add_xsoftmax_symbolic

Conversation

@fatcat-z
Copy link
Copy Markdown
Contributor

@fatcat-z fatcat-z commented Oct 15, 2021

What does this PR do?

For such customized operator, PyTorch ONNX exporter will try to call its symbolic() function for exporting to ONNX file. Add the implementation here so that it could be exported successfully.

@LysandreJik
Copy link
Copy Markdown
Member

LysandreJik commented Oct 16, 2021

Might be of interest to @BigBird01

@BigBird01
Copy link
Copy Markdown
Contributor

BigBird01 commented Oct 16, 2021

Might be of interest to @BigBird01

This is cool! Thanks!

@fatcat-z
Copy link
Copy Markdown
Contributor Author

fatcat-z commented Oct 25, 2021

@BigBird01
The latest CI failure seems like is not relative to my changes. Could you please take a review and see if we can merge this change?

Thanks.

michaelbenayoun
michaelbenayoun approved these changes Oct 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@michaelbenayoun michaelbenayoun left a comment

Choose a reason for hiding this comment

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

Seems good to me, thanks for your great contribution!

Quick question: do you plan to add the ONNX DeBERTa export support now that you have added support for the problematic operator?

@fatcat-z
Copy link
Copy Markdown
Contributor Author

fatcat-z commented Oct 26, 2021

Seems good to me, thanks for your great contribution!

Quick question: do you plan to add the ONNX DeBERTa export support now that you have added support for the problematic operator?

No, no plan for this yet.

LysandreJik
LysandreJik approved these changes Oct 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Yes, LGTM! Thank you @fatcat-z

@LysandreJik LysandreJik merged commit 3f23634 into huggingface:master Oct 26, 2021
@fatcat-z fatcat-z deleted the add_xsoftmax_symbolic branch October 27, 2021 02:11
@pommedeterresautee
Copy link
Copy Markdown
Contributor

pommedeterresautee commented Dec 21, 2021

@fatcat-z Hi, I have a question about the way symbolic conversion is implemented. Is there any reason why you are using uint8 type for some values?
It seems to me to work with signed integers, and uint8 is not supported by TensorRT (it only supports signed integers).

@pommedeterresautee pommedeterresautee mentioned this pull request Dec 26, 2021
Open
@JingyaHuang JingyaHuang mentioned this pull request Jun 10, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelbenayoun michaelbenayoun michaelbenayoun approved these changes

@LysandreJik LysandreJik LysandreJik approved these changes

@mfuntowicz mfuntowicz Awaiting requested review from mfuntowicz

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fatcat-z @LysandreJik @BigBird01 @pommedeterresautee @michaelbenayoun