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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
