You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/09/16 01:37:27 UTC
[GitHub] [tvm] mehrdadh opened a new pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
mehrdadh opened a new pull request #9026:
URL: https://github.com/apache/tvm/pull/9026
- 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 https://github.com/apache/tvm/pull/9018 before merging this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711369451
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
+
+with open(
+ BOARDS_FILE_NAME,
+) as board_f:
+ BOARD_PROPERTIES = json.load(board_f)
Review comment:
since this is used once I think we could just use FileNotFoundError exception? something like this:
```
try:
with open(BOARDS) as boards:
BOARD_PROPERTIES = json.load(boards)
except FileNotFoundError:
raise FileNotFoundError(f"Board file {{{BOARDS}}} does not exist.")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #9026: [microTVM][Zephyr] Add 'main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r712203347
##########
File path: tests/micro/zephyr/test_zephyr.py
##########
@@ -416,23 +407,21 @@ def test_autotune_conv2d(temp_dir, board, west_cmd, tvm_debug):
tasks = tvm.autotvm.task.extract_from_program(mod["main"], {}, target)
assert len(tasks) > 0
- repo_root = pathlib.Path(
- subprocess.check_output(["git", "rev-parse", "--show-toplevel"], encoding="utf-8").strip()
- )
+ if test_utils.qemu_boards(board):
Review comment:
Same thing here, you need to set `config_main_stack_size` in case it's not a QEMU board, otherwise the test will fail.
##########
File path: tests/micro/zephyr/test_zephyr.py
##########
@@ -58,21 +49,23 @@ def _make_sess_from_op(
def _make_session(temp_dir, zephyr_board, west_cmd, mod, build_config):
- stack_size = None
- if conftest.qemu_boards(zephyr_board):
- stack_size = 1536
+ if test_utils.qemu_boards(zephyr_board):
+ config_main_stack_size = 1536
Review comment:
In this case you do need to set `config_main_stack_size = None` because if the board is not a qemu one the code will crash because `config_main_stack_size` will never be set.
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -260,12 +261,7 @@ def _get_nrf_device_args(options):
help="Name of the Zephyr board to build for.",
),
server.ProjectOption(
- "zephyr_model",
- choices=[board["model"] for _, board in BOARD_PROPERTIES.items()],
- help="Name of the model for each Zephyr board.",
- ),
- server.ProjectOption(
- "main_stack_size",
+ "config_main_stack_size",
Review comment:
You need to adapt the PR's title to say now `config_main_stack_size` instead of `main_stack_size`. It's not so important for the PR itself but after the squash the commit title that will land into tree won't match the code, so essentially it will be wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711358099
##########
File path: python/tvm/micro/project.py
##########
@@ -101,7 +101,26 @@ def __init__(self, api_client):
if not self._info["is_template"]:
raise NotATemplateProjectError()
+ def _check_project_options(self, options: dict):
Review comment:
I'm wondering if it would be possible and better to rewrite that as something like:
```
available_options = [option["name"] for option in template.info()["project_options"]]
if not set(options.keys()).issubset(available_options):
raise ValueError(...)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711376548
##########
File path: python/tvm/micro/project.py
##########
@@ -101,7 +101,26 @@ def __init__(self, api_client):
if not self._info["is_template"]:
raise NotATemplateProjectError()
+ def _check_project_options(self, options: dict):
Review comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microTVM][Zephyr] Add 'main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r712264913
##########
File path: tests/micro/zephyr/test_zephyr.py
##########
@@ -58,21 +49,23 @@ def _make_sess_from_op(
def _make_session(temp_dir, zephyr_board, west_cmd, mod, build_config):
- stack_size = None
- if conftest.qemu_boards(zephyr_board):
- stack_size = 1536
+ if test_utils.qemu_boards(zephyr_board):
+ config_main_stack_size = 1536
Review comment:
thanks for catching this. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711376357
##########
File path: tests/micro/zephyr/test_zephyr.py
##########
@@ -67,6 +71,7 @@ def _make_session(temp_dir, zephyr_board, west_cmd, mod, build_config):
"west_cmd": west_cmd,
"verbose": bool(build_config.get("debug")),
"zephyr_board": zephyr_board,
+ "main_stack_size": stack_size,
Review comment:
I agree. changed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh closed pull request #9026: [microTVM][Zephyr] Add 'main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh closed pull request #9026:
URL: https://github.com/apache/tvm/pull/9026
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on pull request #9026: [microTVM][Zephyr] Add 'config_main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#issuecomment-923049082
waiting for CI fix: https://github.com/apache/tvm/pull/9050
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711374226
##########
File path: tests/micro/zephyr/conftest.py
##########
@@ -53,6 +56,24 @@ def zephyr_boards() -> dict:
ZEPHYR_BOARDS = zephyr_boards()
+def qemu_boards(board: str):
Review comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microTVM][Zephyr] Add 'config_main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r712266997
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -260,12 +261,7 @@ def _get_nrf_device_args(options):
help="Name of the Zephyr board to build for.",
),
server.ProjectOption(
- "zephyr_model",
- choices=[board["model"] for _, board in BOARD_PROPERTIES.items()],
- help="Name of the model for each Zephyr board.",
- ),
- server.ProjectOption(
- "main_stack_size",
+ "config_main_stack_size",
Review comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711374230
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
+
+with open(
+ BOARDS_FILE_NAME,
+) as board_f:
+ BOARD_PROPERTIES = json.load(board_f)
Review comment:
@mehrdadh yeah I think that's fine too. Btw, do we need triple `{` `}` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #9026: [microTVM][Zephyr] Add 'config_main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r712297658
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
+
+with open(
+ BOARDS_FILE_NAME,
+) as board_f:
+ BOARD_PROPERTIES = json.load(board_f)
Review comment:
@mehrdadh ah, I see, you want to have the file name between `{` and `}` too. OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #9026: [microTVM][Zephyr] Add 'config_main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #9026:
URL: https://github.com/apache/tvm/pull/9026
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #9026: [microTVM][Zephyr] Add 'config_main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
areusch commented on pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#issuecomment-924471079
blocked on https://github.com/apache/tvm/issues/9013
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711367607
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
Review comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711327235
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
Review comment:
can you compute path relative to API_SERVER_DIR?
##########
File path: tests/micro/zephyr/conftest.py
##########
@@ -53,6 +56,24 @@ def zephyr_boards() -> dict:
ZEPHYR_BOARDS = zephyr_boards()
+def qemu_boards(board: str):
Review comment:
can we place test utils in a test_utils.py instead of conftest?
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -294,6 +264,10 @@ def _get_nrf_device_args(options):
choices=[board["model"] for _, board in BOARD_PROPERTIES.items()],
help="Name of the model for each Zephyr board.",
),
+ server.ProjectOption(
+ "main_stack_size",
Review comment:
can we say main_stack_size_bytes or config_main_stack_size?
##########
File path: tests/micro/zephyr/test_zephyr.py
##########
@@ -58,6 +58,10 @@ def _make_sess_from_op(
def _make_session(temp_dir, zephyr_board, west_cmd, mod, build_config):
+ stack_size = None
Review comment:
can you use same variable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711350143
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
+
+with open(
+ BOARDS_FILE_NAME,
+) as board_f:
+ BOARD_PROPERTIES = json.load(board_f)
Review comment:
A couple of suggestions here.
I think it's not necessary to enforce `BOARD_PROPERTIES = None` before `with` since it won't create a new scope -- please experiment with that possibility.
I prefer the long form, e.g.
```
with open(BOARDS) as boards:
BOARD_PROPERTIES = json.load(boards)
```
I do expect the linter won't complain about it.
I think `with` should be inside a `try` in case the file is missing, corrupt etc for whatever reason, and `except` so could raise a BoardProperties(exception) class like there is already one for `BoardError` and `BoardAutodetectFailed`. I recall @areusch concerned once about overloading too much exceptions in the server side, so I'm wondering what he thinks about that suggestion actually :)
##########
File path: tests/micro/zephyr/test_zephyr.py
##########
@@ -67,6 +71,7 @@ def _make_session(temp_dir, zephyr_board, west_cmd, mod, build_config):
"west_cmd": west_cmd,
"verbose": bool(build_config.get("debug")),
"zephyr_board": zephyr_board,
+ "main_stack_size": stack_size,
Review comment:
I think this is the first time an option is passed that is of type 'None'. So far usually what we do if an option is deemed `None` is to simply don't pass the option and then use the following in the server side:
```
if `options.get("main_stack_size"):
... options["main_stack_size"]
```
So I personally prefer to keep this way instead of passing an option with value "None". If that form is adopted in this case it would be a matter of checking for stack_size after it's set accordingly and then check if it's None, appending (or not) the "main_stack_size" option to the options dict. @areusch wdyt?
##########
File path: python/tvm/micro/project.py
##########
@@ -101,7 +101,26 @@ def __init__(self, api_client):
if not self._info["is_template"]:
raise NotATemplateProjectError()
+ def _check_project_options(self, options: dict):
Review comment:
I'm wondering if it would be possible and better to rewrite that as:
```
available_options = [option["name"] for option in template.info()["project_options"]]
if not set(options.keys()).issubset(available_options):
raise ValueError(...)
```
##########
File path: python/tvm/micro/project.py
##########
@@ -101,7 +101,26 @@ def __init__(self, api_client):
if not self._info["is_template"]:
raise NotATemplateProjectError()
+ def _check_project_options(self, options: dict):
+ """Check if options are valid ProjectOptions"""
+ valid_project_options = [item["name"] for item in self.info()["project_options"]]
+ valid = True
+
+ if options is not None:
+ if valid_project_options is None:
+ valie = False
Review comment:
s/valie/valid/ if that form will be kept?
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
Review comment:
"boards.json" path needs to be computed relative to API_SERVER_DIR as Andrew said, i.e.:
`BOARDS_FILE_NAME = API_SERVER_DIR / "boards.json"`
I also suggest changing `BOARDS_FILE_NAME` to simply `BOARDS`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on pull request #9026: [microTVM][Zephyr] Add 'main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#issuecomment-922114683
@gromero thanks for the review. I addressed the comments. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on pull request #9026: [microTVM][Zephyr] Add 'config_main_stack_size' option to API server
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#issuecomment-923069885
@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 https://github.com/apache/tvm/issues/9049 to keep track of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #9026: [microtvm][Zephyr] Add MAIN_STACK_SIZE option to API server
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9026:
URL: https://github.com/apache/tvm/pull/9026#discussion_r711376734
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -59,44 +60,13 @@
# Data structure to hold the information microtvm_api_server.py needs
# to communicate with each of these boards.
-BOARD_PROPERTIES = {
- "qemu_x86": {
- "board": "qemu_x86",
- "model": "host",
- },
- "qemu_riscv32": {
- "board": "qemu_riscv32",
- "model": "host",
- },
- "qemu_riscv64": {
- "board": "qemu_riscv64",
- "model": "host",
- },
- "mps2_an521": {
- "board": "mps2_an521",
- "model": "mps2_an521",
- },
- "nrf5340dk_nrf5340_cpuapp": {
- "board": "nrf5340dk_nrf5340_cpuapp",
- "model": "nrf5340dk",
- },
- "stm32f746g_disco": {
- "board": "stm32f746g_disco",
- "model": "stm32f746xx",
- },
- "nucleo_f746zg": {
- "board": "nucleo_f746zg",
- "model": "stm32f746xx",
- },
- "nucleo_l4r5zi": {
- "board": "nucleo_l4r5zi",
- "model": "stm32l4r5zi",
- },
- "qemu_cortex_r5": {
- "board": "qemu_cortex_r5",
- "model": "zynq_mp_r5",
- },
-}
+BOARD_PROPERTIES = None
+BOARDS_FILE_NAME = "boards.json"
+
+with open(
+ BOARDS_FILE_NAME,
+) as board_f:
+ BOARD_PROPERTIES = json.load(board_f)
Review comment:
yeah because of f"" statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org