[microTVM] Refactor RVM scripts and fix DNS network issue by mehrdadh · Pull Request #11741 · apache/tvm

mehrdadh · 2022-06-15T23:44:23Z

mehrdadh · 2022-06-15T23:48:23Z

guberti

Looks good, just a few nits. Thanks for removing so much duplicate code!

guberti · 2022-06-16T00:05:10Z

apps/microtvm/reference-vm/arduino/base-box/base_box_setup.sh

-sudo apt-get --purge remove modemmanager  # required to access serial ports.
+# Install common configs
+~/base_box_setup_common.sh
+rm -rf ~/base_box_setup_common.sh


Do we need the -rf flags here or in the other base_box_setup.sh?

nope, changed them to -f

guberti · 2022-06-16T00:06:50Z

apps/microtvm/reference-vm/arduino/base-box/base_box_setup.sh

-# ONNX deps
-sudo apt install -y protobuf-compiler libprotoc-dev
+# Poetry
+sed -i "/^# If not running interactively,/ i source \$HOME/.poetry/env" ~/.bashrc


Add a comment explaining this line? Why are we looking for "If not running interactively"?

this is just added to top of the .bashrc file. Also I only moved this part from other scripts

Can we just add the line to the top of the file then, instead of relying on the exact wording of the comment? Say,

sed -i "1i source \$HOME/.poetry/env" ~/.bashrc

May be out of scope for this PR, though.

guberti · 2022-06-16T00:08:08Z

apps/microtvm/reference-vm/base_box_setup_common.sh

+
+# Core
+sudo ~/ubuntu_install_core.sh
+rm -rf ~/ubuntu_install_core.sh


Same as above - do we need -rf flags?

guberti · 2022-06-16T00:10:25Z

apps/microtvm/reference-vm/base_box_setup_common.sh

+# Fix network DNS issue
+sudo sed -i 's/DNSSEC=yes/DNSSEC=no/' /etc/systemd/resolved.conf
+sudo systemctl restart systemd-resolved
+sudo cat /etc/systemd/resolved.conf


Do we want to keep printing /etc/systemd/resolved.conf to the console?

good catch, removed it!

guberti · 2022-06-16T00:13:21Z

apps/microtvm/reference-vm/base_box_setup_common.sh

+
+# Cmake
+# NOTE: latest cmake cannot be installed due to
+# https://github.com/zephyrproject-rtos/zephyr/issues/30232


This GitHub issue has been closed - are we still blocked from using the latest cmake? This will also downgrade the version of cmake being used for Arduino.

I should remove this note. Because the reason I kept this is because the default cmake that was installed without this command, it was really old. so I'm just keeping this to force it to update cmake from version like 3.13 to 3.22.2

mehrdadh

@guberti thanks for the review! PTAL

mehrdadh · 2022-06-16T00:55:00Z

apps/microtvm/reference-vm/arduino/base-box/base_box_setup.sh

-sudo apt-get --purge remove modemmanager  # required to access serial ports.
+# Install common configs
+~/base_box_setup_common.sh
+rm -rf ~/base_box_setup_common.sh


nope, changed them to -f

mehrdadh · 2022-06-16T00:57:19Z

apps/microtvm/reference-vm/arduino/base-box/base_box_setup.sh

-# ONNX deps
-sudo apt install -y protobuf-compiler libprotoc-dev
+# Poetry
+sed -i "/^# If not running interactively,/ i source \$HOME/.poetry/env" ~/.bashrc


this is just added to top of the .bashrc file. Also I only moved this part from other scripts

mehrdadh · 2022-06-16T00:57:52Z

apps/microtvm/reference-vm/base_box_setup_common.sh

+# Fix network DNS issue
+sudo sed -i 's/DNSSEC=yes/DNSSEC=no/' /etc/systemd/resolved.conf
+sudo systemctl restart systemd-resolved
+sudo cat /etc/systemd/resolved.conf


good catch, removed it!

mehrdadh · 2022-06-16T00:58:01Z

apps/microtvm/reference-vm/base_box_setup_common.sh

+
+# Core
+sudo ~/ubuntu_install_core.sh
+rm -rf ~/ubuntu_install_core.sh


mehrdadh · 2022-06-16T00:59:16Z

apps/microtvm/reference-vm/base_box_setup_common.sh

+
+# Cmake
+# NOTE: latest cmake cannot be installed due to
+# https://github.com/zephyrproject-rtos/zephyr/issues/30232


I should remove this note. Because the reason I kept this is because the default cmake that was installed without this command, it was really old. so I'm just keeping this to force it to update cmake from version like 3.13 to 3.22.2

guberti

Two more nits, but should be fine to merge as-is.

guberti · 2022-06-16T01:22:11Z

apps/microtvm/reference-vm/arduino/base-box/base_box_setup.sh

-# ONNX deps
-sudo apt install -y protobuf-compiler libprotoc-dev
+# Poetry
+sed -i "/^# If not running interactively,/ i source \$HOME/.poetry/env" ~/.bashrc


Can we just add the line to the top of the file then, instead of relying on the exact wording of the comment? Say,

sed -i "1i source \$HOME/.poetry/env" ~/.bashrc

May be out of scope for this PR, though.

guberti · 2022-06-16T01:23:25Z

apps/microtvm/reference-vm/base_box_setup_common.sh

+
+# Python
+sudo ~/ubuntu_install_python.sh
+rm -f ~/ubuntu_install_python.sh


Are these scripts write protected? Do we need the -f flag either?

mehrdadh · 2022-06-16T16:13:02Z

@guberti thanks for the review! Let's add those changes in a separate PR since I'm trying to unblock the CI.

* apache#11470 and apache#11741.

* refactor scripts * address comments

* #11470 and #11741.

mehrdadh changed the title ~~[microTVM] Refactor RVM scripts and fix a network issue~~ [microTVM] Refactor RVM scripts and fix DNS network issue Jun 15, 2022

github-actions bot requested a review from areusch June 15, 2022 23:45

mehrdadh force-pushed the microtvm/fix_pip_version branch from 88fc13f to 58e69ea Compare June 15, 2022 23:52

github-actions bot requested a review from gromero June 15, 2022 23:52

guberti approved these changes Jun 16, 2022

View reviewed changes

refactor scripts

2e032c4

mehrdadh force-pushed the microtvm/fix_pip_version branch from 58e69ea to 2e032c4 Compare June 16, 2022 00:51

address comments

0b7df04

mehrdadh commented Jun 16, 2022

View reviewed changes

guberti approved these changes Jun 16, 2022

View reviewed changes

areusch approved these changes Jun 16, 2022

View reviewed changes

areusch merged commit 7a5f4e0 into apache:main Jun 16, 2022

mehrdadh deleted the microtvm/fix_pip_version branch June 16, 2022 19:49

areusch added a commit to areusch/tvm that referenced this pull request Jun 16, 2022

Fix CI break due to concurrent merge.

782455b

* apache#11470 and apache#11741.

areusch mentioned this pull request Jun 16, 2022

[skip ci] Fix CI break due to concurrent merge. #11753

Merged

areusch added a commit to areusch/tvm that referenced this pull request Jun 16, 2022

Fix CI break due to concurrent merge.

31d5529

* apache#11470 and apache#11741.

zxybazh pushed a commit to zxybazh/tvm that referenced this pull request Jun 16, 2022

[microTVM] Refactor RVM scripts and fix DNS network issue (apache#11741)

56958cd

* refactor scripts * address comments

areusch added a commit that referenced this pull request Jun 16, 2022

Fix CI break due to concurrent merge. (#11753)

b4a77ac

* #11470 and #11741.

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

Conversation

mehrdadh commented Jun 15, 2022 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mehrdadh commented Jun 15, 2022

Uh oh!

guberti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehrdadh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guberti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mehrdadh commented Jun 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

mehrdadh commented Jun 15, 2022 •

edited by github-actions bot

Loading