Enhancements for MXTensor for custom operators #17204

samskalicky · 2020-01-02T22:02:19Z

Description

Enhancements to MXTensor for custom operators. Adds the following features:

version IDs to uniquely identify when a tensor has been modified or is unchanged between calls to operator (can save on data transfer to accelerator if tensor is unchanged)
function to compare MXTensors like we have for MXNet NDarray isSame API

Uprevs MX_LIBRARY_VERSION to 2 since MXTensor struct is changing

Removes opCallBkwd_t since we use opCallFCompute for both forward/backward functions (was dead code)

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

…to mxtensor

samskalicky · 2020-01-03T16:17:13Z

@mxnet-label-bot add [pr-awaiting-review]

samskalicky · 2020-01-03T16:17:59Z

@rondogency @mseth10 @wkcn @junrushao1994 for review

rondogency · 2020-01-03T20:03:01Z

include/mxnet/lib_api.h

+ size_t ID)
+ : data_ptr(data_ptr), shape(shape), dtype(dtype), version(ID) {}
+
+ void update(void *dptr, MXDType type, size_t ver) {


do we really need this function? it doesn't have any checks, only copy pointers. I think we can copy them line by line in lib_api.h and keep MXTensor as simple as possible

per our discussion, lets move the the for loop to copy shape and call to setDLTensor inside this function. Change name to "setTensor"

rondogency · 2020-01-03T20:05:23Z

include/mxnet/lib_api.h

- : data_ptr(data_ptr), shape(shape), dtype(dtype) {}
+ MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype,
+ size_t ID)
+ : data_ptr(data_ptr), shape(shape), dtype(dtype), version(ID) {}


it will be better to unify the naming across all places, like using verID in lib_api.h and here

rondogency · 2020-01-03T20:06:38Z

include/mxnet/lib_api.h

@@ -277,6 +283,14 @@ struct MXTensor {
 return size;
 }

+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {


should we override operator==? since we won't support C anyway

I think operator== is confusing. For a tensor object, == usually means value comparison.
In the future, we may add other operators !=, <, >, etc.
It may be better and more consistent with MXNet NDArray API to use ‘isSame’.

comparing object is how c++ is doing for vector and usually for struct, and in NDArray we don't have operators !=, <, > either, so I don't think it is going to be confusing

I think I agree with @wkcn here, == should compare the values of the tensor not the "state" of the tensor (data_ptr, versionID, etc)

@mseth10 @eric-haibin-lin @haojin2 what do you guys think?

For C++ vector container, operator== compares the values.

#include <iostream> #include <vector> using namespace std; int main() { vector<int> a{1,2,3}; vector<int> b{1,2,3}; vector<int> c{1,2,4}; cout << (a == b) << endl; // 1 cout << (a == c) << endl; // 0 cout << (a.data() == b.data()) << endl; // 0 return 0; }

Although （a==b) is 1, a.data() is not equal to b.data().

…to mxtensor

wkcn · 2020-01-04T01:06:59Z

include/mxnet/lib_api.h

@@ -277,6 +289,14 @@ struct MXTensor {
 return size;
 }

+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {


It will be more consistent with MXNet NDArray API to use ‘IsSame’, since operator== is confusing between same object and same tensor content.

Sorry that I did not read the previous review.

writing a response together with @rondogency:
For clarity, here were checking the following items: dtype, version ID, shape, and the pointer address of the data in memory. If two tensors have the same data pointer then they will have the same data values also. So here the two tensors will have the same content and will have the same tensor attributes (shape, type, version ID) also.

I dont think we need to be consistent with NDarray API here, since users writing custom ops wont necessarily be familiar with NDarray API. And here we're trying to make the right API for this use-case (custom operators). Our comparison API also checks the version number, and the NDarray API does not. So we're already diverging from being consistent with NDArray API.

For the record, here is the similar NDarray API:
https://github.com/apache/incubator-mxnet/blob/55e222b8c97f99193f832f785cf210f876632add/include/mxnet/ndarray.h#L212-L217

Thank @rondogency and you for the explanation in detail!

If two tensors have the same data pointer then they will have the same data values also.
It is right. However, if two tensors have the same data values, they may not have the same data pointer. I did a simple test on C++ vector.

#include <iostream> #include <vector> using namespace std; int main() { vector<int> a{1,2,3}; vector<int> b{1,2,3}; cout << (a == b) << endl; // 1 cout << (a.data() == b.data()) << endl; // 0 return 0; }

we don't need to be consistent with NDarray API here.
Agree : )

Thanks @wkcn, if @rondogency agrees ill change it back to 'isSame'

I agree with @wkcn on this. isSame is less confusing.

ok I agree with changing it back to isSame, thanks for the inspection on c++ vector!

…to mxtensor

wkcn

LGTM. Thank you!

…to mxtensor

samskalicky · 2020-01-07T21:13:46Z

@mxnet-label-bot update [pr-awaiting-merge]

samskalicky · 2020-01-07T21:14:07Z

@wkcn we're ready to merge!

mseth10

LGTM

samskalicky · 2020-01-08T06:42:23Z

@wkcn will you merge this?

wkcn · 2020-01-08T10:14:02Z

Merged : ) Thank you!

initial commit

0c6a300

samskalicky requested review from anirudh2290 and eric-haibin-lin as code owners January 2, 2020 22:02

samskalicky added 5 commits January 2, 2020 22:49

added version initialization in c_api

bfe1195

added isSame helper function

fa80372

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

9cb3bd0

…to mxtensor

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

85d2879

…to mxtensor

fixed whitespace and isSame function

f8b3035

samskalicky changed the title ~~[WIP] enhancements for MXTensor for custom operators~~ Enhancements for MXTensor for custom operators Jan 3, 2020

lanking520 added the pr-awaiting-review PR is waiting for code review label Jan 3, 2020

rondogency reviewed Jan 3, 2020

View reviewed changes

samskalicky added 4 commits January 3, 2020 20:21

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

2eac368

…to mxtensor

changed version to verID

15679f6

changed update to setTensor

bbf8e63

changed isSame to operator==

989b394

wkcn reviewed Jan 4, 2020

View reviewed changes

samskalicky added 3 commits January 6, 2020 21:09

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

8d4ea67

…to mxtensor

changed operator== back to isSame

789302a

retrigger CI

e6cd9ac

wkcn approved these changes Jan 7, 2020

View reviewed changes

samskalicky added 3 commits January 7, 2020 04:23

Merge branch 'master' of https://github.com/apache/incubator-mxnet in…

3f001aa

…to mxtensor

retrigger CI

c310281

retrigger ci

d139686

lanking520 added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Jan 7, 2020

rondogency approved these changes Jan 7, 2020

View reviewed changes

mseth10 approved these changes Jan 7, 2020

View reviewed changes

wkcn merged commit c1f6d64 into apache:master Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancements for MXTensor for custom operators #17204

Enhancements for MXTensor for custom operators #17204

samskalicky commented Jan 2, 2020 •

edited

Loading

samskalicky commented Jan 3, 2020

samskalicky commented Jan 3, 2020 •

edited

Loading

rondogency Jan 3, 2020

samskalicky Jan 3, 2020

rondogency Jan 3, 2020

samskalicky Jan 3, 2020

rondogency Jan 3, 2020

samskalicky Jan 3, 2020

wkcn Jan 4, 2020

rondogency Jan 4, 2020

samskalicky Jan 4, 2020

wkcn Jan 4, 2020 •

edited

Loading

wkcn Jan 4, 2020

wkcn Jan 4, 2020

samskalicky Jan 4, 2020

wkcn Jan 5, 2020 •

edited

Loading

samskalicky Jan 5, 2020

mseth10 Jan 6, 2020

rondogency Jan 6, 2020

wkcn left a comment

samskalicky commented Jan 7, 2020

samskalicky commented Jan 7, 2020

mseth10 left a comment

samskalicky commented Jan 8, 2020

wkcn commented Jan 8, 2020

Enhancements for MXTensor for custom operators #17204

Enhancements for MXTensor for custom operators #17204

Conversation

samskalicky commented Jan 2, 2020 • edited Loading

Description

Checklist

Essentials

samskalicky commented Jan 3, 2020

samskalicky commented Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkcn Jan 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkcn Jan 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkcn left a comment

Choose a reason for hiding this comment

samskalicky commented Jan 7, 2020

samskalicky commented Jan 7, 2020

mseth10 left a comment

Choose a reason for hiding this comment

samskalicky commented Jan 8, 2020

wkcn commented Jan 8, 2020

samskalicky commented Jan 2, 2020 •

edited

Loading

samskalicky commented Jan 3, 2020 •

edited

Loading

wkcn Jan 4, 2020 •

edited

Loading

wkcn Jan 5, 2020 •

edited

Loading