Arm backend: Add tosa_spec info to .tosa files #9392
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9392
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New FailuresAs of commit fa1e3bb with merge base 644b7dd ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
oscarandersson8218
added
partner: arm
Hi could you help us point out what error you got from this, to help us knowing how to avoid this better in the future or even better could the internal tests be ported to be runned in github also so we can get proper testing while merging? |
|
@larryliu0820 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
The original PR was failing all |
|
This patch renames the tosa files could you have tests that assume some filenames? |
|
This seems fine in terms of internal test |
|
Hi @larryliu0820 does this mean we should go ahead and merge this again now? Or will you merge it to do the extended testing? |
backends/arm/tosa_backend.py
Outdated
| @@ -122,10 +123,12 @@ def preprocess( # noqa: C901 | |||
|
|
|||
| if artifact_path: | |||
| tag = _get_first_delegation_tag(graph_module) | |||
| et_version = version("executorch") | |||
There was a problem hiding this comment.
This is causing issues for internal setup, I need to take a look.
There was a problem hiding this comment.
Thanks, This was merged and reverted during your days off and we were asked to reupload the PR. We don't know what caused the problem so the PR was unchanged. One suspicion we have is if you assume and tosa file names in some way in your flow that would break as they get renamed by this patch.
There was a problem hiding this comment.
I need to take a better look at internal failures but this could be because of how packages are set up internally. Can we drop et_version for now?
There was a problem hiding this comment.
Yes, that's fine for us.
oscarandersson8218
requested review from
GregoryComer,
swolchok,
JacobSzwejbka,
lucylq,
manuelcandales,
kimishpatel,
shoumikhin,
jackzhxng,
iseeyuan,
tarun292,
cccclai,
metascroy,
cymbalrush,
mergennachin and
Gasoonjia
as code owners
oscarandersson8218
removed request for
swolchok,
shoumikhin,
cymbalrush,
mergennachin,
SS-JIA,
iseeyuan,
GregoryComer,
cccclai,
kimishpatel,
lucylq,
JacobSzwejbka,
Gasoonjia,
jackzhxng,
mcr229,
manuelcandales,
tarun292,
kirklandsign and
metascroy
|
I messed up the rebase a bit, hence all the added reviewers in the log. Sorry about that. PR should be in a good state again though. |
zingo
changed the title
|
LGTM, stamped. Let me pull internally just to be safe (re-revert would be bad :p) - should take a couple of hours. I can merge it if you don't mind. |
|
@digantdesai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Thanks, please go ahead 🙏 |
|
@digantdesai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Ci failures related? Internal CI is happy. |
@digantdesai Don't think they are related. |
Arm backend output files will now be named as `output_tag<N>_<tosa_spec>.tosa` instead of `output_tag.tosa`. Signed-off-by: Oscar Andersson <[email protected]>
Arm backend output files will now be named as
output_tag<N>_<tosa_spec>.tosainstead ofoutput_tag.tosa.cc @digantdesai @freddan80 @per @zingo