Skip to content

Conversation

@mehrdadh
Copy link
Member

@github-actions github-actions bot requested review from areusch and gromero July 25, 2022 20:41
Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehrdadh Did a first pass and have a few comments on it.


This tutorial is showcasing microTVM host-driven AoT compilation with
a TFLite model. This tutorial can be executed on a X86 CPU using C runtime (CRT)
or on Zephyr plarform on a microcontroller that supports Zephyr platform.
Copy link
Contributor

@gromero gromero Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

platform

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

# Import a TFLite model
# ---------------------
#
# To begin with, download and import a TFLite model from TinyMLPerf models.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not actually importing directly from the MLPerf Tiny bench, i.e. from:

https://github.com/mlcommons/tiny/tree/master/benchmark/training/keyword_spotting/trained_models

Should this benchmark be mentioned just as the original source of this model?

Also I think the current name for the bench is MLPerf Tiny, not TinyMLPerf, like in the text found in the READM.md here? https://github.com/mlcommons/tiny

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comment about the origin of the model and fixed MLPerf Tiny name.

`Mehrdad Hessar <https://github.com/mehrdadh>`_,
`Alan MacDonald <https://github.com/alanmacd>`_

This tutorial is showcasing microTVM host-driven AoT compilation with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be mentioned, at least briefly, the benefits of the AOT executor or the scenarios where it helps -- in contrast to the Graph executor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added few comments, please take another look.

Copy link
Member

@guberti guberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of good suggestions from @gromero - I've only got a few nits.

`Alan MacDonald <https://github.com/alanmacd>`_

This tutorial is showcasing microTVM host-driven AoT compilation with
a TFLite model. This tutorial can be executed on a X86 CPU using C runtime (CRT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit here and in a few other places:

Suggested change
a TFLite model. This tutorial can be executed on a X86 CPU using C runtime (CRT)
a TFLite model. This tutorial can be executed on a x86 CPU using C runtime (CRT)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

# with the compiled model in microTVM. To do this, we use Project API. We have defined
# CRT and Zephyr microTVM template projects which are used for X86 CPU and Zephyr platforms
# respectively.
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this and leave a blank line here

Suggested change
#

# **Note:** By default this tutorial runs on X86 CPU using CRT, if you would like to run on Zephyr platform
# you need to export `TVM_MICRO_USE_HW` environment variable.
#
use_physical_hw = bool(os.getenv("TVM_MICRO_USE_HW"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention the KWS dataset we're using, and probably ought to credit Google as the author (see https://ai.googleblog.com/2017/08/launching-speech-commands-dataset.html).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added that.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great comments all around, just a few suggestions to clarify text.

# -------------------
#
# Now we need to define the target, runtime and executor. In this tutorial, we focused on
# using AOT host driven executor. We use the host micro target which is for running a model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to make this super clear:

# Use the C runtime (crt) and enable static linking by setting system-lib to True
RUNTIME = Runtime("crt", {"system-lib": True})

# Simulate a microcontroller on the host machine. Uses the main() from src/runtime/crt/host/main.cc. To use physical hardware, replace "host" with something matching your hardware. See abc location for instructions.
TARGET = tvm.target.target.micro("host")
# Use the AOT executor rather than graph or vm executors. Don't use unpacked API or C calling style
EXECUTOR = Executor("aot")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Copy link
Member Author

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gromero @guberti @areusch thanks for the great feedbacks! PTAL

`Alan MacDonald <https://github.com/alanmacd>`_

This tutorial is showcasing microTVM host-driven AoT compilation with
a TFLite model. This tutorial can be executed on a X86 CPU using C runtime (CRT)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

`Mehrdad Hessar <https://github.com/mehrdadh>`_,
`Alan MacDonald <https://github.com/alanmacd>`_

This tutorial is showcasing microTVM host-driven AoT compilation with
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added few comments, please take another look.


This tutorial is showcasing microTVM host-driven AoT compilation with
a TFLite model. This tutorial can be executed on a X86 CPU using C runtime (CRT)
or on Zephyr plarform on a microcontroller that supports Zephyr platform.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

# Import a TFLite model
# ---------------------
#
# To begin with, download and import a TFLite model from TinyMLPerf models.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comment about the origin of the model and fixed MLPerf Tiny name.

# **Note:** By default this tutorial runs on X86 CPU using CRT, if you would like to run on Zephyr platform
# you need to export `TVM_MICRO_USE_HW` environment variable.
#
use_physical_hw = bool(os.getenv("TVM_MICRO_USE_HW"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added that.

# -------------------
#
# Now we need to define the target, runtime and executor. In this tutorial, we focused on
# using AOT host driven executor. We use the host micro target which is for running a model
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@mehrdadh
Copy link
Member Author

mehrdadh commented Jul 26, 2022

@gromero thanks for the second review. I addressed your comments

Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @mehrdadh Thanks for addressing the comments. I'll just give some time so @areusch and @guberti can have a chance to ack the changes too and then I'll merge it.

# to use the compiled model with microTVM. To do this, we use Project API. We have defined
# CRT and Zephyr microTVM template projects which are used for x86 CPU and Zephyr platforms
# CRT and Zephyr microTVM template projects which are used for x86 CPU and Zephyr boards
# respectively.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a native English speaker, but I think that ideally there must be a comma before "respectively", so it's up to you to add it or not :) I won't block on this nit, so just saying in case you need to re-spin the PR after some other review comment and if you can confirm that is indeed correct ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be, for some reason my eyes where seeing it there but I didn't actually put it there lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha I missed it previously too! :)

@gromero
Copy link
Contributor

gromero commented Jul 27, 2022

@mehrdadh I'm not sure what failed here and how to reproduce it locally. Hardly it's related to your PR, but I wonder if there is any CI issue filed for that error. Any idea? If not, maybe just ret-rigger the CI for now.

@guberti
Copy link
Member

guberti commented Jul 27, 2022

@mehrdadh @gromero I've also seen this issue on my PRs, and I believe it has been resolved. git rebase -i main will fix it.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for me, @guberti can take a look and then we can merge

@gromero
Copy link
Contributor

gromero commented Jul 28, 2022

@mehrdadh Could you please rebase?

@mehrdadh mehrdadh force-pushed the micro/add_aot_host_tutorial branch from 9f939b5 to 83124d9 Compare July 28, 2022 16:06
@mehrdadh
Copy link
Member Author

@gromero done!

Copy link
Member

@guberti guberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mehrdadh mehrdadh merged commit ebbce64 into apache:main Jul 28, 2022
@mehrdadh mehrdadh deleted the micro/add_aot_host_tutorial branch July 28, 2022 18:56
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants