NXP backend - added support for aten.conv2d using the new Neutron flow#19717
NXP backend - added support for aten.conv2d using the new Neutron flow#19717novak-vaclav wants to merge 2 commits into
aten.conv2d using the new Neutron flow#19717Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19717
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit bc6ecdf with merge base 2b7a5a2 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
|
@pytorchbot label "module: nxp" |
|
@pytorchbot label "release notes: nxp" |
pytorch-bot
Bot
added
module: nxp
|
Please also review @MartinPavella @roman-janik-nxp @StrycekSimon. Thank you 😄 |
There was a problem hiding this comment.
Pull request overview
Adds aten.conv2d delegation coverage for the NXP Neutron “new flow” and expands the NXP backend test suite to validate delegation behavior and (where possible) numerical correctness.
Changes:
- Add a new-flow support-check path in the convolution converter and tighten failure behavior when group conv decomposition is missing.
- Add extensive new pytest coverage for conv2d delegation/non-delegation scenarios under the new Neutron flow (including depthwise and edge/bounds cases, plus targeted
xfails for known issues). - Improve NPU-vs-CPU mismatch reporting in the output comparator by including the max diff in the assertion message.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backends/nxp/tests/model_output_comparator.py | Improves assertion failure message for output mismatches. |
| backends/nxp/tests/ir/converter/node_converter/test_conv_converter.py | Adds a large new test suite for conv2d under the new Neutron flow, including delegation checks and known-issue xfails. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/convolution_converter.py | Introduces new-flow target support checks and adjusts convolution argument handling / group-conv behavior. |
| backends/nxp/aten_passes/split_group_convolution.py | Updates group-conv splitting pass to handle optional bias and propagate FakeTensor meta safely. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
novak-vaclav
force-pushed
the
feature/EIEX-861-add-conv2d-support-new-neutron-flow
branch
from
9cedbc4 to
bc6ecdf
Compare
|
Fixed the issues found by Copilot. |
MartinPavella
left a comment
MartinPavella
left a comment
There was a problem hiding this comment.
Will review the rest of test_conv_converter tomorrow.
| assert all_close | ||
| assert ( | ||
| all_close | ||
| ), f"NPU output doesn't match reference. Maximum absolute difference: {max_diff}" |
There was a problem hiding this comment.
Why duplicate the message?
If you think the error message should contain the max_diff, please remove the print statement and keep only the assert message.
There was a problem hiding this comment.
I added it there so we are able to catch what assert failed. Catching a specific printed string (ie. stdout) is more difficult.
I used this string in test_conv_converter to get the information that tests failed because of incorrect results.
There was a problem hiding this comment.
Sure, that sounds good. My point was that if you need the assert message, please remove the print statement.
|
|
||
| except AssertionError as e: | ||
| if "NPU output doesn't match reference" in str(e): | ||
| pytest.xfail( |
There was a problem hiding this comment.
This seems dangerous.
If I understand correctly, all tests (which don't fail with NeutronConverter) will pass. Either their error is low (so it passes), or the error is high and the test is marked with xfail (so it passes).
Is that really the behavior?
There was a problem hiding this comment.
I thought that when Neutron team resolves the JIRA issue with CONV_2D producing incorrect results (the one in the pytest.xfail), we will remove this.
The issue specifically mentions CONV_2D not providing correct output, that is what "NPU output doesn't match reference" string indicates.
So this catches and marks by xfail every configuration that does not yield correct results, which is exactly what the JIRA issue addresses.
There was a problem hiding this comment.
I understand that, but there are some issues with the implementation:
- It is not clear how many tests are marked
xfail. It could be that they are all skipped and we don't actually have tests verifying the correct behavior. Even if this extreme case does not happen, it is nice to have an idea of the severity of the Neutron bug (how often does it happen...). - Due to the dynamic nature of the implementation, we will not notice any changes is the Neutron behavior. For example if the Neutron bug is fixed, the tests will still be passing, and we will not be alerted (that's why I strongly prefer to statically mark the failing instances with
xfailandstrict=True). Also, if there is a regression in Neutron and more cases start failing, we will have no idea. - Let's say the Neutron bug is fixed. We would reflect the change (as you suggested) by removing your code for dynamic
xfailmarking. What if we then discover that some cases are still failing due to some other bug? Our only option is to rollback thexfailmarking code (or a bigger refactor). So what I'm trying to say is that with this system, there is no way to gradually increase our successful test coverage.
Please remove the code for dynamic xfail marking and instead identify the failing cases and statically mark them with xfail and strict=True.
|
@novak-vaclav please take a look at this issue: https://jira.sw.nxp.com/browse/EIEX-809 |
Summary
Added support for
aten.conv2dusing new Neutron flow.Test plan
tests can be manually run using
pytest -c /dev/null backends/nxp/tests/cc @robert-kalmar @JakeStevens @digantdesai @rascani @MartinPavella