-
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
Rename runtime-config to executor-config and add documentation for Model Library Format #8270
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.
It looks very clear documentation. I think users would benefit also from a short code snippet giving an example of how to generate a MLF package.
Also a few nits and suggestions.
docs/dev/model_library_format.rst
Outdated
-------------------------- | ||
|
||
TVM traditionally exports generated libraries as Dynamic Shared Objects | ||
(e.g. DLLs (Windows) or .so (linux)). Inference can be performed on those libraries by loading them |
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.
(e.g. DLLs (Windows) or .so (linux)). Inference can be performed on those libraries by loading them | |
(e.g. DLLs (Windows) or .so (linux)). Inferences can be performed on those libraries by loading them |
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.
done
docs/dev/model_library_format.rst
Outdated
into an executable using ``libtvm_runtime.so``. This process is very dependent on services provided | ||
by traditional OS. | ||
|
||
For deployment to unconventional platforms (e.g. those lacking traditional OS), the microTVM project |
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 use embedded instead?
For deployment to unconventional platforms (e.g. those lacking traditional OS), the microTVM project | |
For deployment to embedded platforms (e.g. those lacking traditional OS), the microTVM project |
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 feel like it's not strictly limited to embedded though
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 agree.
to that end, I think we should word this as a general output format that produces what strictly TVM (code)generates. The creation of .so/.dll requires external toolchains called after tvm compilation -- such as LLVM and C compilers.
WDYT ?
Model Library Format | ||
==================== |
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 it would be beneficial to introduce MLF as an acronym for Model Lbrary Format early in the document, so that we can get people used to it?
Model Library Format | |
==================== | |
Model Library Format (MLF) | |
========================== |
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 haven't used the acronym yet in this doc though. but i agree it's an easy shorthand for the format. maybe it would make sense more in tvmc docs, where it's a command-line param? wdyt?
Would it be worth adding a JSON Schema for |
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 this PR!
Mostly good, I have just a few suggestions.
docs/dev/model_library_format.rst
Outdated
* ``<target>`` - Identifies the TVM target on which the code should run. Currently, only ``host`` | ||
is supported. | ||
* ``<unique_name>`` - A unique slug identifying this file. Currently ``lib<n>``, with ``<n>>` an | ||
autoincrementing integer. |
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 change to auto-incrementing?
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.
done
``parameters`` | ||
^^^^^^^^^^^^^^ | ||
|
||
Contains machine-parseable parameters. A variety of formats may be provided, but at present, only |
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.
parsable?
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.
done
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.
Looks good, just one more nit -- (bar other's comments)
docs/dev/model_library_format.rst
Outdated
`Memory Usage Summary`_. | ||
- ``model_name``: The name of this model (e.g. the ``name`` parameter supplied to | ||
``tvm.relay.build``). | ||
- ``runtimes``: A list of runtimes supported by this model. Currently, this list is always |
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 executors ?
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 :)
@leandron @manupa-arm please take another look! |
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.
Did some (final) nit picking! hope you dont mind.
Mostly text changes.
docs/dev/model_library_format.rst
Outdated
into an executable using ``libtvm_runtime.so``. This process is very dependent on services provided | ||
by traditional OS. | ||
|
||
For deployment to unconventional platforms (e.g. those lacking traditional OS), the microTVM project |
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 agree.
to that end, I think we should word this as a general output format that produces what strictly TVM (code)generates. The creation of .so/.dll requires external toolchains called after tvm compilation -- such as LLVM and C compilers.
WDYT ?
docs/dev/model_library_format.rst
Outdated
|
||
For deployment to unconventional platforms (e.g. those lacking traditional OS), the microTVM project | ||
can be used to export a generated library in pieces. In this case, microTVM provides another output | ||
format, Model Library Format. Model Library Format is a tarball containing a file for each part 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.
I think we should not use "microTVM provides another output format", though we could use microTVM as an example this output format becomes useful.
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.
done
docs/dev/model_library_format.rst
Outdated
format, Model Library Format. Model Library Format is a tarball containing a file for each part of | ||
the TVM compiler output. | ||
|
||
What can be Exported |
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.
What can be Exported | |
What can be exported ? |
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.
just nit picking :)
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.
done
docs/dev/model_library_format.rst
Outdated
Directory Layout | ||
---------------- | ||
|
||
Model Library Format is traditionally contained within a tarball. All paths are relative to the root |
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.
Model Library Format is traditionally contained within a tarball. All paths are relative to the root | |
Model Library Format is contained within a tarball. All paths are relative to the root |
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.
nit: we dont have any other way. Do we?
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 point :)
docs/dev/model_library_format.rst
Outdated
sub-target which describes that relay backend used for that ``device_type``. | ||
- ``version``: A numeric version number that identifies the format used in this Model Library | ||
Format. This number is incremented when the metadata structure or on-disk structure changes. | ||
This document reflects version ``3``. |
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.
This document reflects version ``3``. | |
This document reflects version ``5``. |
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.
done
@manupa-arm it won't let me continue your earlier thread, but basically i'd agree with that idea in the medium term however we cannot yet export BYOC Module in MLF. once that is possible, i think we should promote it to a general TVM output format. |
@manupa-arm please take another look |
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
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 @mehrdadh @Mousius @manupa-arm, this is merged now! |
…del Library Format (apache#8270) * Rename runtime-config to executor-config. * Add documentation. * address comments, make tests pass * fix unit test * fix sphinx doc errors * address manupa comments
…del Library Format (apache#8270) * Rename runtime-config to executor-config. * Add documentation. * address comments, make tests pass * fix unit test * fix sphinx doc errors * address manupa comments
This PR:
runtime-config
toexecutor-config
to match Rename GraphRuntime to GraphExecutor #7653.@mehrdadh @leandron @gromero @manupa-arm @jwfromm @guberti @Mousius @giuseros