[microTVM] Add platform version check to template project by mehrdadh · Pull Request #9274 · apache/tvm

mehrdadh · 2021-10-13T17:55:42Z

This PR:

adds platform version check to both Arduino and Zephyr
adds macro for version in different files to make it easier to find when update is needed.

mehrdadh · 2021-10-13T17:55:58Z

areusch · 2021-10-14T16:52:27Z

    def generate_project(self, model_library_format_path, standalone_crt_dir, project_dir, options):
+        # Check Arduino version
+        version = self._get_platform_version(options["arduino_cli_cmd"])
+        if version != ARDUINO_CLI_VERSION:


i'm not sure we should do an exact match check. cc @guberti do you have a better suggestion as to how loose we should be? i could even just be okay with a warning

I think two levels of version, Major + Minor, is enough. We don't need to check the patch level version since they usually don't change interfaces. I discussed this with @gromero and that seems to be the case for Zephyr. For Arduino I got the same impression based on their release messages: https://github.com/arduino/arduino-cli/releases
@guberti probably has more info here.

Yea the Arduino CLI seems to be relatively stable - this looks fine to me.

gromero

Hi @mehrdadh Only regarding the Zephyr platform changes and considering the nit pointed by Andrew is fixed, LGTM! I think that's a nice enhancement and that it's right for now to match exactly against Zephyr MAJOR and MINOR versions and so treat the version as a string. In the future if 2.5 becomes the minimum version supported on microTVM (not an exact match) we can easily adapt the code to match against, for instance, a float, like it's done in https://github.com/apache/tvm/blob/main/python/tvm/topi/arm_cpu/arm_utils.py#L24

Thanks!

mehrdadh · 2021-10-15T19:17:22Z

@gromero thanks for the review.
I changed the versions to float so we could do comparison in future. Since we only tested Zephyr/Arduino with the current versions (2.5/0.18), we just compare with the exact version for now. In future, we can change this to be like >= MIN_VERSION_SUPPORTED.

gromero

I missed these two typos on my last review. Could you please fix them?

Otherwise LGTM!

gromero

I can't comment on Arduino versioning, but since we now just warn when versions don't match rather than abort by default I'm ok with it. Plus the versioning mechanism can now be used for non exact matches, so LGTM. Thanks.

mehrdadh · 2021-10-20T00:13:13Z

@areusch PTAL when you have time.
thanks!

areusch

@mehrdadh sorry for delay--mostly small things here.

guberti

Looks good to me! Sorry about the delay caused by my midterms and being sick.

guberti · 2021-10-22T21:13:16Z

    def generate_project(self, model_library_format_path, standalone_crt_dir, project_dir, options):
+        # Check Arduino version
+        version = self._get_platform_version(options["arduino_cli_cmd"])
+        if version != ARDUINO_CLI_VERSION:


Yea the Arduino CLI seems to be relatively stable - this looks fine to me.

guberti · 2021-10-22T21:15:34Z

+    def _get_platform_version(self, arduino_cli_path: str) -> float:
+        version_output = subprocess.check_output([arduino_cli_path, "version"], encoding="utf-8")
+        version_output = (
+            version_output.replace("\n", "").replace("\r", "").replace(":", "").lower().split(" ")


What exactly is this line stripping out? Can we use a regex or .strip()? I'd love to see a comment here with the standard output of arduino-cli version so I can better understand what's happening here.

thanks for the review. I will add the format as a comment and change it to use regex in the following PR which is waiting for this:
#9309

junrushao

I'm not an expert in microTVM, but the other parts LGTM

junrushao · 2021-10-25T18:04:18Z

Given @areusch is out this week, and it's been approved by multiple contributors in our community, I believe we can go ahead to merge this PR to unblock a subsequent one. What do you think @mehrdadh

mehrdadh · 2021-10-25T18:18:36Z

@junrushao1994 sounds good to me. thanks!

junrushao · 2021-10-25T18:19:35Z

Thanks @mehrdadh @guberti @gromero! The PR is merged

* add platform version to project template * fix arduino cli version on qemu * specify arduino/zephyr version everywhere * cleanup * address comments * fix warning message * fix typo * trigger * trigger * address comments * trigger * trigger

mehrdadh added 4 commits October 13, 2021 10:40

add platform version to project template

e8eacd7

fix arduino cli version on qemu

a0f771b

specify arduino/zephyr version everywhere

bd980a8

cleanup

007165a

mehrdadh requested review from areusch, comaniac, jroesch, junrushao, leandron, tqchen, yzhliu and zhiics as code owners October 13, 2021 17:55

areusch reviewed Oct 14, 2021

View reviewed changes

gromero approved these changes Oct 15, 2021

View reviewed changes

address comments

1b0077c

mehrdadh force-pushed the add_platform_version branch from 8c73aed to 1b0077c Compare October 15, 2021 19:16

gromero requested changes Oct 15, 2021

View reviewed changes

Comment thread apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated

fix warning message

489c699

gromero requested changes Oct 18, 2021

View reviewed changes

Comment thread apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated

Comment thread apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated

fix typo

75ab41d

mehrdadh mentioned this pull request Oct 18, 2021

[microTVM] Add microTVM Template Projects to tlcpack pip Package #9309

Merged

gromero approved these changes Oct 18, 2021

View reviewed changes

trigger

39e9b76

mehrdadh mentioned this pull request Oct 19, 2021

[CI Problem][Possible Falky Test] tests/python/frontend/tensorflow/test_forward.py::test_forward_resnetv2 #9322

Closed

trigger

f3c7be6

areusch reviewed Oct 21, 2021

View reviewed changes

address comments

bd97931

mehrdadh added 3 commits October 21, 2021 10:31

Merge branch 'main' into add_platform_version

6ea391b

trigger

13df1cf

trigger

643fc05

guberti approved these changes Oct 22, 2021

View reviewed changes

junrushao approved these changes Oct 25, 2021

View reviewed changes

junrushao merged commit 133a7dc into apache:main Oct 25, 2021

mehrdadh deleted the add_platform_version branch October 25, 2021 18:19

Uh oh!

Conversation

mehrdadh commented Oct 13, 2021

Uh oh!

mehrdadh commented Oct 13, 2021

Uh oh!

Uh oh!

areusch Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mehrdadh Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

guberti Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

gromero left a comment

Choose a reason for hiding this comment

Uh oh!

mehrdadh commented Oct 15, 2021

Uh oh!

Uh oh!

gromero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gromero left a comment

Choose a reason for hiding this comment

Uh oh!

mehrdadh commented Oct 20, 2021

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guberti left a comment

Choose a reason for hiding this comment

Uh oh!

guberti Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

guberti Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

mehrdadh Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrushao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrushao commented Oct 25, 2021

Uh oh!

mehrdadh commented Oct 25, 2021

Uh oh!

junrushao commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mehrdadh Oct 22, 2021 •

edited

Loading

junrushao left a comment •

edited

Loading