[GitHub] [tvm] guberti commented on a change in pull request #8940: [microTVM] Change platform to device

[GitBox <gi...@apache.org>]

guberti commented on a change in pull request #8940:
URL: https://github.com/apache/tvm/pull/8940#discussion_r704050902



##########
File path: tests/micro/arduino/conftest.py
##########
@@ -48,13 +35,23 @@
 ).resolve()
 
 
+def arduino_micro_devices() -> dict:

Review comment:
       Can we cache this value? `arduino_micro_devices()` will probably be called several times by our test file.

##########
File path: apps/microtvm/reference-vm/README.md
##########
@@ -72,20 +72,20 @@ For example:
 $ ./base-box-tool.py --provider virtualbox build zephyr
 ```
 
-2. **Run** release tests for each platform:
+2. **Run** release tests for each microTVM device:

Review comment:
       ```suggestion
   2. **Run** release tests for the specified `MICROTVM_DEVICE` (e.g. `nano33ble` for Arduino)
   ```
   I don't actually think `base-box-tool.py` supports testing more than one microTVM device at once.

##########
File path: apps/microtvm/reference-vm/README.md
##########
@@ -72,20 +72,20 @@ For example:
 $ ./base-box-tool.py --provider virtualbox build zephyr
 ```
 
-2. **Run** release tests for each platform:
+2. **Run** release tests for each microTVM device:
 
    A. Connect any needed hardware to the VM host machine;
 
    B. Run tests:
    ```bash
-   $ ./base-box-tool.py [--provider=PROVIDER] test --microtvm-platform=MICROTVM_PLATFORM [--test-device-serial=SERIAL] PLATFORM
+   $ ./base-box-tool.py [--provider=PROVIDER] test --microtvm-device=MICROTVM_DEVICE [--test-device-serial=SERIAL] PLATFORM
    ```
-   where MICROTVM_PLATFORM is one of the options listed in the
+   where MICROTVM_DEVICE is one of the options listed in the
    PLATFORM/base-box/test-config.json file.
 
    For example:
    ```base
-   $ ./base-box-tool.py --provider virtualbox test --microtvm-platform=stm32f746xx_disco zephyr
+   $ ./base-box-tool.py --provider virtualbox test --microtvm-device=stm32f746xx_disco zephyr

Review comment:
       ```suggestion
      $ ./base-box-tool.py --provider virtualbox test zephyr --microtvm-device=stm32f746xx_disco
   ```
   I'd love to specify `microtvm-device` after setting `platform` to Arduino/Zephyr in the example commands, to help reinforce that the options for what `microtvm-device` can be depend on the choice of platform. I'd love to do this for all other example `test` commands in this file as well.

##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -57,6 +57,19 @@
 
 IS_TEMPLATE = not (API_SERVER_DIR / MODEL_LIBRARY_FORMAT_RELPATH).exists()
 
+# Maps a short, identifying microtvm device string to (target, zephyr_board).
+MICRO_DEVICES = {
+    "qemu_x86": ("host", "qemu_x86"),

Review comment:
       ^

##########
File path: tests/micro/arduino/conftest.py
##########
@@ -48,13 +35,23 @@
 ).resolve()
 
 
+def arduino_micro_devices() -> dict:
+    sys.path.insert(0, str(TEMPLATE_PROJECT_DIR))
+    try:
+        import microtvm_api_server
+    finally:
+        sys.path.pop(0)
+
+    return microtvm_api_server.MICRO_DEVICES

Review comment:
       I'm with Andrew - is there any way this can be done by reading the choices from `ProjectOption`?

##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -43,6 +43,19 @@
 
 IS_TEMPLATE = not (API_SERVER_DIR / MODEL_LIBRARY_FORMAT_RELPATH).exists()
 
+# Maps a short, identifying microtvm device string to (target, arduino_board).
+MICRO_DEVICES = {

Review comment:
       I agree with @gromero that `BOARDS` is a better name for these than `MICRO_DEVICES`. However, I don't like the idea of having the identifying device string map hard-coded into `microtvm_api_server.py`. 
   
   Could we merge the `MICRO_DEVICES/BOARDS` and `BOARD_PROPERTIES` into a single `boards.json` file? That way, we wouldn't have to edit `microtvm_api_server.py` whenever a new Arduino board is tested and shown to work with TVM (and will also make `microtvm_api_server.py`like 80 lines shorter). 




-- 
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