move exec.reshape to backend by ZiyueHuang · Pull Request #10882 · apache/mxnet

ZiyueHuang · 2018-05-10T06:09:06Z

Description

move exec.reshape to backend

cc @reminisce @eric-haibin-lin @piiswrong

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
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

eric-haibin-lin · 2018-05-10T06:15:51Z

  API_END();
 }

+int MXExecutorReshapeEX(SymbolHandle symbol_handle,


nit: MXExecutorReshapeEX -> MXExecutorReshapeEx

piiswrong

Is there existing test covering reshape?

piiswrong · 2018-05-10T18:16:04Z

+ *! \brief Return a new executor with the same symbol and shared memory,
+ *         but different input/output shapes.
+ */
+MXNET_DLL int MXExecutorReshapeEx(SymbolHandle symbol_handle,


why Ex? is there a MXExecutorReshape already?

reminisce

Please write unit tests for this. It should cover expected behaviors of sizing down and sizing up.

reminisce · 2018-05-10T18:41:23Z

+  for (uint32_t nid : idx.input_nodes()) {
+    std::string name = idx[nid].source->attrs.name;
+    const TShape& new_shape = shape_vec[idx.entry_id(nid, 0)];
+    if (idx.mutable_input_nodes().count(nid) == 0) {


Can you consolidate the implementation for args and aux? There is a big chunk of duplicate code.

reminisce · 2018-05-10T18:42:34Z

+    if (idx.mutable_input_nodes().count(nid) == 0) {
+      NDArray* arr = static_cast<NDArray*>(*arg);
+      NDArray* darr = static_cast<NDArray*>(*grad);
+      if (new_shape == arr->shape()) {


Please simplify this. There is no need to have a do nothing block.

reminisce · 2018-05-10T18:45:15Z

+              << "is more efficient than the reverse."
+              << "If you really want to up size, set allow_up_sizing=True "
+              << "to enable allocation of new arrays.";
+          NDArray* empty_arr = new NDArray(new_shape, arr->ctx(), false, arr->dtype());


This looks like a memory leak. Please use arr.ReshapeAndAlloc(new_shape).

reminisce · 2018-05-10T18:45:39Z

+              << "is more efficient than the reverse."
+              << "If you really want to up size, set allow_up_sizing=True "
+              << "to enable allocation of new arrays.";
+          NDArray* empty_arr = new NDArray(new_shape, arr->ctx(), false, arr->dtype());


Same here. ReshapeAndAlloc.

ZiyueHuang · 2018-05-10T18:56:12Z

Thanks for your comments.
For the unit tests part, I found the existing tests cover both up sizing and down sizing, https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_module.py#L703, https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_module.py#L205, https://github.com/apache/incubator-mxnet/blob/master/tests/python/unittest/test_executor.py#L144.
Will address other parts tomorrow.

reminisce · 2018-05-11T04:32:42Z

+                provided_arg_shape_data.extend(v)
+                provided_arg_shape_idx.append(len(provided_arg_shape_data))
+
+        args_handle, args = self._symbol._get_ndarray_inputs(


I think you can get all the arg/aux/grad arrays from the executor in backend? Is it necessary to prepare them here and pass to the backend again?

ZiyueHuang · 2018-05-14T06:13:49Z

Could you please review again? I realized that Reshape is more appropriate to be a member function of Executor so that it can access the internal arguments of the executor. Also I think ReshapeAndAlloc should not be used, since the original executor should work fine after reshape.

eric-haibin-lin · 2018-05-14T06:18:06Z

+      auto it = arg_grad_map_.find(name);
+      if (partial_shaping || provided_arg_shapes.count(name) || new_shape == arr.shape()) {
+        if (new_shape.Size() > arr.shape().Size()) {
+          CHECK(allow_up_sizing) << name << up_sizing_msg.str();


Need better error message

eric-haibin-lin · 2018-05-14T06:22:06Z

+  size_t grad_top = 0;
+
+  std::ostringstream up_sizing_msg, unspecified_msg;
+  up_sizing_msg << ": Arg of new shape which is larger than original."


<< is expensive. Normal execution doesn't need to construct these long strings.

reminisce

It looks like the existing unit tests have not covered checking whether the reshaped executor and the original executor share NDArrays depending on the new shape sizes. Could you add test cases covering that check? For checking whether two NDArrays are the same in Python, you can use function same_array

reminisce · 2018-05-14T17:15:03Z

+  symbol.outputs = g.outputs;
+  const nnvm::IndexedGraph& idx = g.indexed_graph();
+  nnvm::ShapeVector arg_shapes(idx.input_nodes().size(), TShape());
+  for (size_t i = 0; i < num_forward_inputs_; ++i) {


Call in_args->reserve(num_args) first outside loop? Same for aux_states and arg_grads.

reminisce · 2018-05-14T17:16:05Z

+      if (partial_shaping || provided_arg_shapes.count(name) || new_shape == arr.shape()) {
+        if (new_shape.Size() > arr.shape().Size()) {
+          CHECK(allow_up_sizing) << name << up_sizing_msg.str();
+          in_args->push_back(NDArray(new_shape, arr.ctx(), false, arr.dtype()));


Use in_args->emplace_back(new_shape, arr.ctx(), false, arr.dtype()) to avoid constructing temporary copy of NDArray. Same for all the other push_back(ndarray).

reminisce · 2018-05-14T17:16:29Z

+            arg_grads->push_back(darr.Reshape(new_shape));
+            grad_req_types.push_back(grad_store_.at(grad_top++).first);
+          } else {
+            arg_grads->push_back(NDArray());


Use arg_grads->emplace_back().

reminisce · 2018-05-14T17:19:23Z

+    *aux_states = &(ret->ret_handles[nd_idx]);
+    nd_idx = ret->ret_handles.size();
+  }
+  API_END();


delete out here if an exception is thrown?

reminisce · 2018-05-14T17:20:33Z

+        TShape(provided_arg_shape_data+provided_arg_shape_idx[i],
+          provided_arg_shape_data+provided_arg_shape_idx[i+1]));
+    CHECK(p.second) << "Duplicate shapes are provided for argument "
+      << provided_arg_shape_names[i] << " in simple_bind";


Is it always simple_bind?

ZiyueHuang · 2018-05-17T11:56:16Z

unittest for checking

whether the reshaped executor and the original executor share NDArrays depending on the new shape sizes

is added in test_executor.py @reminisce

ZiyueHuang · 2018-05-21T04:53:00Z

ping @reminisce @piiswrong

szha · 2018-05-21T21:47:33Z

ping @piiswrong

piiswrong · 2018-05-23T19:19:35Z

LGTM. Any concerns? @reminisce @eric-haibin-lin

* move exec.reshape to backend * fix lint * fix lint * fix Symbol._get_ndarray_inputs * update * update * move Reshape as a member function of Executor * address comments

move exec.reshape to backend

02adf98

ZiyueHuang requested a review from szha as a code owner May 10, 2018 06:09

fix lint

c82c105

eric-haibin-lin reviewed May 10, 2018

View reviewed changes

eric-haibin-lin self-assigned this May 10, 2018

ZiyueHuang added 4 commits May 10, 2018 06:37

fix lint

6d00f1f

fix Symbol._get_ndarray_inputs

d05973d

update

3f4cb4d

update

61a172a

piiswrong suggested changes May 10, 2018

View reviewed changes

reminisce suggested changes May 10, 2018

View reviewed changes

reminisce reviewed May 11, 2018

View reviewed changes

move Reshape as a member function of Executor

de842b8

eric-haibin-lin reviewed May 14, 2018

View reviewed changes

reminisce reviewed May 14, 2018

View reviewed changes

address comments

9c5edb4

reminisce approved these changes May 21, 2018

View reviewed changes

szha removed their request for review May 21, 2018 21:47

piiswrong approved these changes May 23, 2018

View reviewed changes

eric-haibin-lin approved these changes May 23, 2018

View reviewed changes

eric-haibin-lin merged commit 704d218 into apache:master May 24, 2018

chinakook mentioned this pull request Jul 18, 2018

Fix MXPredReshape in the c_predict_api #11493

Merged

7 tasks

jes-bz mentioned this pull request Jul 30, 2018

Scala Module API resize is leaking memory on the native size. #10867

Closed

Conversation

ZiyueHuang commented May 10, 2018

Description

Checklist

Essentials

Changes

Comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piiswrong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reminisce left a comment

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!

ZiyueHuang commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZiyueHuang commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reminisce left a comment

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!

ZiyueHuang commented May 17, 2018

Uh oh!

ZiyueHuang commented May 21, 2018

Uh oh!

szha commented May 21, 2018

Uh oh!

piiswrong commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants