-
Notifications
You must be signed in to change notification settings - Fork 684
NXP backend: Make the flow robust against input/output swapping. #12890
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
NXP backend: Make the flow robust against input/output swapping. #12890
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12890
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 2 New FailuresAs of commit 3c5e971 with merge base 4197fc1 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
299fc73
to
03abf61
Compare
@pytorchbot label "release notes: nxp" |
NXP build failing |
Thanks, I noticed it earlier but I was already out of office. I will fix it. |
03abf61
to
d0eb3c4
Compare
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.
Looks good to me. The CI failures seems irrelevant to the changes in this PR.
Rebased to main to have more up to date test results.
assert ( | ||
len(outputs) < 256 | ||
len(neutron_artifacts.input_indices) < 256 | ||
), "Models with more than 255 inputs are not supported." |
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.
NIT: This is actually a valid case, not a "programming error". We just use 8bit field to encode input indices. Consider using ValueError instead of assert.
len(outputs) < 256 | ||
len(neutron_artifacts.input_indices) < 256 | ||
), "Models with more than 255 inputs are not supported." | ||
assert ( |
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.
NIT: This is actually a valid case, not a "programming error". We just use 8bit field to encode input indices. Consider using ValueError instead of assert.
d0eb3c4
to
3c5e971
Compare
#define OUTPUT_TENSOR_FORMAT_LEN_POS 1 | ||
#define INPUT_TENSOR_FORMAT_ARRAY_ADDR(base) (base + 2 * ITEM_SIZE) | ||
#define INPUT_ARGS_LEN_POS 2 | ||
#define INPUT_TENSOR_FORMAT_ARRAY_ADDR(base) (base + 3 * ITEM_SIZE) |
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.
This is ok for now but in the future consider FC/BC issues when updating serialization formats.
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.
We hope that the format is now general enough to cover the future scenarios.
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.
Says like a true developer, until a second before breaking BC :-p
…orch#12890) ### Summary The NXP backend is now robust against the swapping the order of inputs/outputs in the model converter. Release notes: NXP ### Test plan test_neutron_backend.py tests this feature
Summary
The NXP backend is now robust against the swapping the order of inputs/outputs in the model converter.
Release notes: NXP
Test plan
test_neutron_backend.py tests this feature
cc @digantdesai @JakeStevens @robert-kalmar