You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/04/08 17:35:31 UTC
[GitHub] [tvm] PhilippvK opened a new pull request, #10941: TVMC: Add new text/relay frontend
PhilippvK opened a new pull request, #10941:
URL: https://github.com/apache/tvm/pull/10941
This feature enables passing a textural representation of a relay module to the tvmc command line.
Example: `tvmc compile relay.txt --target c --runtime=crt --executor=aot --executor-aot-unpacked-api=1 --pass-config tir.disable_vectorize=1 -f mlf`
Currently it is not possible to supply parameters as it is mainly intended to be used for testing certain relay functions or operators. In the future (with minor changes to the tvmc frontend api) params could be passed via an additional i.e. `params.bin` file
This commit also adds minimal unit testing of the added feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1099314151
This failed in CI due to what is being fixed in https://github.com/apache/tvm/pull/11007. Once that gets merged, I think we can re-trigger CI here to get this merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a diff in pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on code in PR #10941:
URL: https://github.com/apache/tvm/pull/10941#discussion_r847334491
##########
python/tvm/driver/tvmc/frontends.py:
##########
@@ -284,13 +285,48 @@ def load(self, path, shape_dict=None, **kwargs):
return relay.frontend.from_paddle(prog, shape_dict=shape_dict, **kwargs)
+class TextFrontend(Frontend):
Review Comment:
I think calling it "text" might sound too ambiguous in TVM context. I suggest we use "Relay" terminology to make it more specific.
```suggestion
class RelayFrontend(Frontend):
```
##########
python/tvm/driver/tvmc/frontends.py:
##########
@@ -284,13 +285,48 @@ def load(self, path, shape_dict=None, **kwargs):
return relay.frontend.from_paddle(prog, shape_dict=shape_dict, **kwargs)
+class TextFrontend(Frontend):
+ """Text (Relay) frontend for TVMC"""
+
+ @staticmethod
+ def name():
+ return "text"
+
+ @staticmethod
+ def suffixes():
+ return ["relay", "txt", "text"]
Review Comment:
Same reason as previously stated, I suggest only referring to it as the "Relay" frontend.
```suggestion
return ["relay"]
```
##########
python/tvm/driver/tvmc/frontends.py:
##########
@@ -284,13 +285,48 @@ def load(self, path, shape_dict=None, **kwargs):
return relay.frontend.from_paddle(prog, shape_dict=shape_dict, **kwargs)
+class TextFrontend(Frontend):
+ """Text (Relay) frontend for TVMC"""
+
+ @staticmethod
+ def name():
+ return "text"
+
+ @staticmethod
+ def suffixes():
+ return ["relay", "txt", "text"]
+
+ def load(self, path, shape_dict=None, **kwargs):
+ with open(path, "r", encoding="utf-8") as relay_text:
+ text = relay_text.read()
+ if shape_dict is not None:
+ logger.warning("Supplied shape_dict argument ignored for text frontend")
+ ir_mod = parser.fromtext(text)
Review Comment:
Just to clarify my point, I think this `parser.frontext()` call is appropriate because the full name of the module makes it implicit that it refers to Relay, given it is `relay.parser.frontext()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1095230572
@leandron Thank you for your feedback. I already considered using the name `relay` instead. I just though `RelayFrontend` might easy be confused with the implementations in `tvm.relay.frontend.*`.
What do you think about adding a way to provide custom (non-constant) data for the weights? This would involve small changes to the TVMC command line as more that one input file (e.g. `mod.relay` and `mod.params`) would need to be allowed in this special case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1100723308
I just ran into a few issues with the implementation when providing more complex models. I will have a look at but, but please do not merge this in its current state. Sorry for the inconvenience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron merged pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
leandron merged PR #10941:
URL: https://github.com/apache/tvm/pull/10941
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1103944215
@areusch
These are the two issues I ran into:
1. As the frontend is not able to distinguish between real model inputs and parameters, I propagated all of the relay inputs with constant values. This way the model inputs are removed due to Constant Folding which makes the resulting model less complex. **Current Workaround:** Use `--input-shapes "x:[1,2,3]"` etc. (if available) to get the names of the actual inputs and skip them when generating the parameter values.
2. I wanted to experiment with some models which make use of `meta[relay.Constant]` for internal constant vectors. These relay models can only be parsed if the Relay model was exported via `ir_mod.astext()` (which uses `show_meta_data=True` by default). The `relay.txt` format used in the MLF archive is using `str(ir_mod)` does neither include the `#[metadata]` section nor the `#[version ...]` header. I added a checker to detect if one this information is missing.
@areusch Feel free to have another look at the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1093124506
@masahi @gromero @leandron @jwfromm Please let me know if this is a meaningful addition to the TVMC command line interface. I am sure we could also agree on a way to provide actual values for the module parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1095243234
> @leandron Thank you for your feedback. I already considered using the name `relay` instead. I just though `RelayFrontend` might easy be confused with the implementations in `tvm.relay.frontend.*`.
>
I think the package makes it clear as a distinction, so I'd be in favour or renaming `TextFrontend` to `RelayFrontend`, given it is located at `tvm.driver.tvmc.frontend.RelayFrontend`, as a module.
> What do you think about adding a way to provide custom (non-constant) data for the weights? This would involve small changes to the TVMC command line as more that one input file (e.g. `mod.relay` and `mod.params`) would need to be allowed in this special case.
Not sure, it feels like we should think about a file format that incorporate all these for Relay, so that we bundle all that is needed in a single file to be used and reused in TVM, rather making it a special case in tvmc? cc @areusch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #10941:
URL: https://github.com/apache/tvm/pull/10941#discussion_r847595319
##########
python/tvm/driver/tvmc/frontends.py:
##########
@@ -284,13 +285,48 @@ def load(self, path, shape_dict=None, **kwargs):
return relay.frontend.from_paddle(prog, shape_dict=shape_dict, **kwargs)
+class RelayFrontend(Frontend):
+ """Relay frontend for TVMC"""
+
+ @staticmethod
+ def name():
+ return "relay"
+
+ @staticmethod
+ def suffixes():
+ return ["relay"]
+
+ def load(self, path, shape_dict=None, **kwargs):
+ with open(path, "r", encoding="utf-8") as relay_text:
+ text = relay_text.read()
+ if shape_dict is not None:
+ logger.warning("Supplied shape_dict argument ignored for text frontend")
+ ir_mod = parser.fromtext(text)
+
+ def _gen_params(ir_mod):
+ """Populate the all the params in the mode with ones."""
+ main_func = ir_mod["main"]
+ shape_dict = {p.name_hint: p.checked_type.concrete_shape for p in main_func.params}
+ type_dict = {p.name_hint: p.checked_type.dtype for p in main_func.params}
+ params = {}
+ for name, shape in shape_dict.items():
+ data = np.ones(shape).astype(type_dict[name])
Review Comment:
any thoughts on supplying data in e.g. `.npz`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1099994528
@leandron @areusch CI has successfully passed. Feel free to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
areusch commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1104577966
hm. for (1), it seems like we could do something like:
- add a parameter which you have to pass for Relay models indicating the inputs
- accept params.npz and treat everything not in there as inputs
- a combination of both
for (2), that seems like an oversight on Model Library Format part. it seems like we should make tvmc validate the version numbers in relay models. Perhaps we should change Model Library Format?
i suppose it's also a bit of a burden on users authoring relay to figure out the presently-supported version, and they probably always just mean "the current version." so maybe allowing some implicitness there makes sense.
@leandron what are your thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1096556415
Thanks for your feedback. I renamed all instances of `test` to `relay`.
The recommended approach for bundling the relay format with the parameters/weights wouldn't be solved in this PR, right?
As recommended by @areusch I could add a warning like `Parameters in the module will be initialized with ones.`. Any ideas for a different message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1096581885
> The recommended approach for bundling the relay format with the parameters/weights wouldn't be solved in this PR, right?
You're correct. This would need to be agreed within the TVM community in an RFC.
> As recommended by @areusch I could add a warning like `Parameters in the module will be initialized with ones.`. Any ideas for a different message?
I think this message is OK given it is the Relay frontend and it has limited scope in terms of published models.
However, I would suggest, for the sake of having a better way to provide Relay in future (not only in tvmc, but for TVM in general), to start that discussion above (regarding a Relay format that includes parameters) in the community forum - https://discuss.tvm.apache.org - what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a diff in pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #10941:
URL: https://github.com/apache/tvm/pull/10941#discussion_r847596130
##########
python/tvm/driver/tvmc/frontends.py:
##########
@@ -284,13 +285,48 @@ def load(self, path, shape_dict=None, **kwargs):
return relay.frontend.from_paddle(prog, shape_dict=shape_dict, **kwargs)
+class RelayFrontend(Frontend):
+ """Relay frontend for TVMC"""
+
+ @staticmethod
+ def name():
+ return "relay"
+
+ @staticmethod
+ def suffixes():
+ return ["relay"]
+
+ def load(self, path, shape_dict=None, **kwargs):
+ with open(path, "r", encoding="utf-8") as relay_text:
+ text = relay_text.read()
+ if shape_dict is not None:
+ logger.warning("Supplied shape_dict argument ignored for text frontend")
+ ir_mod = parser.fromtext(text)
+
+ def _gen_params(ir_mod):
+ """Populate the all the params in the mode with ones."""
+ main_func = ir_mod["main"]
+ shape_dict = {p.name_hint: p.checked_type.concrete_shape for p in main_func.params}
+ type_dict = {p.name_hint: p.checked_type.dtype for p in main_func.params}
+ params = {}
+ for name, shape in shape_dict.items():
+ data = np.ones(shape).astype(type_dict[name])
Review Comment:
oh i just read your commit note more clearly. shall we instead add a warning perhaps so it's clear to the user how the interface is expected to work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
areusch commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1103273019
@PhilippvK we could also merge and you could submit some fixes. could you elaborate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a diff in pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on code in PR #10941:
URL: https://github.com/apache/tvm/pull/10941#discussion_r847338062
##########
python/tvm/driver/tvmc/frontends.py:
##########
@@ -284,13 +285,48 @@ def load(self, path, shape_dict=None, **kwargs):
return relay.frontend.from_paddle(prog, shape_dict=shape_dict, **kwargs)
+class TextFrontend(Frontend):
+ """Text (Relay) frontend for TVMC"""
+
+ @staticmethod
+ def name():
+ return "text"
+
+ @staticmethod
+ def suffixes():
+ return ["relay", "txt", "text"]
+
+ def load(self, path, shape_dict=None, **kwargs):
+ with open(path, "r", encoding="utf-8") as relay_text:
+ text = relay_text.read()
+ if shape_dict is not None:
+ logger.warning("Supplied shape_dict argument ignored for text frontend")
+ ir_mod = parser.fromtext(text)
Review Comment:
Just to clarify my point, I think this `parser.fromtext()` call is appropriate because the full name of the module makes it implicit that it refers to Relay, given it is `relay.parser.fromtext()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1097542679
@leandron I totally agree. I will start a thread in the discussion forum in the next few days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] PhilippvK commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
PhilippvK commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1185259036
@areusch
> add a parameter which you have to pass for Relay models indicating the inputs
I currently already use `--input-shapes` to get the input names, ignoring the provided shapes. Adding new flag for only the names would be somehow redundant in my opinion.
> accept params.npz and treat everything not in there as inputs
Would maybe be a good idea for the future, but I think this is out-of scope for this PR.
> for (2), that seems like an oversight on Model Library Format part.
The thing is that exporting the full relay model including metadata can result in very lang files which might not be desirable for the `relay.txt` in the MLF. However I would prefer some consistency here having at least the proper header in there.
> it seems like we should make tvmc validate the version numbers in relay models.
I can look into this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on pull request #10941: TVMC: Add new text/relay frontend
Posted by GitBox <gi...@apache.org>.
leandron commented on PR #10941:
URL: https://github.com/apache/tvm/pull/10941#issuecomment-1189189851
@PhilippvK I'm merging this after a long time. It's a great new feature - sorry it took so long to realise it was pending.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org