-
Notifications
You must be signed in to change notification settings - Fork 696
NXP backend: Resolve limitations of uncertain tensor formats. #14576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
NXP backend: Resolve limitations of uncertain tensor formats. #14576
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14576
Note: Links to docs will display an error until the docs builds have been completed. ❌ 7 New Failures, 2 Unrelated FailuresAs of commit 768c489 with merge base fdfeaa4 ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "module: nxp" "release notes: nxp" |
| deps = [ | ||
| ":neutron_sdk", | ||
| ":aten_passes", | ||
| ":_passes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add neutron_backend build to this https://github.com/pytorch/executorch/blob/main/.ci/scripts/unittest-buck2.sh. Thats the one that was failing last time. This way we get signal on the pr itself
|
@kimishpatel has imported this pull request. If you are a Meta employee, you can view this in D83311730. |
|
i have imported it internally. There is an nxp test failing as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review automatically exported from Phabricator review in Meta.
bb48aad to
e1893f5
Compare
|
@kimishpatel I have fixed the failing nxp test, and added the nxp backend build to the 1: From load at backends/nxp/TARGETS:4 These lines were in the file already before my changes. Do you know what is causing the issue/how to fix it? |
I guess it is triggering your build in oss and we have never really had this test in oss so its trying to pull in internal files. I would say roll back that change for now just to be able to land. |
2672277 to
1ecfe36
Compare
@kimishpatel thanks for the response. I have removed the build as you said. Can you please now verify that your internal build passes? |
|
doing |
@kimishpatel any updates? Is the build passing? |
@kimishpatel pinging in case you missed this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the numMacs related comment. Did the remove_geititem_pass.py just pass the linter with such long lines?
backends/nxp/backend/ir/converter/node_converters/ops_converters/cat_converter.py
Outdated
Show resolved
Hide resolved
backends/nxp/tests/ir/converter/node_converter/test_softmax_converter.py
Show resolved
Hide resolved
Yes, the linter passed. I have shortened the lines now anyway. |
1ecfe36 to
27444cf
Compare
27444cf to
fa43ac2
Compare
974c05d to
a3ba1bb
Compare
@kimishpatel any updates? This issue is blocking multiple other PRs. I really don't want to break your internal builds again, so it would be great to confirm they are passing before merging this. |
|
Sorry for the late response on this. Lets rebase and then merge it |
… node format. The pass `RemoveGetItemPass` replaces a `max_pool2d_with_indices` node with a `max_pool2d` node, that doesn't require a GetItem afterward. The new operator must, however, preserve the original node format. Therefore, a copy of the pass was created in `backends/nxp/_passes`, where it was modified. The new directory was created, because the pass doesn't follow the `NeutronEdgePass` interface.
Before, the format inference was done during conversion to NeutronIR (after partitioning), so the partitioner didn't yet know the formats. Now, the partitioner has the format data, which can be used to accurately select nodes for delegation.
a3ba1bb to
768c489
Compare
|
@kimishpatel The failing checks appear unrelated, but I still cannot merge the PR. The "cuda" checks fail on huggingface:
The samsung check fails on
The
And |
Summary
This PR resolves format related issues by inferring the format (NCHW/NHWC) for all nodes before partitioning. These formats are then used by the NeutronPartitioner to accurately determine which nodes are supported on Neutron.
Test plan
Unit tests provided, and correct function is tested by nearly every test in the nxp backend.
cc @kimishpatel