[RFC][MLF] Model Library Format with Multiple Modules by mehrdadh · Pull Request #76 · apache/tvm-rfcs

mehrdadh · 2022-05-31T16:24:45Z

This RFC proposes adding multiple module support in Model Library Format.

cc @gromero @areusch

areusch · 2022-05-31T22:10:56Z

cc @Mousius @manupa-arm @leandron

driazati · 2022-06-01T15:28:22Z

Discussion in the community meeting was pretty supportive! Some summary notes:

Name prefix option makes sense, change seems like a natural evolution of MLF
Before users would have to do the same thing by hand
Is the deserializer going to be backwards compatible?
- No loader in C++ runtime, only in project API. Eventually MLF scope will expand to include other things like multiple devices, so C++ loader might be useful later on. Code typically used along with the same TVM revision so no need to keep around the old loader. The version number can be used to provide helpful error messages.
It would be good to add a description for users on what needs to change, if anything and reference documentation

gromero

@mehrdadh Thanks for the RFC. Looks good!

Just some nits and I have a couple of questions for clarification and my own (better) understanding.

Cheers.

gromero · 2022-06-03T00:17:20Z

+[guide-level-explanation]: #guide-level-explanation
+
+Users call `export_model_library_format` function with multiple Relay builds (output from `tvm.relay.build`) or
+single a Relay build module. We change the `export_model_library_format` API to support multiple Relay Build, however 


s/single a/a single/?

Also, maybe say "Users will call export_model_library_format function ... or a single Relay build module?"

gromero · 2022-06-03T00:20:54Z

+it is backward compatible and, we handle required changes internally.
+
+Since we are including multiple modules in a single MLF file, we need to make some changes. Let's assume we
+are want to export MLF file for two Relay build modules called `mod1` and `mod2`.


d/are/

Maybe "...export A MLF file WITH two Relay build modules..."? Just a suggestion.

gromero · 2022-06-03T00:36:16Z

+    }
+    ```
+  2. There are multiple Relay text file generated per each Relay build. We propose to structure these files by using
+     module name and `.relay` extension to differentiate its format(E.g. {`mod1.relay`, `mod2.relay`, ...}). Similarly, for 


Just for clarification, does it mean that src/relay.txt won't exist anymore in this new version and will be replaced by mod1.relay, modN.delay, ... but nothing changes for the current (old) version (i.e. relay.txt will still exist)? Either way maybe it's worth mentioning what changes/doesn't change regarding src/relay.txt.

By old version if you mean MLF version 6, yes we are not changing that. However, for the new version we are changing relay.txt to MOD_NAME.relay

gromero · 2022-06-03T00:48:13Z

+    ```javascript
+    {
+      'modules': {
+        'mod1': {


Could you please clarify which structures will be moved inside 'modN': {} level in this new version? I'm wondering about memory, functions, and operator_functions ...

added more details to the RFC. Everything would be the same as before except version which will remain at top level.

gromero · 2022-06-03T00:53:33Z

+
+The drawback here is that we are changing generated metadata and file structure in the MLF file which means external 
+tools which are dependent to this need to update their tool. Hopefully this RFC makes it clear on what steps they need
+to take.


Since we'are updating the "version" field I think any dependency/tool can adapt to the new MLF version smoothly.

gromero · 2022-06-03T00:54:38Z

+# **Rationale and alternatives**
+An alternative way to implement this feature is to break down each Relay build module to a subdirectory and keep the
+previous format inside each sub Relay build module. Using the example of `mod1` and `mod2`, in this approach we have
+an MLF file format with structure bellow if we extract:


gromero · 2022-06-03T00:57:16Z

+    mod.relay
+```
+
+One of the benefits of this approach is that create a more readable file structure which is modularized for


nit. s/is that create/is that is creates/?

should be is that is creates I think. I changed it to that.

gromero · 2022-06-03T01:00:57Z

+```
+
+One of the benefits of this approach is that create a more readable file structure which is modularized for
+each Relay build module. However, the downside is that this approach will result in more modifications in project


Really can't find any issue with proposed dir. structure. Also to me it isn't such a great benefit for readability breaking down the modules in separate subdirs tbh.

I agree, I think it will add more complexity to the build process.

mehrdadh

@gromero thanks for the review! I addressed your comments. PTAL

mehrdadh · 2022-06-03T16:26:42Z

+[guide-level-explanation]: #guide-level-explanation
+
+Users call `export_model_library_format` function with multiple Relay builds (output from `tvm.relay.build`) or
+single a Relay build module. We change the `export_model_library_format` API to support multiple Relay Build, however 


mehrdadh · 2022-06-03T16:27:36Z

+it is backward compatible and, we handle required changes internally.
+
+Since we are including multiple modules in a single MLF file, we need to make some changes. Let's assume we
+are want to export MLF file for two Relay build modules called `mod1` and `mod2`.


mehrdadh · 2022-06-03T16:34:04Z

+    ```javascript
+    {
+      'modules': {
+        'mod1': {


added more details to the RFC. Everything would be the same as before except version which will remain at top level.

mehrdadh · 2022-06-03T16:40:13Z

+    }
+    ```
+  2. There are multiple Relay text file generated per each Relay build. We propose to structure these files by using
+     module name and `.relay` extension to differentiate its format(E.g. {`mod1.relay`, `mod2.relay`, ...}). Similarly, for 


By old version if you mean MLF version 6, yes we are not changing that. However, for the new version we are changing relay.txt to MOD_NAME.relay

mehrdadh · 2022-06-03T16:42:05Z

+# **Rationale and alternatives**
+An alternative way to implement this feature is to break down each Relay build module to a subdirectory and keep the
+previous format inside each sub Relay build module. Using the example of `mod1` and `mod2`, in this approach we have
+an MLF file format with structure bellow if we extract:


mehrdadh · 2022-06-03T16:43:03Z

+    mod.relay
+```
+
+One of the benefits of this approach is that create a more readable file structure which is modularized for


should be is that is creates I think. I changed it to that.

mehrdadh · 2022-06-03T16:44:15Z

+```
+
+One of the benefits of this approach is that create a more readable file structure which is modularized for
+each Relay build module. However, the downside is that this approach will result in more modifications in project


I agree, I think it will add more complexity to the build process.

gromero

@mehrdadh Thanks for expanding the example for the new structure. LGTM.

mehrdadh force-pushed the mlf/multiple_modules branch from ef7a760 to 90e829f Compare May 31, 2022 16:25

MLF with Multiple Module RFC

d492d22

mehrdadh force-pushed the mlf/multiple_modules branch from 90e829f to d492d22 Compare June 1, 2022 14:57

gromero reviewed Jun 3, 2022

View reviewed changes

gromero self-assigned this Jun 3, 2022

address @gromero comments

40512a9

mehrdadh commented Jun 3, 2022

View reviewed changes

mehrdadh requested a review from gromero June 3, 2022 17:56

gromero approved these changes Jun 7, 2022

View reviewed changes

gromero merged commit f47c6ad into apache:main Jun 7, 2022

mehrdadh deleted the mlf/multiple_modules branch June 7, 2022 19:11

mehrdadh mentioned this pull request Jun 7, 2022

[MLF] Add support for multiple modules in Model Library Format apache/tvm#11464

Merged

Conversation

mehrdadh commented May 31, 2022

Uh oh!

areusch commented May 31, 2022

Uh oh!

driazati commented Jun 1, 2022

Uh oh!

gromero 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gromero left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants