-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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] Project API infrastructure #8380
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@areusch Thanks for working on this!
I'll review it bit by bit instead of at once.
Please see my first comment inline.
Besides that, some other general comments:
-
A concern from Matt (https://discuss.tvm.apache.org/t/rfc-tvm-project-api/9449) still resonates with me: is it really necessary to have two entry points for the server? i.e. a .sh and .py entry point? One would have to craft an RPC server which accepts two pipes (read and write) right? I don't see why we would need that, unless you have other plans for the future :)
-
Instead of a single query (
server_info_query()
) returning a series of predetermined values (inserver.py
,_dispatch_server_info_query()
) would it be better also to have it accepting a single key a returning only its value instead of a series of values? It's that I believe the number of keys return will increase over time, so it will increasingly return probably more and more data that won't be used at all in some scenarios. -
From a TVMC perspective, for the
run
command, in the Graph executor case, tvmc will need the JSON for the graph, which will be located in/model/executor-config/graph/graph.json'
. Of course the project dir was passed to the 'run' in the command line, so it can be easily determined by tvmc. Nonetheless that patch will be kind of hardcoded by the client (tvmc in this case, so I'm wondering if it should actually be returned via the Project API, byserver_info_query()
. Another way would be to queryserver_info_query()
for keymodel_library_format_path
and then untar it locally to find themod.json
file, however that case also sounds like a shortcut to by pass the Project API design. Could you please share your thoughts on that case, i.e. what's the correct way for a client to have access to themod.json
? -
That said, I'm still struggling to understand why a RPC client-server model is necessary here. The client starts the server as a subprocess and passes to it (to the server) a read and write local pipe. This is a bit odd to me. A RPC that should have a remote part at some point, but that is used as something quite local afaics. Why something simpler with a
importlib.import_module
for instance won't work here? The RPC implementation works nice, so I just would like to understand the rational behind it.
Thank you.
self._info = self._client.server_info_query() | ||
if self._info['is_template']: | ||
raise TemplateProjectError() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think server_info_query()
must be exposed as well just like build()
and friends, like:
index 096db4bdd..fb6e0b94b 100644
--- a/python/tvm/micro/project.py
+++ b/python/tvm/micro/project.py
@@ -51,6 +51,9 @@ class GeneratedProject:
if self._info['is_template']:
raise TemplateProjectError()
+ def server_info_query(self):
+ return self._client.server_info_query()
+
def build(self):
self._client.build(self._options)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you say why? right now this code is just attempting to document the uses of project_api. i think we don't need to expose that here if it's not called. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@areusch I thought of this code as a convenient wrapper around TemplateProject
class and, ultimately, around project_api.client module. Hence in the use case where a program wants to simply query some info only about the template project (without generating a project based on the template dir) it would use the server_info_query()
method. For instance, with the most recent code there is now a new options
'project_type
(which can be currently host_driven
or aot_driven
. TVMC create-project command would need to accept an argument specifying which one it can take. Since Client API detects it automatically based on what's in src/
, it would be matter of querying the project_type
s available by using TemplateProject.server_info_query
to show the user the options available - we would need also create an associated wrapper like tvmc.micro.project.template_project
On a second run the user woulds already know the right option and pass to TVMC create-project command, which by its turn would that time call tvm.micro.project.create_project
.
Maybe I've misunderstood some project.py role. Let me know if it's not clear the use case I'm thinking of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gromero ah gotcha. yeah so i view this more as an internal interface rather than an external one. as such, i do think that a server_info_query() may have a place here. maybe better for your use case would be a function get_project_options
which would call server_info_query
if it hadn't yet been, and then return the project_options
. the idea here is to document all of TVM's uses of Project API all in once place. since there isn't that use case today in the code, it isn't there yet. however, you should feel free to add such functions to project.py as needed.
does that sound ok to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@areusch Sounds good!
055f329
to
86a9b07
Compare
@@ -1,361 +0,0 @@ | |||
# Licensed to the Apache Software Foundation (ASF) under one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe keep this and add warning that it will be removed in the next version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i was thinking about this, one thing is that i had to make breaking changes to the apps/microtvm/zephyr
code. i don't mind leaving in the old tvm.micro.Compiler
stuff (though I'd need to adapt more Transport layers), but it might be confusing to have two different styles of Zephyr template projects. we haven't done a TVM release with this code in there, so i wonder if it's necessary to keep the Compiler stuff in there. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mehrdadh any thoughts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@areusch I agree that since there's no official release we don't commit to support older stuff. However I recommend to make it more clear in the PR title/description that this PR is removing certain modules so if anyone has a problem could raise their voice.
The other thing is that you could announce it in the microtvm community meeting and get some feedback there. My concern is that people have internal projects depending on these features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks great!
My question is that whether we need to keep the previous compiler setup and give time to community before deleting it.
d7df2b6
to
07c0f2b
Compare
07c0f2b
to
de75022
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@areusch Thanks a lot for addressing all the issues and concerns. LGTM! |
|
||
|
||
# kwargs passed to usb.core.find to find attached boards for the openocd flash runner. | ||
BOARD_USB_FIND_KW = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the nrf5340dk_nrf5340_cpuapp
need to be in there?
) | ||
f.write("# For TVMPlatformAbort().\n" "CONFIG_REBOOT=y\n" "\n") | ||
|
||
if True: # options["project_type"] == "host_driven": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may want to clean up that comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
CRT_LIBS_BY_PROJECT_TYPE = { | ||
"host_driven": "microtvm_rpc_server microtvm_rpc_common common", | ||
"aot_demo": "aot_executor memory microtvm_rpc_common common", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is self-hosted dropped in favor of AOT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @areusch for the big effort in this milestone in the project IMHO.
I discussed with @areusch one small usability point that we well need to aadress later, which is basically that the template project won't be added to Python packages generated from the TVM repo as is.
We'll need to offer some mechanism (and documentation) for a user to point to a Git repo and a path to access the template project to be used, or even maintaining an index of known-good repos and paths we know. This can be done via APIs or indeed via tvmc
(cc @gromero).
It is probably something that will confues people who are not familiar with the code base and just want to use TVM as a tool.
I'm happy for anyone to merge this once the conflicting files are solved and CI is green again. cc @tmoreau89 |
* fix random number generator prj.conf for physical hw * uncomment proper aot option
These were re-introduced in apache#8380, noticed when I went to rebase apache#8650.
These were re-introduced in apache#8380, noticed when I went to rebase apache#8650.
@leandron yeah, that's a good point. I was not aware that the template dirs would be out of the current existing TVM Python packages. How is the code in 'apps' handled currently in general? Would it make sense have another package just for the templates and add it as a dependency when on installs the other TVM packages? Or even leave the package install as optional and |
@gromero right now we haven't broadly addressed data dependencies in the wheel. i think we need to at some point build a central e.g. |
* Initial commit of API server impl. * initial commit of api client * Add TVM-side glue code to use Project API * Change tvm.micro.Session to use Project API * Rework how crt_config.h is used on the host. * use template crt_config.h for host test runtime; delete src/runtime/crt/host/crt_config.h so that it doesn't diverge from the template * bring template crt_config.h inline with the one actually in use * rename to MAX_STRLEN_DLTYPE * Create a dedicated TVM-side host crt_config.h in src/runtime/micro * Modify Transport infrastructure to work with Project API * Add host microTVM API server * Zephyr implementation of microTVM API server * move all zephyr projects to apps/microtvm/zephyr/template_project * consolidate CcompilerAnnotator * Allow model library format with c backend, add test. * Update unit tests * fix incorrect doc * Delete old Zephyr build infrastructure * Delete old build abstractions * Delete old Transport implementations and simplify module * lint * ASF header * address gromero comments * final fixes? * fix is_shutdown * fix user-facing API * fix TempDirectory / operator * Update micro_tflite tutorial * lint * fix test_crt and test_link_params * undo global micro import, hopefully fix fixture * lint * fix more tests * Address tmoreau89 comments and mehrdadh comments * fix random number generator prj.conf for physical hw * uncomment proper aot option
These were re-introduced in apache#8380, noticed when I went to rebase apache#8650.
* Initial commit of API server impl. * initial commit of api client * Add TVM-side glue code to use Project API * Change tvm.micro.Session to use Project API * Rework how crt_config.h is used on the host. * use template crt_config.h for host test runtime; delete src/runtime/crt/host/crt_config.h so that it doesn't diverge from the template * bring template crt_config.h inline with the one actually in use * rename to MAX_STRLEN_DLTYPE * Create a dedicated TVM-side host crt_config.h in src/runtime/micro * Modify Transport infrastructure to work with Project API * Add host microTVM API server * Zephyr implementation of microTVM API server * move all zephyr projects to apps/microtvm/zephyr/template_project * consolidate CcompilerAnnotator * Allow model library format with c backend, add test. * Update unit tests * fix incorrect doc * Delete old Zephyr build infrastructure * Delete old build abstractions * Delete old Transport implementations and simplify module * lint * ASF header * address gromero comments * final fixes? * fix is_shutdown * fix user-facing API * fix TempDirectory / operator * Update micro_tflite tutorial * lint * fix test_crt and test_link_params * undo global micro import, hopefully fix fixture * lint * fix more tests * Address tmoreau89 comments and mehrdadh comments * fix random number generator prj.conf for physical hw * uncomment proper aot option
* Initial commit of API server impl. * initial commit of api client * Add TVM-side glue code to use Project API * Change tvm.micro.Session to use Project API * Rework how crt_config.h is used on the host. * use template crt_config.h for host test runtime; delete src/runtime/crt/host/crt_config.h so that it doesn't diverge from the template * bring template crt_config.h inline with the one actually in use * rename to MAX_STRLEN_DLTYPE * Create a dedicated TVM-side host crt_config.h in src/runtime/micro * Modify Transport infrastructure to work with Project API * Add host microTVM API server * Zephyr implementation of microTVM API server * move all zephyr projects to apps/microtvm/zephyr/template_project * consolidate CcompilerAnnotator * Allow model library format with c backend, add test. * Update unit tests * fix incorrect doc * Delete old Zephyr build infrastructure * Delete old build abstractions * Delete old Transport implementations and simplify module * lint * ASF header * address gromero comments * final fixes? * fix is_shutdown * fix user-facing API * fix TempDirectory / operator * Update micro_tflite tutorial * lint * fix test_crt and test_link_params * undo global micro import, hopefully fix fixture * lint * fix more tests * Address tmoreau89 comments and mehrdadh comments * fix random number generator prj.conf for physical hw * uncomment proper aot option
These were re-introduced in apache#8380, noticed when I went to rebase apache#8650.
* Initial commit of API server impl. * initial commit of api client * Add TVM-side glue code to use Project API * Change tvm.micro.Session to use Project API * Rework how crt_config.h is used on the host. * use template crt_config.h for host test runtime; delete src/runtime/crt/host/crt_config.h so that it doesn't diverge from the template * bring template crt_config.h inline with the one actually in use * rename to MAX_STRLEN_DLTYPE * Create a dedicated TVM-side host crt_config.h in src/runtime/micro * Modify Transport infrastructure to work with Project API * Add host microTVM API server * Zephyr implementation of microTVM API server * move all zephyr projects to apps/microtvm/zephyr/template_project * consolidate CcompilerAnnotator * Allow model library format with c backend, add test. * Update unit tests * fix incorrect doc * Delete old Zephyr build infrastructure * Delete old build abstractions * Delete old Transport implementations and simplify module * lint * ASF header * address gromero comments * final fixes? * fix is_shutdown * fix user-facing API * fix TempDirectory / operator * Update micro_tflite tutorial * lint * fix test_crt and test_link_params * undo global micro import, hopefully fix fixture * lint * fix more tests * Address tmoreau89 comments and mehrdadh comments * fix random number generator prj.conf for physical hw * uncomment proper aot option
These were re-introduced in apache#8380, noticed when I went to rebase apache#8650.
This PR implements the Project API RFC, a plugin-like infrastructure that allows TVM to interact with build tools for third-party platforms. By implementing the Project API interface for a platform, developers can run core TVM workflows such as host-driven inference and AutoTVM without needing to build the full TVM library on the platform. This is initially targeted at microTVM use cases, where platforms don't have the OS facilities nor the flash space to run a full TVM C++ runtime nor a Python interpreter.
More discussion is available in the RFC. Implementations are provided here for Zephyr and for the POSIX subprocess test platform.
NOTE: This PR is based on top of #8072 and will merge following that one.
@mehrdadh @guberti @tqchen @jroesch @tkonolige @csullivan @leandron @u99127 @Mousius @giuseros @gromero @stoa @hogepodge