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 2021/05/21 08:57:25 UTC
[GitHub] [tvm] masahi opened a new pull request #8105: [Relay] Support dynamic inputs in gather_nd and scatter_nd
masahi opened a new pull request #8105:
URL: https://github.com/apache/tvm/pull/8105
--
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] [tvm] masahi merged pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #8105:
URL: https://github.com/apache/tvm/pull/8105
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641009417
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1087,6 +1087,10 @@ def gather_nd(data, indices, batch_dims=0):
batch_dims : int
The number of batch dimensions.
+ num_indices_per_tuple : int
+ The size of an indexing tuple, which is a fixed value and the same as indices.shape[0]
+ Only needed when other dimensions of indices are dynamic.
Review comment:
It is not significant. Having a default value at call sites means this value is not explicitly set, which is an error for dynamic workload. The negative value is only for catching an error at https://github.com/apache/tvm/blob/55a4430f507d801b7441e60d00f06c8161aea30a/python/tvm/relay/op/_transform.py#L1106.
This value is not optional either, since this is required for dynamic workloads.
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641011555
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
Yeah I spent sometime thinking about the best name. I like `index_rank` but the confusion could arise, because it is not clear if this is the rank of indices tensor (the second argument to this op) or the rank of indexing tuple (which is the case). In the ONNX doc https://github.com/onnx/onnx/blob/master/docs/Operators.md#GatherND, the former is `q` and the latter one is `indices_shape[-1]`. I'm looking for a concise and clear word to describe the latter rank. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641012378
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
Also I'd like to think that this attributes was added to workaround the current Relay limitation I described, ideally it should go away when we have a better representation of runtime shape in relay...
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641011555
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
Yeah I spent sometime thinking about the best name. I like `index_rank` but the confusion could arise, because it is not clear if this is the rank of indices tensor (the second argument to this op) or the rank of indexing tuple (which is what I want here). In the ONNX doc https://github.com/onnx/onnx/blob/master/docs/Operators.md#GatherND, the former is `q` and the latter one is `indices_shape[-1]`. I'm looking for a concise and clear word to describe the latter rank. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641094140
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
yeah thats fair, I personally think `index_rank` makes a little more sense but am fine either way.
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641012378
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
Also I'd like to think that this attributes was added to workaround the current Relay limitation I described above, ideally it should go away when we have a better representation of runtime shape in relay...
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641254814
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1087,6 +1087,10 @@ def gather_nd(data, indices, batch_dims=0):
batch_dims : int
The number of batch dimensions.
+ num_indices_per_tuple : int
+ The size of an indexing tuple, which is a fixed value and the same as indices.shape[0]
+ Only needed when other dimensions of indices are dynamic.
Review comment:
Made it Optional with default None.
--
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] [tvm] jwfromm commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641093560
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1087,6 +1087,10 @@ def gather_nd(data, indices, batch_dims=0):
batch_dims : int
The number of batch dimensions.
+ num_indices_per_tuple : int
+ The size of an indexing tuple, which is a fixed value and the same as indices.shape[0]
+ Only needed when other dimensions of indices are dynamic.
Review comment:
Would it be inconvenient to make the default value `None` (indicating that it won't be used / the input must be static) instead of `-1`? That would be more consistent with the behavior and less confusing.
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641253962
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
Change to `index_rank`.
--
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] [tvm] jwfromm commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641082435
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1087,6 +1087,10 @@ def gather_nd(data, indices, batch_dims=0):
batch_dims : int
The number of batch dimensions.
+ num_indices_per_tuple : int
+ The size of an indexing tuple, which is a fixed value and the same as indices.shape[0]
+ Only needed when other dimensions of indices are dynamic.
Review comment:
Would it be inconvenient to use `None` instead of `-1`? That would be more consistent with the behavior in my opinion.
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641175130
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1087,6 +1087,10 @@ def gather_nd(data, indices, batch_dims=0):
batch_dims : int
The number of batch dimensions.
+ num_indices_per_tuple : int
+ The size of an indexing tuple, which is a fixed value and the same as indices.shape[0]
+ Only needed when other dimensions of indices are dynamic.
Review comment:
Ok I can make it an `tvm::Optional<Integer>`.
--
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] [tvm] masahi commented on pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#issuecomment-848656953
pinging for reviews.
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641011555
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
Yeah I spent sometime thinking about the best name. I like `index_rank` but the confusion could arise, because it is not clear if this is the rank of indices tensor (the second argument to this op) or the rank of indexing tuple (which is what I want here). In the ONNX doc https://github.com/onnx/onnx/blob/master/docs/Operators.md#GatherND, the former is `q` and the latter one is `indices_shape[-1]`. I'm looking for a concise and clear word to describe the latter rank. `num_indices_per_tuple` is verbose but precise. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641093560
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1087,6 +1087,10 @@ def gather_nd(data, indices, batch_dims=0):
batch_dims : int
The number of batch dimensions.
+ num_indices_per_tuple : int
+ The size of an indexing tuple, which is a fixed value and the same as indices.shape[0]
+ Only needed when other dimensions of indices are dynamic.
Review comment:
Would it be inconvenient to make the default value `None` instead of `-1`? That would be more consistent with the behavior and less confusing.
--
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] [tvm] masahi commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r641253962
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
Changed to `index_rank`.
--
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] [tvm] jwfromm commented on a change in pull request #8105: [Relay] Support dynamic indices size in gather_nd and scatter_nd
Posted by GitBox <gi...@apache.org>.
jwfromm commented on a change in pull request #8105:
URL: https://github.com/apache/tvm/pull/8105#discussion_r640999354
##########
File path: python/tvm/relay/op/transform.py
##########
@@ -1087,6 +1087,10 @@ def gather_nd(data, indices, batch_dims=0):
batch_dims : int
The number of batch dimensions.
+ num_indices_per_tuple : int
+ The size of an indexing tuple, which is a fixed value and the same as indices.shape[0]
+ Only needed when other dimensions of indices are dynamic.
Review comment:
does the default value of -1 have some significance or does it just mean its not used by default?
##########
File path: include/tvm/relay/attrs/transform.h
##########
@@ -146,11 +146,18 @@ struct GatherAttrs : public tvm::AttrsNode<GatherAttrs> {
struct GatherNDAttrs : public tvm::AttrsNode<GatherNDAttrs> {
Integer batch_dims;
+ Integer num_indices_per_tuple;
TVM_DECLARE_ATTRS(GatherAttrs, "relay.attrs.GatherNDAttrs") {
TVM_ATTR_FIELD(batch_dims).set_default(Integer(0)).describe("The number of batch dimensions.");
+ TVM_ATTR_FIELD(num_indices_per_tuple)
Review comment:
I think this name is a little difficult to parse, would it make sense to instead call it something like `num_dims_per_index` or `index_rank`?
--
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