[ONNX] Handle Gelu approximate attribute from Opset 20#18773
[ONNX] Handle Gelu approximate attribute from Opset 20#18773Mr-Neutr0n wants to merge 2 commits intoapache:mainfrom
Conversation
The ONNX frontend Gelu converter was hardcoded to always emit R.nn.gelu, ignoring the `approximate` attribute introduced in ONNX Opset 20. When a model specifies approximate="tanh", TVM would produce incorrect results by using the exact erf-based Gelu instead of the tanh approximation. Add _impl_v20 that checks the `approximate` attribute and routes to R.nn.gelu_tanh when approximate="tanh", falling back to the standard R.nn.gelu otherwise. Fixes apache#18750 Signed-off-by: Mr-Neutr0n <64578610+Mr-Neutr0n@users.noreply.github.com>
Summary of ChangesHello @Mr-Neutr0n, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the ONNX frontend where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly adds support for the approximate attribute in the ONNX Gelu operator for Opset 20. The implementation correctly dispatches to relax.op.nn.gelu_tanh when approximate="tanh".
I have one suggestion to make the implementation more robust by explicitly handling invalid values for the approximate attribute.
Additionally, while the change is straightforward, adding a dedicated test case to tests/python/relax/test_frontend_onnx.py would be beneficial to prevent future regressions. This test should cover both approximate="tanh" and approximate="none" for Opset 20.
| approximate = attr.get("approximate", "none") | ||
| if approximate == "tanh": | ||
| return relax.op.nn.gelu_tanh(inputs[0]) | ||
| return relax.op.nn.gelu(inputs[0]) |
There was a problem hiding this comment.
While the current implementation correctly handles the 'tanh' approximation, it silently falls back to the default 'erf' implementation for any other value of approximate besides 'tanh'. According to the ONNX specification for Gelu, the only valid values for approximate are 'tanh' and 'none'.
To make the implementation more robust and explicit, it would be better to raise an error for any unsupported values. This aligns with how other operators with string attributes are handled in this file (e.g., BitShift).
| approximate = attr.get("approximate", "none") | |
| if approximate == "tanh": | |
| return relax.op.nn.gelu_tanh(inputs[0]) | |
| return relax.op.nn.gelu(inputs[0]) | |
| approximate = attr.get("approximate", "none") | |
| if approximate == "tanh": | |
| return relax.op.nn.gelu_tanh(inputs[0]) | |
| elif approximate == "none": | |
| return relax.op.nn.gelu(inputs[0]) | |
| else: | |
| raise ValueError(f"Unsupported approximate mode for Gelu: {approximate}") |
There was a problem hiding this comment.
Please include testcase in
tvm/tests/python/relax/test_frontend_onnx.py
Line 922 in 84d05e9
Add test_gelu_approximate to verify that the Gelu converter correctly handles the approximate="tanh" and approximate="none" attributes introduced in ONNX Opset 20. Signed-off-by: Mr-Neutr0n <64578610+Mr-Neutr0n@users.noreply.github.com>
|
Thanks for the review @mshr-h! I've added |
mshr-h
left a comment
There was a problem hiding this comment.
Please fix the CI error. @Mr-Neutr0n
Description
Fixes #18750
The ONNX frontend
Geluconverter was hardcoded to always emitR.nn.gelu, ignoring theapproximateattribute introduced in ONNX Opset 20. When a model usesapproximate="tanh", TVM would silently produce incorrect results by applying the exact erf-based Gelu instead of the tanh approximation.Changes
Added
_impl_v20to theGeluconverter class that checks theapproximateattribute:approximate="tanh"→ routes torelax.op.nn.gelu_tanhapproximate="none"(default) → routes torelax.op.nn.geluThe existing
_impl_v1is left untouched for backward compatibility with the Microsoft onnxruntime contrib opset. The version dispatch mechanism will automatically select_impl_v20for Opset 20+ models.Testing
R.nn.gelu_tanh).