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/10/13 17:55:56 UTC
[GitHub] [tvm] mehrdadh opened a new pull request #9274: [microTVM] Add platform version check to template project
mehrdadh opened a new pull request #9274:
URL: https://github.com/apache/tvm/pull/9274
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.
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#issuecomment-951183224
@junrushao1994 sounds good to me. thanks!
--
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] junrushao1994 commented on pull request #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#issuecomment-951171512
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
--
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] guberti commented on a change in pull request #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
guberti commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r734846374
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -335,7 +337,23 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ def _get_platform_version(self, arduino_cli_path: str) -> str:
+ version_output = subprocess.check_output([arduino_cli_path, "version"], encoding="utf-8")
+ version_output = (
+ version_output.replace("\n", "").replace("\r", "").replace(":", "").lower().split(" ")
+ )
+ version_index = version_output.index("version") + 1
+
+ return version_output[version_index]
+
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:
Review comment:
Yea the Arduino CLI seems to be relatively stable - this looks fine to me.
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -275,7 +286,25 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ 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(" ")
Review comment:
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.
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#issuecomment-942572704
cc @areusch @gromero @guberti
--
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] junrushao1994 commented on pull request #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#issuecomment-951184076
Thanks @mehrdadh @guberti @gromero! The PR is merged
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#issuecomment-944589794
@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`.
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r734892831
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -275,7 +286,25 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ 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(" ")
Review comment:
thanks for the review. The format is like `major_version:minor_version:patch`. I will add comment and change it to use regex in the following PR which is waiting for this:
https://github.com/apache/tvm/pull/9309
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r731081003
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -265,6 +269,11 @@ def _get_nrf_device_args(options):
"config_main_stack_size",
help="Sets CONFIG_MAIN_STACK_SIZE for Zephyr board.",
),
+ server.ProjectOption(
+ "warning_as_error",
+ choices=(True, False),
+ help="Tread warnings as errors.",
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 pull request #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#issuecomment-947199948
@areusch PTAL when you have time.
thanks!
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#issuecomment-942572704
cc @areusch @gromero @guberti
--
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] junrushao1994 merged pull request #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #9274:
URL: https://github.com/apache/tvm/pull/9274
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r733903602
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -36,13 +36,17 @@
import serial.tools.list_ports
from tvm.micro.project_api import server
+_LOG = logging.getLogger("MicroTVM API Server")
Review comment:
revert it.
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -335,7 +344,25 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ 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(" ")
+ )
+ full_version = version_output[version_output.index("version") + 1].split(".")
+ version = float(f"{full_version[0]}.{full_version[1]}")
+
+ return version
+
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:
+ message = f"Arduino CLI version found is not supported: found {version}, expected {ARDUINO_CLI_VERSION}."
+ if options.get("warning_as_error") is not None and options["warning_as_error"]:
+ raise ValueError(message)
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r733883895
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -36,13 +36,17 @@
import serial.tools.list_ports
from tvm.micro.project_api import server
+_LOG = logging.getLogger("MicroTVM API Server")
Review comment:
can you use the convention of `logging.getLogger(__name__)`? ooc how come you changed the other one? open to departing from convention if it's not working
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -36,13 +36,17 @@
import serial.tools.list_ports
from tvm.micro.project_api import server
+_LOG = logging.getLogger("MicroTVM API Server")
+
MODEL_LIBRARY_FORMAT_RELPATH = pathlib.Path("src") / "model" / "model.tar"
API_SERVER_DIR = pathlib.Path(os.path.dirname(__file__) or os.path.getcwd())
BUILD_DIR = API_SERVER_DIR / "build"
MODEL_LIBRARY_FORMAT_PATH = API_SERVER_DIR / MODEL_LIBRARY_FORMAT_RELPATH
IS_TEMPLATE = not (API_SERVER_DIR / MODEL_LIBRARY_FORMAT_RELPATH).exists()
+ARDUINO_CLI_VERSION = 0.18
Review comment:
comment why this is here--what is the semantic meaning of this version?
##########
File path: apps/microtvm/reference-vm/arduino/base-box/base_box_provision.sh
##########
@@ -31,8 +31,9 @@ cd ~
sudo apt-get install -y ca-certificates
# Install Arduino-CLI (specific version)
+ARDUINO_CLI_VERSION="0.18.3"
Review comment:
add a note to keep in sync with the version defined in apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -61,6 +61,10 @@
BOARDS = API_SERVER_DIR / "boards.json"
+
+ZEPHYR_VERSION = 2.5
Review comment:
comment similarly
##########
File path: docker/install/ubuntu_install_zephyr.sh
##########
@@ -44,9 +44,10 @@ sudo apt-get install -y cmake
pip3 install west
# Init ZephyrProject
+ZEPHYR_VERSION="v2.5-branch"
Review comment:
add similar note but explain why we checkout `-branch` here.
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -138,6 +142,11 @@ class BoardAutodetectFailed(Exception):
server.ProjectOption(
"verbose", help="True to pass --verbose flag to arduino-cli compile and upload"
),
+ server.ProjectOption(
+ "warning_as_error",
+ choices=(True, False),
+ help="Treat warnings as errors.",
Review comment:
maybe state "and raise an Exception"
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -335,7 +344,25 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ 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(" ")
+ )
+ full_version = version_output[version_output.index("version") + 1].split(".")
+ version = float(f"{full_version[0]}.{full_version[1]}")
+
+ return version
+
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:
+ message = f"Arduino CLI version found is not supported: found {version}, expected {ARDUINO_CLI_VERSION}."
+ if options.get("warning_as_error") is not None and options["warning_as_error"]:
+ raise ValueError(message)
Review comment:
can you raise a ServerError subclass?
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r734892831
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -275,7 +286,25 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ 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(" ")
Review comment:
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:
https://github.com/apache/tvm/pull/9309
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r730058853
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -335,7 +337,23 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ def _get_platform_version(self, arduino_cli_path: str) -> str:
+ version_output = subprocess.check_output([arduino_cli_path, "version"], encoding="utf-8")
+ version_output = (
+ version_output.replace("\n", "").replace("\r", "").replace(":", "").lower().split(" ")
+ )
+ version_index = version_output.index("version") + 1
+
+ return version_output[version_index]
+
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:
Review comment:
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.
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -356,13 +361,16 @@ def _get_platform_version(self) -> str:
if "VERSION_MINOR" in line:
version_minor = line.split("=")[1]
- return f"{version_major}.{version_minor}"
+ return float(f"{version_major}.{version_minor}")
def generate_project(self, model_library_format_path, standalone_crt_dir, project_dir, options):
# Check Zephyr version
version = self._get_platform_version()
if version != ZEPHYR_VERSION:
- raise ValueError(f"Zephyr version does not math: {version} != {ZEPHYR_VERSION}")
+ message = f"Zephyr version does not math: {version} != {ZEPHYR_VERSION}"
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r731073407
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -138,6 +142,11 @@ class BoardAutodetectFailed(Exception):
server.ProjectOption(
"verbose", help="True to pass --verbose flag to arduino-cli compile and upload"
),
+ server.ProjectOption(
+ "warning_as_error",
+ choices=(True, False),
+ help="Tread warnings as errors.",
Review comment:
Sorry, I missed that type on the last review: s/Tread/Treat?
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -265,6 +269,11 @@ def _get_nrf_device_args(options):
"config_main_stack_size",
help="Sets CONFIG_MAIN_STACK_SIZE for Zephyr board.",
),
+ server.ProjectOption(
+ "warning_as_error",
+ choices=(True, False),
+ help="Tread warnings as errors.",
Review comment:
Same here: s/Tread/Treat/
--
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 #9274: [microTVM] Add platform version check to template project
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9274:
URL: https://github.com/apache/tvm/pull/9274#discussion_r730153277
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -356,13 +361,16 @@ def _get_platform_version(self) -> str:
if "VERSION_MINOR" in line:
version_minor = line.split("=")[1]
- return f"{version_major}.{version_minor}"
+ return float(f"{version_major}.{version_minor}")
def generate_project(self, model_library_format_path, standalone_crt_dir, project_dir, options):
# Check Zephyr version
version = self._get_platform_version()
if version != ZEPHYR_VERSION:
- raise ValueError(f"Zephyr version does not math: {version} != {ZEPHYR_VERSION}")
+ message = f"Zephyr version does not math: {version} != {ZEPHYR_VERSION}"
Review comment:
This still needs to get fixed. While you're at it, maybe it could give an additional hint to the user about what zephyr version we're expecting? Maybe something like:
```
f"Zephyr version found is not supported: found {version}, expected {ZEPHYR_VERSION}."
```
Arduino part will need to get updated accordingly.
--
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