You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/02/15 00:57:37 UTC
[GitHub] [incubator-mxnet] connorgoggins opened a new pull request #17599:
Fixed Embedding op for LT input
connorgoggins opened a new pull request #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599
## Description ##
The Embedding op was previously breaking on large tensor (dimension >= 2^32) data. With the following input:
```
nd.Embedding(data=nd.random_normal(shape=(2**32,1)), weight=nd.random_normal(shape=(2**32,1)), input_dim=2**32, output_dim=1)
```
the following error was thrown:
```
mxnet.base.MXNetError: MXNetError: Invalid Parameter format for input_dim expect int but value='4294967296', in operator Embedding(name="", output_dim="1", input_dim="4294967296")
```
To fix this issue, I modified `indexing_op.h` to switch from storing input_dim as an `int` to storing it as an `index_t`. After implementing my fix and rebuilding, the previous input command displayed the correct output:
```
[[[-0.5190417]]
[[-1.4388928]]
[[ 1.1367434]]
...
[[-1.4388928]]
[[-1.4388928]]
[[-0.5190417]]]
<NDArray 4294967296x1x1 @cpu(0)>
```
## Checklist ##
### Essentials ###
Please feel free to remove inapplicable items for your PR.
- [x] Changes are complete (i.e. I finished coding on this PR)
- [x] All changes have test coverage
- [x] Code is well-documented
- [x] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
### Changes ###
- M src/operator/tensor/indexing_op.h
## Comments ##
Tested on r5dn.24xl-ubuntu 16.04 and p2.16xl-ubuntu 16.04 with
1. Individual op run
2. Full OpPerf run
## Results ##
[Single operator test - Embedding op (GPU)](https://gist.github.com/connorgoggins/2ead27ac3b142e458ddba43eb4093bf7)
[Single operator test - Embedding op (CPU)](https://gist.github.com/connorgoggins/cdd5659abd5d2152a29f44a1f1ee03c7)
[Full OpPerf test (GPU)](https://gist.github.com/connorgoggins/09131cdbbdcbba1dc39a93099dccaad4)
[Full OpPerf test (CPU)](https://gist.github.com/connorgoggins/3fc3c7a40dae8b5cea7f82d9ca4b1a72)
@apeforest @access2rohit
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] access2rohit commented on issue #17599: Fixed
Embedding op for LT input
Posted by GitBox <gi...@apache.org>.
access2rohit commented on issue #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-586540131
@connorgoggins can you paste output of a case where sparseEmbedding params are used ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] apeforest commented on issue #17599: Fixed
Embedding op for LT input
Posted by GitBox <gi...@apache.org>.
apeforest commented on issue #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-589365174
LGTM. Have you verified that your nightly test can run successfully in your local machine?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] connorgoggins commented on issue #17599: Fixed
Embedding op for LT input
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on issue #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-586543671
@access2rohit great question. Here is an example of when sparseEmbedding params are used without my fix, and the resulting error (same error as with Embedding):
```
>>> mx.contrib.nd.SparseEmbedding(data=mx.nd.random_normal(shape=(2**32,1)), weight=mx.nd.random_normal(shape=(2**32, 1)), input_dim=2**32, output_dim=1)
mxnet.base.MXNetError: MXNetError: Invalid Parameter format for input_dim expect int but value='4294967296', in operator _contrib_SparseEmbedding(name="", output_dim="1", input_dim="4294967296")
```
With my fix, the call above passes without any issues - output below:
```
[[[-0.5190417]]
[[-1.4388928]]
[[ 1.1367434]]
...
[[-1.4388928]]
[[-1.4388928]]
[[-0.5190417]]]
<NDArray 4294967296x1x1 @cpu(0)>
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] connorgoggins commented on issue #17599: [Large
Tensor] Fixed Embedding op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on issue #17599: [Large Tensor] Fixed Embedding op
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-589386576
@apeforest thanks! The individual nightly tests I wrote for each op run successfully on my local machine. Running the full nightly test suite for each op now.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] connorgoggins edited a comment on issue #17599:
[Large Tensor] Fixed Embedding op
Posted by GitBox <gi...@apache.org>.
connorgoggins edited a comment on issue #17599: [Large Tensor] Fixed Embedding op
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-589456926
@apeforest update: full suite of nightly tests passed on r5dn.24xl instances for every one of my PRs with additional tests added.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] apeforest commented on a change in pull request
#17599: Fixed Embedding op for LT input
Posted by GitBox <gi...@apache.org>.
apeforest commented on a change in pull request #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599#discussion_r379766442
##########
File path: src/operator/tensor/indexing_op.h
##########
@@ -66,7 +66,7 @@ enum QuantizedEmbeddingOpResource {kTempSpace};
struct SparseEmbeddingParam: public dmlc::Parameter<SparseEmbeddingParam> {
- int input_dim;
+ index_t input_dim;
int output_dim;
Review comment:
Would there be a case where output_dim also exceeds 2^32?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] connorgoggins commented on a change in pull
request #17599: Fixed Embedding op for LT input
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on a change in pull request #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599#discussion_r380334678
##########
File path: src/operator/tensor/indexing_op.h
##########
@@ -66,7 +66,7 @@ enum QuantizedEmbeddingOpResource {kTempSpace};
struct SparseEmbeddingParam: public dmlc::Parameter<SparseEmbeddingParam> {
- int input_dim;
+ index_t input_dim;
int output_dim;
Review comment:
@apeforest excellent point, there would be. For example, the following call throws an error:
```
>>> mx.nd.Embedding(data=mx.nd.random_normal(shape=(1,)), weight=mx.nd.random_normal(shape=(1,2**32)), input_dim=1, output_dim=2**32)
mxnet.base.MXNetError: MXNetError: Invalid Parameter format for output_dim expect int but value='4294967296', in operator Embedding(name="", output_dim="4294967296", input_dim="1")
```
With my latest update (changing the dtype of `output_dim` to `index_t`), here is the result:
```
>>> mx.nd.Embedding(data=mx.nd.random_normal(shape=(1,)), weight=mx.nd.random_normal(shape=(1,2**32)), input_dim=1, output_dim=2**32)
[[ 1.6323917 -0.33354783 -1.7378405 ... -0.36648417 0.6363522
2.367109 ]]
<NDArray 1x4294967296 @cpu(0)>
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] apeforest merged pull request #17599: [Large
Tensor] Fixed Embedding op
Posted by GitBox <gi...@apache.org>.
apeforest merged pull request #17599: [Large Tensor] Fixed Embedding op
URL: https://github.com/apache/incubator-mxnet/pull/17599
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] apeforest commented on issue #17599: [Large
Tensor] Fixed Embedding op
Posted by GitBox <gi...@apache.org>.
apeforest commented on issue #17599: [Large Tensor] Fixed Embedding op
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-589894213
@connorgoggins I retriggered CI tests
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] access2rohit edited a comment on issue #17599:
Fixed Embedding op for LT input
Posted by GitBox <gi...@apache.org>.
access2rohit edited a comment on issue #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-586540131
@connorgoggins can you paste output of a case where sparseEmbedding params are used ? Are both used in the same example use case presented here ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] connorgoggins commented on issue #17599: Fixed
Embedding op for LT input
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on issue #17599: Fixed Embedding op for LT input
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-586535371
@mxnet-label-bot add [pr-awaiting-review]
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] connorgoggins commented on issue #17599: [Large
Tensor] Fixed Embedding op
Posted by GitBox <gi...@apache.org>.
connorgoggins commented on issue #17599: [Large Tensor] Fixed Embedding op
URL: https://github.com/apache/incubator-mxnet/pull/17599#issuecomment-589456926
@apeforest update: full suite of nightly tests passed for every one of my PRs with additional tests added.
----------------------------------------------------------------
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
With regards,
Apache Git Services