[ONNX] Relax unsupported node analysis on complex dtype #113785
Conversation
[ghstack-poisoned]
titaiwangms
requested review from
BowenBao,
abock,
thiagocrepaldi and
wschin
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113785
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 018ec28 with merge base 7183926 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
the
release notes: onnx
ghstack-source-id: 948556b7aad93e6c9102dfccc6608999ade02de6 Pull Request resolved: #113785
titaiwangms
added
module: onnx
| target = node.target | ||
| op_to_target_mapping.setdefault(op, {}).setdefault(str(target), None) | ||
| # Extract the dtype information from the diagnostic message. | ||
| if e.diagnostic.message and e.diagnostic.message.startswith( |
There was a problem hiding this comment.
can we raise specific exception type instead of string matching to detect this error? string matching on logs is usually flaky. we log in one place and check the string matching in another. Unless we have a test for guarding this string matching, it will eventually break
There was a problem hiding this comment.
We all know that. The root cause is that the mismatch message in SARIF and screen error message. I wonder if we should create a new rule to get a new template in this error message. @BowenBao Do you have any suggestion.
There was a problem hiding this comment.
Added test for the current approach
There was a problem hiding this comment.
Maybe we can just expand the error being emitted from this analysis, showing diagnostic.message for each op. That way we don't need to do string parsing on them.
There was a problem hiding this comment.
Or maybe we can make it standard to attach dtype with optype, through looking at the graph/node instead of string parsing.
There was a problem hiding this comment.
I see. The first one seems good and we can align the message in screen printing with SARIF.
There was a problem hiding this comment.
Maybe we can just expand the error being emitted from this analysis, showing
diagnostic.messagefor each op. That way we don't need to do string parsing on them.
Not relying on string matching is a nice improvement. Could we go further and populate op_to_target_mapping (thus the error message) without using diagnostic at all?
The diagnostics is a passive component that captures extra debug information and put on a SARIF file - only that. Ideally, it should not influence (by populating op_to_target_mapping) and alter UnsupportedFxNodesAnalysisResult outcome.
Maybe I am missing something, but by reusing diagnostics for anything other than SARIF, we are creating a direct coupling between UnsupportedFxNodesAnalysis and SARIF; That is, UnsupportedFxNodesAnalysis cannot properly report the error without depending on diagnostics machinery.
IMO, if we could find a way to capture the missing information on the analysis pass independently of diagnostics, we can keep the analysis pass and diagnostics decoupled.
There was a problem hiding this comment.
Made some modifications. Essentially, now we don't leverage on get_function_overloads, instead, we use is_registered_op to capture the ATen that doesn't even have at least one ONNX invariant. Completely extract out the arg/function sig matching from UnsupoortedNodeAnalysis.
If ATen is only missing complex support, the error will be emitted during dispatcher now, which provides a more suitable error message.
…nalysis" In cases like #113444, users usually stop at UnsupportedNodeAnalysis with unsupported nodes information. Although in SARIF, they can clearly see it's due to lack of COMPLEX support, in screen error message, it's only showing original FX node name, such as `aten.mul.Tensor`. This PR catches the information from diagnostic messages and reveal it to users. [ghstack-poisoned]
ghstack-source-id: 1fdea28a307b830429c0e6ed223cbd43ff837261 Pull Request resolved: #113785
…nalysis" In cases like #113444, users usually stop at UnsupportedNodeAnalysis with unsupported nodes information. Although in SARIF, they can clearly see it's due to lack of COMPLEX support, in screen error message, it's only showing original FX node name, such as `aten.mul.Tensor`. This PR catches the information from diagnostic messages and reveal it to users. [ghstack-poisoned]
ghstack-source-id: 184cdbab860f428798a5ec68495b4cddd0910a33 Pull Request resolved: #113785
titaiwangms
changed the title
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Stack from ghstack (oldest at bottom):
In cases like #113444, users usually stop at UnsupportedNodeAnalysis with unsupported nodes information. Although in SARIF, they can clearly see it's due to lack of COMPLEX support, in screen error message, it's only showing original FX node name, such as
aten.mul.Tensor.This PR catches the information from diagnostic messages and reveal it to users.The root cause is that UnsupportedNodeAnalysis is leveraging on
onnxfunction_dispatcher.get_function_overloads()to decide if an ATen is supported or not. However, inonnxfunction_dispatcher.get_function_overloads(), lacking of complex function support is considered unsupported. This PR defines Unsupported FX nodes as not in registry.