Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[microTVM][Zephyr] Add 'config_main_stack_size' option to API server #9026

Merged
merged 11 commits into from
Sep 23, 2021
Merged

[microTVM][Zephyr] Add 'config_main_stack_size' option to API server #9026

merged 11 commits into from
Sep 23, 2021

Conversation

mehrdadh
Copy link
Member

  • When using API server in for external project, sometime we need to set main stack size for zephyr board if model is large. This PR adds this option to Zephyr project API.
  • In addition, this PR moves zephyr board properties to a json file to create a single source for board information for testing purposes.
  • Finally, this PR adds a validation check for projectOption passed to api server.

cc @areusch @gromero

waiting for #9018 before merging this.

@mehrdadh mehrdadh force-pushed the add_stack_api_server branch from 8a64854 to 9545be9 Compare September 16, 2021 04:30
@mehrdadh mehrdadh mentioned this pull request Sep 16, 2021
Merged
areusch
areusch requested changes Sep 17, 2021
View reviewed changes
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.

thanks @mehrdadh

apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
tests/micro/zephyr/test_zephyr.py Outdated Show resolved Hide resolved
tests/micro/zephyr/conftest.py Outdated Show resolved Hide resolved
gromero
gromero requested changes Sep 17, 2021
View reviewed changes
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.

Hi @mehrdadh Thanks for the change. Looks good overall.

Please see my comments inline. And a nit in the title: I think it should be:
[microTVM][Zephyr] Add 'main_stack_size' option to API server

Cheers

apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
tests/micro/zephyr/test_zephyr.py Outdated Show resolved Hide resolved
python/tvm/micro/project.py Show resolved Hide resolved
python/tvm/micro/project.py Outdated Show resolved Hide resolved
@mehrdadh mehrdadh changed the title [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server [microTVM][Zephyr] Add 'main_stack_size' option to API server Sep 17, 2021
@mehrdadh
Copy link
Member Author

@gromero thanks for the review. I addressed the comments. PTAL.

@mehrdadh mehrdadh force-pushed the add_stack_api_server branch from 37c559f to 92a3b4a Compare September 17, 2021 22:31
@mehrdadh mehrdadh force-pushed the add_stack_api_server branch from 92a3b4a to d8a1b0e Compare September 18, 2021 00:55
@mehrdadh mehrdadh closed this Sep 18, 2021
@mehrdadh mehrdadh reopened this Sep 18, 2021
gromero
gromero requested changes Sep 20, 2021
View reviewed changes
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 Thanks for addressing the comments. There is only one remaining issue in the code that was not caught by the CI (PTAL at the inline comments) and a nit in the PR title. Otherwise LGTM.

tests/micro/zephyr/test_zephyr.py Show resolved Hide resolved
tests/micro/zephyr/test_zephyr.py Show resolved Hide resolved
apps/microtvm/zephyr/template_project/microtvm_api_server.py Show resolved Hide resolved
@mehrdadh mehrdadh changed the title [microTVM][Zephyr] Add 'main_stack_size' option to API server [microTVM][Zephyr] Add 'config_main_stack_size' option to API server Sep 20, 2021
@mehrdadh mehrdadh mentioned this pull request Sep 20, 2021
Merged
@mehrdadh
Copy link
Member Author

waiting for CI fix: #9050

@gromero
Copy link
Contributor

gromero commented Sep 20, 2021

@mehrdadh LGTM, thanks! Once CI is happy with it I'm happy too.

PS: that branch is still failing on disco board on test_autotune_conv2d but the error is not related to the changes in this PR. It seems the error was introduced with AutoTVM patchset. Anyway, I've filed #9049 to keep track of it.

areusch
areusch approved these changes Sep 21, 2021
View reviewed changes
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.

@mehrdadh please fix the CI issue

@mehrdadh mehrdadh mentioned this pull request Sep 21, 2021
Merged
@areusch
Copy link
Contributor

areusch commented Sep 21, 2021

blocked on #9013

@areusch areusch merged commit 73c2845 into apache:main Sep 23, 2021
@mehrdadh mehrdadh deleted the add_stack_api_server branch September 23, 2021 15:55
gromero pushed a commit to gromero/tvm that referenced this pull request Sep 24, 2021
@mehrdadh @gromero
[microTVM][Zephyr] Add 'config_main_stack_size' option to API server (a…
9f4cc25 
…pache#9026)
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
@mehrdadh @ylc
[microTVM][Zephyr] Add 'config_main_stack_size' option to API server (a…
23562e9 
…pache#9026)
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@mehrdadh @ylc
[microTVM][Zephyr] Add 'config_main_stack_size' option to API server (a…
8997c66 
…pache#9026)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gromero gromero gromero approved these changes

@areusch areusch areusch approved these changes

@comaniac comaniac Awaiting requested review from comaniac

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@liangfu liangfu Awaiting requested review from liangfu

@manupak manupak Awaiting requested review from manupak

@merrymercy merrymercy Awaiting requested review from merrymercy

@tmoreau89 tmoreau89 Awaiting requested review from tmoreau89

@tqchen tqchen Awaiting requested review from tqchen

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mehrdadh @gromero @areusch