Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[microTVM] Refactor platform used as board name in microTVM #8940

Merged
merged 14 commits into from
Sep 9, 2021

Conversation

mehrdadh
Copy link
Member

@mehrdadh mehrdadh commented Sep 6, 2021

Since now we are supporting two microtvm platforms (zephyr, arduino), it is confusing to use platform for both zephyr/arduino and the device target (e.g. qemu_x86/nrf5340dk and due/feathers2) that we support for each platform. This PR refactors the later platform to (arduino/zephyr)_board.

This PR also moves the supported microtvm devices for each platform to its microtvm_api_server.py to create a single source for supported devices.

cc @areusch @gromero

Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. Thanks for the change. The first time I crossed with the option --microtvm-platforms and found it was actually listing the boards it stroke me with a certain oddness, so I think it makes sense to change it, yeah.

On microTVM world that overload of what a platform means is imho understandable. I believe it happens because Zephyr and Arduino are regarded (correctly in my opinion) as platforms for microTVM and at the same time a board (for instance mps2_an521, among others) is considered a platform for the Zephyr project, thus since Zephyr was the first platform widely experimented on microTVM what is considered a platform by microTVM was the same what was considered by Zephyr, hence the confusion with board and platform I believe.

But as you said now there is the Arduino platform and it makes sense to review the terms.

That said, on various contexts, like on the USB vid and pid in base-tool-box.py where the USB ids are actually tied to a dev board (like the disco board), but mainly because I think that the devices in MICRO_DEVICES must become the same in the list returned by the Project API query info method as choices for the platform board option ( optionzephyr_board and arduino_board more precisely) I think the best fit here would be to replace "platform" by "board" in all cases I can think of now. Wdyt of s/MICRO_DEVICES/BOARDS/?

apps/microtvm/reference-vm/README.md Outdated Show resolved Hide resolved
tests/micro/arduino/conftest.py Outdated Show resolved Hide resolved
tests/micro/zephyr/conftest.py Outdated Show resolved Hide resolved
@mehrdadh
Copy link
Member Author

mehrdadh commented Sep 7, 2021

@gromero thanks for the feedbacks. I think micro_board is a good option too. Initially I tried to avoid using micro_board term since Zephyr used board and I wanted to make sure we can differentiate those. But now that I think of it we could have micro_board which maps to (target, zephyr_board) in case of Zephyr and (target, arduino_board) in case of Arduino. Based on my understanding, "board" is unique in terms of TVM and microTVM so it shouldn't be confusing with another thing.

I'll be happy to get more opinion on this before changing it to micro_board. cc @areusch @guberti @manupa-arm @leandron @Mousius

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehrdadh thanks for raising this. I think we have several concepts here. here's how i think of them:

  • platform -- which RTOS we are using
  • device -- which hardware is being tested against
  • target -- the TVM target string, which for now means the whole system but in the future (e.g. with heterogenous execution) may mean just e.g. the CPU or accelerator
  • this flag, which kind of does double-duty

what do you think about --test-setting or --test-environment? i think that might be a bit more accurate for the test_zephyr.py and base_box_tool.py

apps/microtvm/reference-vm/README.md Outdated Show resolved Hide resolved
tests/micro/arduino/conftest.py Outdated Show resolved Hide resolved
@areusch
Copy link
Contributor

areusch commented Sep 7, 2021

i chatted a bit more with @mehrdadh about the question of how to represent the (target, arduino_board) or (target, zephyr_board) pairs. It sort of seems like the neatest solution is to place the mapping in microtvm_api_server.py and make server_info_query return a suggested target based on the ProjectOption provided by the user to e.g. tvmc. My hesitations in doing this are:

  1. right now Project API is entirely "downstream" of TVM--it just consumes a specified artifact of TVM and doesn't configure TVM. doing this would change that relationship. this means if TVM e.g. evolves the defintion of target to split it into pieces, Project API implementations may need to be updated. I'm not sure this is a big deal, but it's something to think on.
  2. Since we would likely move the tvm.target.micro shorthands into Project API servers, it means that all platforms are suggested to make a Project API implementation even if they don't use the project generator. This wouldn't be required, though--you could still specify the target the old-fashioend way. it just could be a burden in some cases. we could always alleviate it by allowing platforms to create something specific to them.

right now this is essentially test configuration. I think we could continue to import microtvm_api_server from these tests until we resolve this question.

Copy link
Member

@guberti guberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of changing microtvm-platforms to microtvm-device, but we need to settle on a better solution is passed between the client and the server - see above comments.

apps/microtvm/reference-vm/README.md Outdated Show resolved Hide resolved
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ ./base-box-tool.py --provider virtualbox test --microtvm-device=stm32f746xx_disco zephyr
$ ./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.

tests/micro/arduino/conftest.py Outdated Show resolved Hide resolved
tests/micro/arduino/conftest.py Outdated Show resolved Hide resolved
@mehrdadh
Copy link
Member Author

mehrdadh commented Sep 8, 2021

@areusch @gromero @guberti Thanks for the reviews.
I have updated conftest.py for each platform to use project options for board choices and updated the terminology to use zephyr-board and arduino-board for each.

As a result of using zephyr-board directly for zephyr tests, we have to remove nrf5340dk as a choice and use the board name which is nrf5340dk_nrf5340_cpuapp. I updated the tutorial that uses this board.

Same thing for stm32f746xx_nucleowhich is now we use nucleo_f746zg.

@mehrdadh mehrdadh changed the title [microTVM] Change platform to device [microTVM] Refactor platform used as board name in microTVM Sep 8, 2021
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mehrdadh i think this looks okay now. i think we still have some places we are importing microtvm_api_server.py, but we're not making those worse with this PR. i'd like @guberti and @gromero to comment but this seems mergeable to me

"""Returns a dict mapping board to target model"""
template = project.TemplateProject.from_directory(TEMPLATE_PROJECT_DIR)
project_options = template.info()["project_options"]
for option in project_options:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we should make a helper for this, can be a follow-on

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree yep

Copy link
Member

@guberti guberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

apps/microtvm/reference-vm/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks Mehrdad!

@areusch areusch merged commit f6a4044 into apache:main Sep 9, 2021
@mehrdadh mehrdadh deleted the micro_refactor_platform branch September 9, 2021 20:28
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants