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/12/06 19:42:26 UTC
[GitHub] [tvm] mehrdadh opened a new pull request #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
mehrdadh opened a new pull request #9663:
URL: https://github.com/apache/tvm/pull/9663
Fixing this issue: https://github.com/apache/tvm/issues/9661
Added tests for this error.
@gromero @leandron
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#discussion_r764787623
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -75,8 +75,10 @@ def test_tvmc_model_build_only(board, output_dir):
target, platform = _get_target_and_platform(board)
if not os.path.isabs(output_dir):
- output_dir = pathlib.Path(os.path.abspath(output_dir))
- os.mkdir(output_dir)
+ out_dir_temp = os.path.abspath(output_dir)
Review comment:
I think both `if`s (if `isabs()` and if `isdir()`) in this part can be avoid by:
```
diff --git a/tests/micro/common/test_tvmc.py b/tests/micro/common/test_tvmc.py
index eb0b3a628..9de3d144b 100644
--- a/tests/micro/common/test_tvmc.py
+++ b/tests/micro/common/test_tvmc.py
@@ -140,11 +140,9 @@ def test_tvmc_model_build_only(board, output_dir):
def test_tvmc_model_run(board, output_dir):
target, platform = _get_target_and_platform(board)
- if not os.path.isabs(output_dir):
- out_dir_temp = os.path.abspath(output_dir)
- if os.path.isdir(out_dir_temp):
- shutil.rmtree(out_dir_temp)
- os.mkdir(out_dir_temp)
+ out_dir_temp = os.path.abspath(output_dir)
+ shutil.rmtree(out_dir_temp, ignore_errors=True)
+ os.mkdir(out_dir_temp)
model_path = model_path = download_testdata(MODEL_URL, MODEL_FILE, module="data")
tar_path = str(output_dir / "model.tar")
```
But if course won't hold the patch for that. That could be done on a follow-on clean up.
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#issuecomment-988212497
@leandron thanks for the review. I have updated the description.
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#discussion_r764406816
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -118,17 +126,25 @@ def test_tvmc_model_build_only(board):
["micro", "build", project_dir, platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: build"
+ shutil.rmtree(output_dir)
@pytest.mark.requires_hardware
@tvm.testing.requires_micro
-def test_tvmc_model_run(board):
+@pytest.mark.parametrize(
+ "output_dir,",
+ [pathlib.Path("./tvmc_relative_path_test"), pathlib.Path(tempfile.mkdtemp())],
+)
+def test_tvmc_model_run(board, output_dir):
target, platform = _get_target_and_platform(board)
+ if not os.path.isabs(output_dir):
Review comment:
Same here: it defeats the main purpose of testing relative paths as mentioned above. It needs to get fixed.
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#discussion_r764471742
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -118,17 +126,25 @@ def test_tvmc_model_build_only(board):
["micro", "build", project_dir, platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: build"
+ shutil.rmtree(output_dir)
@pytest.mark.requires_hardware
@tvm.testing.requires_micro
-def test_tvmc_model_run(board):
+@pytest.mark.parametrize(
+ "output_dir,",
+ [pathlib.Path("./tvmc_relative_path_test"), pathlib.Path(tempfile.mkdtemp())],
+)
+def test_tvmc_model_run(board, output_dir):
target, platform = _get_target_and_platform(board)
+ if not os.path.isabs(output_dir):
Review comment:
done.
##########
File path: python/tvm/driver/tvmc/micro.py
##########
@@ -236,18 +236,26 @@ def drive_micro(args):
args.subcommand_handler(args)
+def get_project_dir(args: argparse.Namespace) -> str:
Review comment:
moved the function and changed input type.
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -66,13 +67,20 @@ def test_tvmc_exist(board):
@tvm.testing.requires_micro
-def test_tvmc_model_build_only(board):
+@pytest.mark.parametrize(
+ "output_dir,",
+ [pathlib.Path("./tvmc_relative_path_test"), pathlib.Path(tempfile.mkdtemp())],
+)
+def test_tvmc_model_build_only(board, output_dir):
target, platform = _get_target_and_platform(board)
+ if not os.path.isabs(output_dir):
Review comment:
Thanks for catching this. Initially the test was correct, I added that last piece to create the directory before running tvmc command which made the test wrong. Now it should be correct. Please let me know
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#discussion_r764404512
##########
File path: python/tvm/driver/tvmc/micro.py
##########
@@ -236,18 +236,26 @@ def drive_micro(args):
args.subcommand_handler(args)
+def get_project_dir(args: argparse.Namespace) -> str:
Review comment:
That function will be used in other contexts too, beyond `tvmc micro`. For instance, in `tvmc run ---device micro ...`, hence it should be moved to `common.py` and should not rely on any specific `Namespace` attribute, so it must accept a `pathlib.Path` or simply a `str` type.
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -66,13 +67,20 @@ def test_tvmc_exist(board):
@tvm.testing.requires_micro
-def test_tvmc_model_build_only(board):
+@pytest.mark.parametrize(
+ "output_dir,",
+ [pathlib.Path("./tvmc_relative_path_test"), pathlib.Path(tempfile.mkdtemp())],
+)
+def test_tvmc_model_build_only(board, output_dir):
target, platform = _get_target_and_platform(board)
+ if not os.path.isabs(output_dir):
Review comment:
This check is actually defeating the main purpose of this test regarding relative paths since `project_dir` derives from `output_dir` that is resolved here (made absolute here), so `project_dir`ends up not to be relative anymore. This needs to get fixed. Only as an example and reference, a change like that on top of this PR makes the test to stress the relative paths:
```
diff --git a/tests/micro/common/test_tvmc.py b/tests/micro/common/test_tvmc.py
index 200c7f616..419dd9405 100644
--- a/tests/micro/common/test_tvmc.py
+++ b/tests/micro/common/test_tvmc.py
@@ -110,7 +110,7 @@ def test_tvmc_model_build_only(board, output_dir):
create_project_cmd = [
"micro",
"create-project",
- project_dir,
+ "./project",
tar_path,
platform,
"--project-option",
@@ -123,11 +123,11 @@ def test_tvmc_model_build_only(board, output_dir):
assert cmd_result == 0, "tvmc micro failed in step: create-project"
cmd_result = _run_tvmc(
- ["micro", "build", project_dir, platform, "--project-option", f"{platform}_board={board}"]
+ ["micro", "build", "./project", platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: build"
shutil.rmtree(output_dir)
-
+ shutil.rmtree("./project")
@pytest.mark.requires_hardware
@tvm.testing.requires_micro
@@ -174,7 +174,7 @@ def test_tvmc_model_run(board, output_dir):
create_project_cmd = [
"micro",
"create-project",
- project_dir,
+ "./project",
tar_path,
platform,
"--project-option",
@@ -187,12 +187,12 @@ def test_tvmc_model_run(board, output_dir):
assert cmd_result == 0, "tvmc micro failed in step: create-project"
cmd_result = _run_tvmc(
- ["micro", "build", project_dir, platform, "--project-option", f"{platform}_board={board}"]
+ ["micro", "build", "./project", platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: build"
cmd_result = _run_tvmc(
- ["micro", "flash", project_dir, platform, "--project-option", f"{platform}_board={board}"]
+ ["micro", "flash", "./project", platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: flash"
@@ -201,7 +201,7 @@ def test_tvmc_model_run(board, output_dir):
"run",
"--device",
"micro",
- project_dir,
+ "./project",
"--project-option",
f"{platform}_board={board}",
"--fill-mode",
@@ -210,6 +210,7 @@ def test_tvmc_model_run(board, output_dir):
)
assert cmd_result == 0, "tvmc micro failed in step: run"
shutil.rmtree(output_dir)
+ shutil.rmtree("./project")
if __name__ == "__main__":
```
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -118,17 +126,25 @@ def test_tvmc_model_build_only(board):
["micro", "build", project_dir, platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: build"
+ shutil.rmtree(output_dir)
@pytest.mark.requires_hardware
@tvm.testing.requires_micro
-def test_tvmc_model_run(board):
+@pytest.mark.parametrize(
+ "output_dir,",
+ [pathlib.Path("./tvmc_relative_path_test"), pathlib.Path(tempfile.mkdtemp())],
+)
+def test_tvmc_model_run(board, output_dir):
target, platform = _get_target_and_platform(board)
+ if not os.path.isabs(output_dir):
Review comment:
Same here: it defeats the main purpose of testing relative paths as mentioned above. It need to get fixed.
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#discussion_r764787623
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -75,8 +75,10 @@ def test_tvmc_model_build_only(board, output_dir):
target, platform = _get_target_and_platform(board)
if not os.path.isabs(output_dir):
- output_dir = pathlib.Path(os.path.abspath(output_dir))
- os.mkdir(output_dir)
+ out_dir_temp = os.path.abspath(output_dir)
Review comment:
I think both `if`s (if `isabs()` and if `isdir()`) in this part can be avoid by:
```
diff --git a/tests/micro/common/test_tvmc.py b/tests/micro/common/test_tvmc.py
index eb0b3a628..9de3d144b 100644
--- a/tests/micro/common/test_tvmc.py
+++ b/tests/micro/common/test_tvmc.py
@@ -140,11 +140,9 @@ def test_tvmc_model_build_only(board, output_dir):
def test_tvmc_model_run(board, output_dir):
target, platform = _get_target_and_platform(board)
- if not os.path.isabs(output_dir):
- out_dir_temp = os.path.abspath(output_dir)
- if os.path.isdir(out_dir_temp):
- shutil.rmtree(out_dir_temp)
- os.mkdir(out_dir_temp)
+ out_dir_temp = os.path.abspath(output_dir)
+ shutil.rmtree(out_dir_temp, ignore_errors=True)
+ os.mkdir(out_dir_temp)
model_path = model_path = download_testdata(MODEL_URL, MODEL_FILE, module="data")
tar_path = str(output_dir / "model.tar")
```
But of course won't hold the patch for that. That could be done on a follow-on clean up.
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#issuecomment-988770798
@leandron @areusch I think this is ready to be 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] leandron merged pull request #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
leandron merged pull request #9663:
URL: https://github.com/apache/tvm/pull/9663
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#discussion_r764406340
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -66,13 +67,20 @@ def test_tvmc_exist(board):
@tvm.testing.requires_micro
-def test_tvmc_model_build_only(board):
+@pytest.mark.parametrize(
+ "output_dir,",
+ [pathlib.Path("./tvmc_relative_path_test"), pathlib.Path(tempfile.mkdtemp())],
+)
+def test_tvmc_model_build_only(board, output_dir):
target, platform = _get_target_and_platform(board)
+ if not os.path.isabs(output_dir):
Review comment:
This check is actually defeating the main purpose of this test regarding relative paths since `project_dir` derives from `output_dir` that is resolved here (made absolute here), so `project_dir`ends up not to be relative anymore. This needs to get fixed. Only as an example and reference, a change like the following on top of this PR makes the test to stress the relative paths:
```
diff --git a/tests/micro/common/test_tvmc.py b/tests/micro/common/test_tvmc.py
index 200c7f616..419dd9405 100644
--- a/tests/micro/common/test_tvmc.py
+++ b/tests/micro/common/test_tvmc.py
@@ -110,7 +110,7 @@ def test_tvmc_model_build_only(board, output_dir):
create_project_cmd = [
"micro",
"create-project",
- project_dir,
+ "./project",
tar_path,
platform,
"--project-option",
@@ -123,11 +123,11 @@ def test_tvmc_model_build_only(board, output_dir):
assert cmd_result == 0, "tvmc micro failed in step: create-project"
cmd_result = _run_tvmc(
- ["micro", "build", project_dir, platform, "--project-option", f"{platform}_board={board}"]
+ ["micro", "build", "./project", platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: build"
shutil.rmtree(output_dir)
-
+ shutil.rmtree("./project")
@pytest.mark.requires_hardware
@tvm.testing.requires_micro
@@ -174,7 +174,7 @@ def test_tvmc_model_run(board, output_dir):
create_project_cmd = [
"micro",
"create-project",
- project_dir,
+ "./project",
tar_path,
platform,
"--project-option",
@@ -187,12 +187,12 @@ def test_tvmc_model_run(board, output_dir):
assert cmd_result == 0, "tvmc micro failed in step: create-project"
cmd_result = _run_tvmc(
- ["micro", "build", project_dir, platform, "--project-option", f"{platform}_board={board}"]
+ ["micro", "build", "./project", platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: build"
cmd_result = _run_tvmc(
- ["micro", "flash", project_dir, platform, "--project-option", f"{platform}_board={board}"]
+ ["micro", "flash", "./project", platform, "--project-option", f"{platform}_board={board}"]
)
assert cmd_result == 0, "tvmc micro failed in step: flash"
@@ -201,7 +201,7 @@ def test_tvmc_model_run(board, output_dir):
"run",
"--device",
"micro",
- project_dir,
+ "./project",
"--project-option",
f"{platform}_board={board}",
"--fill-mode",
@@ -210,6 +210,7 @@ def test_tvmc_model_run(board, output_dir):
)
assert cmd_result == 0, "tvmc micro failed in step: run"
shutil.rmtree(output_dir)
+ shutil.rmtree("./project")
if __name__ == "__main__":
```
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#issuecomment-988318054
@mehrdadh btw, a nit: would you mind if we stick with the tag `[microTVM]` (case sensitive)?
--
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 #9663: [TVMC][MicroTVM] Fix tvmc micro `project_dir` arg relative path
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #9663:
URL: https://github.com/apache/tvm/pull/9663#discussion_r764787623
##########
File path: tests/micro/common/test_tvmc.py
##########
@@ -75,8 +75,10 @@ def test_tvmc_model_build_only(board, output_dir):
target, platform = _get_target_and_platform(board)
if not os.path.isabs(output_dir):
- output_dir = pathlib.Path(os.path.abspath(output_dir))
- os.mkdir(output_dir)
+ out_dir_temp = os.path.abspath(output_dir)
Review comment:
I think both `if`s (if `isabs()` and if `isdir()`) in this part can be avoided by:
```
diff --git a/tests/micro/common/test_tvmc.py b/tests/micro/common/test_tvmc.py
index eb0b3a628..9de3d144b 100644
--- a/tests/micro/common/test_tvmc.py
+++ b/tests/micro/common/test_tvmc.py
@@ -140,11 +140,9 @@ def test_tvmc_model_build_only(board, output_dir):
def test_tvmc_model_run(board, output_dir):
target, platform = _get_target_and_platform(board)
- if not os.path.isabs(output_dir):
- out_dir_temp = os.path.abspath(output_dir)
- if os.path.isdir(out_dir_temp):
- shutil.rmtree(out_dir_temp)
- os.mkdir(out_dir_temp)
+ out_dir_temp = os.path.abspath(output_dir)
+ shutil.rmtree(out_dir_temp, ignore_errors=True)
+ os.mkdir(out_dir_temp)
model_path = model_path = download_testdata(MODEL_URL, MODEL_FILE, module="data")
tar_path = str(output_dir / "model.tar")
```
But of course won't hold the patch for that. That could be done on a follow-on clean up.
--
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