[MXNET-82] Sparse op tutorial for developers by eric-haibin-lin · Pull Request #10081 · apache/mxnet

eric-haibin-lin · 2018-03-12T22:48:23Z

Description

A guide to implement sparse ops
requires #10091

Checklist

Essentials

Passed code style checking (make lint)
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
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

anirudh2290 · 2018-03-13T19:00:50Z

docs/how_to/add_sparse_op_in_backend.md

+  const nnvm::dim_t num_rows = output.shape()[0];                               // 37
+  output.CheckAndAlloc({Shape1(num_rows + 1), Shape1(nnz)});                    // 38
+  MSHADOW_TYPE_SWITCH(output.dtype(), DType, {                                  // 39
+    MSHADOW_TYPE_SWITCH(output.aux_type(kIdx), CType, {                         // 40


MSHADOW_IDX_TYPE_SWITCH?

Good catch!

Good catch! I'll update it

aaronmarkham

Some suggestions provided.

aaronmarkham · 2018-03-15T18:24:25Z

docs/how_to/add_sparse_op_in_backend.md

+infers the output storage type based on operator arguments and inputs types. Then we implement the forward
+function for the case where c is 0.0 and x is a CSRNDArray.
+
+Next, we are going to


Describe what we're generally going to do with the next bullets. Should you summarize the rest of the doc and include the unit test section?
"The next steps will go into detail on how to create a sparse operator in C++:"

Good point. I'll add that

aaronmarkham · 2018-03-15T18:26:49Z

docs/how_to/add_sparse_op_in_backend.md

@@ -0,0 +1,429 @@
+# A Guide to Implementing Sparse Operators in MXNet Backend


in C++ ?
Shouldn't you also add this to the tutorials page under the C++ section?

I only found one tutorial under docs/tutorials/c++/basic.md. My understanding is that basic.md is for the cpp-package, one of the language bindings for MXNet, which belongs to the frontend. The readers would only care about how to use MXNet.

The tutorial I wrote is, however, for developers to get into the backend of MXNet. They build MXNet / add features to MXNet. I don't think it's reasonable to move it there.

If it has to be moved, I would move to some folder of tutorials for MXNet developers.

oh, but anywhere than faq... the broad bucket makes it hard to find... it should go in a logical folder... even if you have to make a backend folder...

aaronmarkham · 2018-03-15T18:27:40Z

docs/how_to/add_sparse_op_in_backend.md

+<NDArray 2x2 @cpu(0)>
+```
+
+The statement `z = mx.nd.quadratic(x, a=1, b=2, c=3)` generates a warning message which says


Your output above didn't have a warning message...

Yeah I could add that ...

aaronmarkham · 2018-03-15T18:29:13Z

docs/how_to/add_sparse_op_in_backend.md

+
+The statement `z = mx.nd.quadratic(x, a=1, b=2, c=3)` generates a warning message which says
+the sparse input is converted to dense storage, and the dense operator is used to compute the dense output.
+This is the "storage fallback" mechanism in MXNet, where a dense operator is automatically used for


Reword? De-complicate.

The concept was introduced in the previous two sparse tutorials (in pre-requisite). I'll shorten the description and refer to the previous tutorials if the readers don't understand.

aaronmarkham · 2018-03-15T18:32:09Z

docs/how_to/add_sparse_op_in_backend.md

+# A Guide to Implementing Sparse Operators in MXNet Backend
+
+## Prerequisites
+- Basic knowledge of [how to implement a dense operator in MXNet backend](https://mxnet.incubator.apache.org/versions/master/how_to/add_op_in_backend.html)


This moved:
https://mxnet.incubator.apache.org/faq/add_op_in_backend.html
And isn't really titled with "dense", but maybe it should be. Maybe rephrase to say that previously, backend ops were dense.
Also, I think you might want to update that page cross-referencing this one and introduce that you now have this dense v sparse option for ops. And also retitle it?

As a prerequisite I'm reading that as well and noted that it needs some updates too. I'll circle back on those as a comment to this PR.

I would prefer updating the title of previous tutorial to be "dense operator" instead of "operator". @reminisce is that okay?

aaronmarkham · 2018-03-15T19:14:33Z

docs/how_to/add_sparse_op_in_backend.md

+      const std::vector<OpReqType>& req,
+      const std::vector<NDArray>& outputs);
+```
+where the vectors of TBlobs are replaced with vectors of NDArrays. Now, let's go through a few important methods in the NDArray class.


Start a new sentence.

aaronmarkham · 2018-03-15T19:15:42Z

docs/how_to/add_sparse_op_in_backend.md

+```
+
+### Storage Type Inference
+Storage type inference is the process of deducing storage types of `NDArray`s


no tick on ndarray

aaronmarkham · 2018-03-15T19:18:54Z

docs/how_to/add_sparse_op_in_backend.md

+python frontend so that `quadratic` is accessible from both `mx.symbol.sparse`
+and `mx.ndarray.sparse`.
+- .set_attr<FInferStorageType>("FInferStorageType", QuadraticOpStorageType)
+This line register the storage type inference attribute of the operator.


Good catch. Thanks

aaronmarkham · 2018-03-15T19:19:03Z

docs/how_to/add_sparse_op_in_backend.md

+- .set_attr<FInferStorageType>("FInferStorageType", QuadraticOpStorageType)
+This line register the storage type inference attribute of the operator.
+- .set_attr<FComputeEx>("FComputeEx<cpu>", QuadraticOpForwardEx<cpu>)
+This line register the `FComputeEx` attribute of the operator.


aaronmarkham · 2018-03-15T19:21:35Z

docs/how_to/add_sparse_op_in_backend.md

+So far, only the forward operator supports sparse inputs. To add sparse support to the
+backward operator, you also need to register these two attributes to `_backward_quadratic`:
+- `FComputeEx` for sparse operator implementation
+- `FInferStorage` for storage tyep inference in backward


fix type; add period or use semicolon before Due
in backward "what?"... the backward operator?

Fixed type; added period. Updated to "backward computation"

aaronmarkham · 2018-03-15T19:23:57Z

Prerequisite add op in backend doc updates:

References a move that seems to have happened, but now the files are in both places...
https://github.com/apache/incubator-mxnet/blame/master/docs/faq/add_op_in_backend.md#L48-L51
Link to specific instructions on adding descriptions
https://github.com/apache/incubator-mxnet/blame/master/docs/faq/add_op_in_backend.md#L95
Reference to FCompute is pretty light, and I'm unclear on the comparison with FComputeX in this sparse tutorial.

For my review on the tutorial for sparse ops, it looks like it was outdated before I finished it, so please take a look at all of my "outdated" comments!

thinksanky · 2018-03-15T19:33:13Z

All how_to are moved to faq and we should not use how_to anymore. Is there a reason for updating how_to ?

eric-haibin-lin · 2018-03-16T20:33:30Z

@aaronmarkham made updates according to your comments. Please review the most recent commit.
Regarding FCompute and FComputeEx:

I had a paragraph explaining the why the FCompute interface is limited and cannot be used for sparse ops. Does it make sense?

Notice the FCompute interface includes TBlobs, which don't include data structures that could be used to query storage
types of inputs, nor manipulate auxiliary arrays like indices and indptr.
Therefore, instead of the FCompute interface, sparse operators are registered with the following FComputeEx interface

eric-haibin-lin · 2018-03-16T20:33:44Z

@thinksanky moved to faq now

Ishitori · 2018-03-20T22:08:29Z

docs/faq/add_sparse_op_in_backend.md

+
+Note that the statement `z = mx.nd.quadratic(x, a=1, b=2, c=3)` generates a warning message, saying that
+a dense operator is used when the sparse operator doesn't support the above case. If you are not
+familiar with the storag fallback mechanism, please revisit the tutorials for


Typo in "storag" -> "storage"

Ishitori · 2018-03-20T22:27:27Z

docs/faq/add_sparse_op_in_backend.md

+### Operator Registration
+Finally let's extend the operator registration logic to expose `sparse.quadratic`
+to frontend. Below is the extended registration code in `quadratic_op.cc`:
+```cpp


Having line numbers here will you to reference to them later, and help readers to understand the content.

Sure I'll add it

Ishitori · 2018-03-20T22:32:06Z

docs/faq/add_sparse_op_in_backend.md

+backward operator, you also need to register these two attributes to `_backward_quadratic`:
+- `FComputeEx` for sparse operator implementation
+- `FInferStorage` for storage type inference in the backward computation.
+Due to length constraint, this is left as an exercise for readers.


Maybe write another tutorial on that? Having more ground truth implementations may increase number of contributions.

eric-haibin-lin · 2018-03-26T23:31:30Z

Guide added to the confluence page:
https://cwiki.apache.org/confluence/display/MXNET/A+Guide+to+Implementing+Sparse+Operators+in+MXNet+Backend

This reverts commit a2c4b0a.

eric-haibin-lin requested review from cjolivier01 and szha as code owners March 12, 2018 22:48

anirudh2290 reviewed Mar 13, 2018

View reviewed changes

aaronmarkham suggested changes Mar 15, 2018

View reviewed changes

eric-haibin-lin changed the title ~~[MXNET-82] [WIP] Sparse op tutorial for developers~~ [MXNET-82] Sparse op tutorial for developers Mar 16, 2018

Ishitori reviewed Mar 20, 2018

View reviewed changes

eric-haibin-lin force-pushed the sparse_op_tutorial branch 2 times, most recently from 16f8d82 to 0306717 Compare March 23, 2018 04:22

code check in

5f835b7

eric-haibin-lin force-pushed the sparse_op_tutorial branch from d7e8772 to 5f835b7 Compare March 26, 2018 21:24

eric-haibin-lin merged commit a2c4b0a into apache:master Mar 26, 2018

eric-haibin-lin deleted the sparse_op_tutorial branch March 27, 2018 04:05

ashokei pushed a commit to ashokei/incubator-mxnet that referenced this pull request Mar 27, 2018

add guide for implementing sparse ops (apache#10081)

6c25054

jinhuang415 pushed a commit to jinhuang415/incubator-mxnet that referenced this pull request Mar 30, 2018

add guide for implementing sparse ops (apache#10081)

479a35b

cjolivier01 pushed a commit to cjolivier01/mxnet that referenced this pull request Mar 30, 2018

Revert "add guide for implementing sparse ops (apache#10081)"

d9ebbec

This reverts commit a2c4b0a.

rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018

add guide for implementing sparse ops (apache#10081)

670d939

zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018

add guide for implementing sparse ops (apache#10081)

bb93618

		@@ -0,0 +1,429 @@
		# A Guide to Implementing Sparse Operators in MXNet Backend

Conversation

eric-haibin-lin commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

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!

aaronmarkham 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!

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!

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!

aaronmarkham commented Mar 15, 2018

Uh oh!

thinksanky commented Mar 15, 2018

Uh oh!

eric-haibin-lin commented Mar 16, 2018

Uh oh!

eric-haibin-lin commented Mar 16, 2018

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!

eric-haibin-lin commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

eric-haibin-lin commented Mar 12, 2018 •

edited

Loading