Fix Arm(R) Ethos(TM)-U55 NPU Demo app #9323
Conversation
grant-arm
requested review from
comaniac,
jroesch,
junrushao,
tqchen,
yzhliu and
zhiics
as code owners
|
I think we should enable running this in the CI -- to keep the demo up to date. |
|
@manupa-arm it'd be better to land this fix which is effecting users (#8922 (comment)) and do any infrastructure work to automate this elsewhere. |
|
Yes, however this PR passing CI does not give much assurance whether the demo is working or not. Am I missing something or can't we atleast add something such as (if we want to explore other ways automating this later) : tvm/tests/scripts/task_cpp_unittest.sh Lines 43 to 47 in 88bf112 |
|
I'll take a look at adding something to CI as part of this PR. |
|
@manupa-arm, to answer your points:
I'm sure that @grant-arm would've tested this before raising a PR.
That's what the follow up work would be, taking the time to triage it and put a solution in place. My concern is for other users who are attempting to use this demo who may not be part of the TVM community and thus aren't accounted for, if we're happy to accept that they'll come to TVM and find it broken then we can wait until the automation question is resolved. |
|
Hi @Mousius ,
Merging this will not guarantee that there wont be such users. In same way this issue occurred, we could have the same problem tomorrow without us knowing. Its better to raise an issue and point to this PR to increase awareness, if thats what we are after. Moreover, can you shed some light why the automation question is bigger piece of work that needs to be in a follow up ? |
- Change tvmc arguments to -executor=aot -interface-api=c -unpacked-api=1 Change-Id: Ic8d97ce9b4b11dc4d23ab028ac6a7832bca3ebbb
- Add a CI test for the Arm(R) Ethos(TM)-U55 NPU Demo app Change-Id: I8c535e6be573ae2cb2223dcbb10c7a9e4180799d
grant-arm
force-pushed
the
fix_ethosu_demo
branch
from
ffde4e7
to
1129228
Compare
|
I've added a CI test now that runs the Arm(R) Cortex(R)-M55 CPU and Ethos(TM)-U55 NPU demo app. |
- pip install tvm/python so that tvmc works from command line Change-Id: Ieb6ec71dd3950f7d4657793f0d02a8078667b9d9
grant-arm
force-pushed
the
fix_ethosu_demo
branch
from
0132013
to
8754008
Compare
tests/scripts/task_cpp_unittest.sh
Outdated
|
|
||
| # Test Arm(R) Cortex(R)-M55 CPU and Ethos(TM)-U55 NPU demo app | ||
| if test -f "/opt/arm/FVP_Corstone_SSE-300_Ethos-U55/models/Linux64_GCC-6.4/FVP_Corstone_SSE-300_Ethos-U55" && pip3 list | grep vela; then | ||
| sudo pip3 install -e python |
There was a problem hiding this comment.
Should this not be done earlier rather than as a side effect of invoking the unit test script?
There was a problem hiding this comment.
@Mousius Could you please clarify where you think we should do this?
There was a problem hiding this comment.
Unsure where exactly this would go but I'd guess somewhere like task_ci_setup.sh ?
There was a problem hiding this comment.
I think this is fine for the current solution.
If we set a flag in future for use in multiple scripts/locations then we can think about setting it earlier.
There was a problem hiding this comment.
I'm not suggesting we shouldn't do it but this scripts responsibility is to run unit tests, this installs TVM python and all its dependencies as a side effect which I wouldn't expect from such a script.
There was a problem hiding this comment.
Ah right, I misunderstood. I see what you're saying now.
@manupa-arm Any ideas?
There was a problem hiding this comment.
@Mousius As an alternative, we could use python3 -m tvm.driver.tvmc instead of tvmc ... in run_demo.sh but I was reluctant to do that because I thought run_demo.sh should serve as an example to users in how to use tvmc from the command line.
There was a problem hiding this comment.
Im fine with either, we can put a comment in the script to say that (one could use tvmc directly once Pip installed) or we can move it to bit higher position in terms of scripts.
Since this tested now, if we need more time to figure out where to put the installation, happy to take python3 -m tvm.driver.tvmc (with a comment explaining how it could be used alternatively)
- Changes task_cpp_unittest.sh addressing comments Change-Id: I10decdcdcec3b61e27bacfac7e94e1449a22b1b7
- Avoid installing TVM python and all its dependencies Change-Id: Iaa4dd5429871f02961cac12476f9dc46091bcb34
grant-arm
force-pushed
the
fix_ethosu_demo
branch
from
c04c500
to
7061172
Compare
- Change the way we test for Vela being installed Change-Id: Ieacb8ca9b4dd2af6821e579094ce86359c2da7f0
|
I think this is good enough for the fix (lets get this in) and thanks @grant-arm for quickly enabling it in the CI. I hope we can follow up with further changes. |
|
Thanks @grant-arm @Mousius . Please do follow up with further concerns. |
* Change tvmc arguments to -executor=aot -interface-api=c -unpacked-api=1. * Enable the demo running on the CI.
* Change tvmc arguments to -executor=aot -interface-api=c -unpacked-api=1. * Enable the demo running on the CI.
Change tvmc arguments to
-executor=aot -interface-api=c -unpacked-api=1for Arm(R) Cortex(R)-M55 CPU and Arm(R) Ethos(TM)-U55 NPU Demo App.#9218 changes the command line arguments required by tvmc which breaks the existing Arm(R) Cortex(R)-M55 CPU and Arm(R) Ethos(TM)-U55 NPU Demo App. This PR fixes the demo app.