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 2020/07/08 22:00:38 UTC
[GitHub] [incubator-tvm] jwfromm opened a new pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
jwfromm opened a new pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020
This PR adds GRU parsing to the onnx frontend. Currently this is done by unrolling the recurrence in a similar way to how we handle LSTMs. Since there's quite a bit of shared code, I've generalized the LSTM converter to an RNN converter and made LSTM and GRU subclasses. For testing, I again replaced the LSTM test function with a more general test_rnn function that supports both LSTM and GRU and should be easily expandable to other RNN functions should we ever want to add any.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#issuecomment-655838958
@siju-samuel please help review this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi commented on pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#issuecomment-657215203
Thanks @jwfromm @areusch @anijain2305
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jwfromm commented on a change in pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#discussion_r452380976
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -1530,6 +1532,14 @@ def _activation_needs_beta(cls, activation):
@classmethod
def _impl_v7(cls, inputs, attr, params):
+ if cls.name == 'LSTM':
Review comment:
you're right thats a nicer organization. I've updated the structure accordingly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] anijain2305 commented on pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
anijain2305 commented on pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#issuecomment-655830391
This might be somewhat relevant - https://discuss.tvm.ai/t/onnx-lstm-op-conversion/7238
Just pointing it out here. I think the GRU might also have the same problems that I encounter with LSTM.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jwfromm commented on pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
jwfromm commented on pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#issuecomment-656258865
I've added a small change to how arguments are parsed in RNNs based on the discussion here: https://discuss.tvm.ai/t/onnx-lstm-op-conversion/7238/5. Our previous implementation assumed that the position of optional arguments could not be known without their name, however, this is not true assuming the nodes are constructing properly. To avoid name based problems, RNNs now use indexing to get inputs. I've updated the `onnx_input` structure to allow indexing outside of the input bounds but return `None` in such cases, indicating that an input was not provided.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jwfromm commented on pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
jwfromm commented on pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#issuecomment-655780293
@masahi, @areusch can you guys take a look at this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] areusch commented on a change in pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#discussion_r451934374
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -1530,6 +1532,14 @@ def _activation_needs_beta(cls, activation):
@classmethod
def _impl_v7(cls, inputs, attr, params):
+ if cls.name == 'LSTM':
Review comment:
here I think it'd be cleaner to just implement the `@classmethod` in each subclass, no?
##########
File path: python/tvm/relay/frontend/onnx.py
##########
@@ -1530,6 +1532,14 @@ def _activation_needs_beta(cls, activation):
@classmethod
def _impl_v7(cls, inputs, attr, params):
+ if cls.name == 'LSTM':
+ return cls._lstm(inputs, attr, params)
+ if cls.name == 'GRU':
+ return cls._gru(inputs, attr, params)
+ raise NotImplementedError("%s RNNs are not yet supported." % cls.name)
+
+ @classmethod
+ def _lstm(cls, inputs, attr, params):
Review comment:
any reason not to move this to LSTM subclass?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jwfromm commented on pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
jwfromm commented on pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#issuecomment-655851725
I've converted this to a draft as I'll try to incorporate the points made by @anijain2305 in the linked discuss post.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jwfromm edited a comment on pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
jwfromm edited a comment on pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020#issuecomment-655780293
@masahi, @areusch, @soiferj can you guys take a look at this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] masahi merged pull request #6020: [Relay][Frontend][Onnx] GRU Layer Support
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #6020:
URL: https://github.com/apache/incubator-tvm/pull/6020
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org