Large Index Support for Slice by access2rohit · Pull Request #15593 · apache/mxnet

access2rohit · 2019-07-18T20:52:05Z

Description

Ops Supported: Slice

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
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 http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html

access2rohit · 2019-07-18T22:23:15Z

@mxnet-label-bot add [pr-work-in-progress]

apeforest · 2019-07-18T23:04:37Z

    check_call(_LIB.MXNDArrayCreateEx(
-        c_array_buf(mx_uint, native_array('I', shape)),
-        mx_uint(len(shape)),
+        c_array_buf(mx_int64, native_array('q', shape)),


need to check python version if 'q' is supported

apeforest · 2019-07-18T23:07:42Z

+#if MXNET_USE_INT64_TENSOR_SIZE == 1
+  return MXNDArrayCreateExInt<int64_t>(shape, ndim, dev_type, dev_id, delay_alloc, dtype, out);
+#else
+  return MXNDArrayCreateExInt<int32_t>(shape, ndim, dev_type, dev_id, delay_alloc, dtype, out);


It may not work. You can't just cast a mx_int64 pointer to int32_t pointer.

apeforest · 2019-07-18T23:10:13Z

  API_END();
 }

+int MXNDArrayCreateEx(const mx_int64 *shape,


Also you are directly changing this C API which might break other language bindings that use this C API.

apeforest · 2019-07-18T23:11:49Z

 MXNET_DLL int MXNDArrayGetShapeEx(NDArrayHandle handle,
                                  int *out_dim,
-                                  const int **out_pdata);
+                                  const int64_t **out_pdata);


You are directly modifying C API which may break other language bindings. As we have agreed, it may be safer to create additional 64bit APIs instead.

kshitij12345 · 2019-07-19T12:02:24Z


 /*! \brief manually define unsigned int */
-typedef unsigned int mx_uint;
+typedef int64_t mx_int64;


Just a nit but,
As per the comment
/*! \brief manually define unsigned int */
Line
typedef int64_t mx_int64; should be above it I guess, as it is not unsigned.

kshitij12345 · 2019-07-19T12:05:07Z

 * \return 0 when success, -1 when failure happens
 */
-MXNET_DLL int MXNDArrayCreateEx(const mx_uint *shape,
+MXNET_DLL int MXNDArrayCreateEx(const mx_int64 *shape,


Sorry if this is a stupid question, but why are we replacing mx_uint (unsigned) with mx_int64 (signed)?
I would assume shape is always positive, shouldn't it be mx_uint64?

Two reasons:
(1) According to Google C++ style guide: "Do not use an unsigned type merely to assert that a variable is non-negative." https://google.github.io/styleguide/cppguide.html#Integer_Types

(2) We intentionally changed to int because we may reserve the index -1 for supporting empty tensor.

Oh. Thank You for the explanation.

apeforest · 2019-07-19T17:52:46Z

 }

+int MXNDArrayGetShapeExInt64(NDArrayHandle handle,
+                        int *out_dim,


apeforest · 2019-07-19T21:50:58Z

+
+MXNET_DLL int MXNDArrayGetShapeExInt64(NDArrayHandle handle,
+                                       int *out_dim,
+                                       const int64_t **out_pdata);


use mx_int64 to be consistent?

access2rohit · 2019-07-19T21:59:07Z

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

ChaiBapchya

Minor changes!
Also, c_api_common.h and c_api_symbolic.cc still use int64_t (simple find for int64_t shows there are 22 places it's being used currently in the PR. Wanted clarity on what's the norm? When to use int64_t and when to use mx_int64? Is it even needed?)

access2rohit · 2019-07-22T23:45:52Z

@ChaiBapchya mx_uint is used for maintaining consistency with language bindings. For example python uses mx_int64 = ctype.c_int64 and its counter part in C++ is typedef int64_t mx_int64 this way we can ensure both signature have same data types. one exception to it is int which on python side is mx_int but on C++ its just int

apeforest · 2019-08-08T22:16:40Z

-  MXAPIThreadLocalEntry *ret = MXAPIThreadLocalStore::Get();
-  API_BEGIN();
+template<typename dtype, typename stype, typename itype>
+inline void SymbolInferShapeImpl(const char** keys,


Also renaming this to SymbolInferShape if you are renaming others too.

apeforest · 2019-08-08T22:17:46Z

+                                  const int **aux_shape_ndim,
+                                  const int64_t ***aux_shape_data,
+                                  int *complete) {
+  int succ;


initialize succ here.

access2rohit · 2019-08-08T23:08:01Z

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

apeforest · 2019-08-09T06:03:45Z

 _NDARRAY_ADVANCED_INDEXING = 1

+# Caching whether MXNet was built with INT64 support or not
+_INT64_TENSOR_SIZE_ENABLED = Features().is_enabled('INT64_TENSOR_SIZE')


This is still not efficient as Features() is still called everytime this variable is evaluated. There is no const variable in Python as in C++

apeforest · 2019-08-13T04:02:36Z

+                          const mx_int64 **out_pdata) {
+  MXAPIThreadLocalEntry<int64_t> *ret = MXAPIThreadLocalStore<int64_t>::Get();
+  API_BEGIN();
+  GetShape<mx_int64>(handle, out_pdata, out_dim, ret);


mx_int64 and int64_t are mixed here. Please consolidate them in your next PR.

apeforest

LGTM

access2rohit requested review from anirudh2290, eric-haibin-lin and szha as code owners July 18, 2019 20:52

access2rohit force-pushed the lis_slice branch 2 times, most recently from c5705c8 to a0014ed Compare July 18, 2019 22:11

marcoabreu added the pr-work-in-progress PR is still work in progress label Jul 18, 2019

access2rohit force-pushed the lis_slice branch from a0014ed to ac0366a Compare July 18, 2019 22:24

apeforest reviewed Jul 18, 2019

View reviewed changes

kshitij12345 reviewed Jul 19, 2019

View reviewed changes

access2rohit force-pushed the lis_slice branch 2 times, most recently from 8fcde44 to 2d31323 Compare July 19, 2019 17:38

apeforest reviewed Jul 19, 2019

View reviewed changes

access2rohit force-pushed the lis_slice branch 4 times, most recently from 9cebf08 to fb9540e Compare July 19, 2019 21:43

apeforest reviewed Jul 19, 2019

View reviewed changes

access2rohit force-pushed the lis_slice branch from fb9540e to 72919d7 Compare July 19, 2019 21:53

marcoabreu added the pr-awaiting-review PR is waiting for code review label Jul 19, 2019

access2rohit force-pushed the lis_slice branch from 72919d7 to f6cbcf0 Compare July 19, 2019 22:00

access2rohit changed the title ~~[WIP]Large Index Support~~ Large Index Support for Slice Jul 19, 2019

ChaiBapchya suggested changes Jul 20, 2019

View reviewed changes

Comment thread src/c_api/c_api.cc

Comment thread src/operator/tensor/matrix_op-inl.h

access2rohit force-pushed the lis_slice branch from f6cbcf0 to 11280e8 Compare July 23, 2019 00:06

apeforest reviewed Aug 8, 2019

View reviewed changes

access2rohit force-pushed the lis_slice branch 3 times, most recently from 961de43 to 124a282 Compare August 8, 2019 23:00

marcoabreu added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Aug 8, 2019

apeforest reviewed Aug 9, 2019

View reviewed changes

access2rohit force-pushed the lis_slice branch 6 times, most recently from e5e382a to 458b1ae Compare August 12, 2019 16:19

Rohit Kumar Srivastava added 7 commits August 12, 2019 23:40

Adding Large Index Support for slice operator

fd1b45f

adding changes to fix py2 related error in CI/CD

3aaea19

refactoring c_api, c_symbolic_api, c_api_common

9df3bf5

rearrange system call and slower Feature() call

dbbf441

fixing base.py

42937bf

templatizing code

a51114d

caching results of runtime features and minor refactoring

24ea3c1

access2rohit force-pushed the lis_slice branch from 7c9f02b to 7ca3d32 Compare August 12, 2019 23:40

fixing local caching in ndarray shape

1d4f263

access2rohit force-pushed the lis_slice branch from 7ca3d32 to 1d4f263 Compare August 13, 2019 00:01

apeforest reviewed Aug 13, 2019

View reviewed changes

apeforest approved these changes Aug 13, 2019

View reviewed changes

apeforest merged commit 05f3ae1 into apache:master Aug 13, 2019

This was referenced Aug 13, 2019

Large Index Support for Slice #15830

Closed

Large Index Support:Part 1 #15728

Closed

ChaiBapchya mentioned this pull request Jan 22, 2020

fix template param as ndim is internally int #17406

Merged

4 tasks

Conversation

access2rohit commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Uh oh!

access2rohit commented Jul 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apeforest Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

access2rohit commented Jul 19, 2019

Uh oh!

ChaiBapchya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

access2rohit commented Jul 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

access2rohit commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apeforest left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

access2rohit commented Jul 18, 2019 •

edited

Loading

apeforest Jul 18, 2019 •

edited

Loading