[MXNET-689] add DataDesc type for the Scala Package #11844

lanking520 · 2018-07-20T22:13:37Z

Description

This PR includes massive changes on all Iterators that extends DataIter, two new fields are added (dtype and layout).

@nswamy @yzhliu @andrewfayres

It intends to solve some issues about the layout information missing in DataDesc. For example: RNN based networks require different layouts(“TNC”/”NTC”). Current implementation of DataBatch, DataIter does not support different layouts and assumes a fixed layout based on the shape of the NDArray returned in the next() call to the iterator.

However I found it hard to have a general solution that both address backward compatibility as well as adding this information. Currently, there are several ways to address this issue:

Assume Shape == 2 follows NT format, Shape == 3 follows TNC, Shape == 4 follows NCHW and only change the implicit function in here.
Add Layout and DType defs for all iterator, then users can pass these information to create DataDesc instead of ListMap[Name, Shape]. However, it breaks the input args for NDArrayIter as well as other iterators. Users are required to add more inputs. However, if the iterator are passed in with dynamic shapes (shapes are not determined), it will break the DataDesc checking scheme.
Do what 2 contains and create this constructor to make sure BC.

Test

IOSuite
ModuleSuite
DataIterSuite

Historical issues

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

lanking520 · 2018-07-20T22:18:46Z

When I am in the middle of changes, some parts looks confusing to me. Are we defining these field in the dataiter class or we fetch these information through databatch. There are different solution in different DataIters' child classes. The currently issue that failing the test is actually here:

  /**
   * DataIter creator
   * @param handle native memory ptr for the iterator
   * @param params parameter passed to the iterator
   * @return created DataIter
   */
  private def creator(handle: DataIterCreator)(
    params: Map[String, String]): DataIter = {
    val out = new DataIterHandleRef
    val keys = params.keys.toArray
    val vals = params.values.toArray
    checkCall(_LIB.mxDataIterCreateIter(handle, keys, vals, out))
    val dataName = params.getOrElse("data_name", "data")
    val labelName = params.getOrElse("label_name", "label")
    new MXDataIter(out.value, dataName, labelName)
  }

This creator did not pass in the layout and dtype information and MXDataIter extends DataIter. By default, the layout are set a "NCHW" that causing the problem.

yzhliu · 2018-07-23T03:15:59Z

scala-package/core/src/main/scala/org/apache/mxnet/io/MXDataIter.scala

+      (null, null)
+    }
+
+  private val (_provideDataDesc: IndexedSeq[DataDesc],


can we only keep DataDesc and construct ListMap from DataDesc? or at least combine it with L45-58

nswamy · 2018-07-22T06:46:04Z

scala-package/core/src/main/scala/org/apache/mxnet/IO.scala

+      * @param dtype The dtype of the label, default is Float32
+      * @return this
+      */
+    def setDType(dtype: DType): Builder = {


can you specify the default value here

nswamy · 2018-07-22T06:50:06Z

scala-package/core/src/main/scala/org/apache/mxnet/IO.scala

@@ -140,7 +140,9 @@ class DataBatch(val data: IndexedSeq[NDArray],
                // use ListMap to indicate the order of data/label loading
                // (must match the order of input data/label)
                private val providedData: ListMap[String, Shape] = null,
-                private val providedLabel: ListMap[String, Shape] = null) {
+                private val providedLabel: ListMap[String, Shape] = null,


I think you should replace this constructor with provideDataShape, provideLabelShape that DataDesc instead ListMap[String, Shape] and constructor(this) another to preserve ListMap so we don't break backwards compatibility

nswamy · 2018-07-22T06:50:45Z

scala-package/core/src/main/scala/org/apache/mxnet/IO.scala

@@ -170,6 +172,8 @@ object DataBatch {
    private var label: IndexedSeq[NDArray] = null
    private var index: IndexedSeq[Long] = null
    private var pad: Int = 0
+    private var layout: String = "NCHW"


we wouldn't need this if add a new constructor with DataDesc

nswamy · 2018-07-22T06:52:57Z

scala-package/core/src/main/scala/org/apache/mxnet/IO.scala

@@ -314,6 +351,12 @@ abstract class DataIter extends Iterator[DataBatch] {
  // The name and shape of label provided by this iterator
  def provideLabel: ListMap[String, Shape]

+  // Provide type:DataDesc of the data
+  def provideDataDesc: IndexedSeq[DataDesc]


can we deprecate the old ones?

nswamy · 2018-07-23T18:22:22Z

scala-package/core/src/main/scala/org/apache/mxnet/io/MXDataIter.scala

+    * Get the DType
+    * @return DType
+    */
+  def getDType(): DType = {


suggest to add two new parameters to the constructor dataDescriptor and labelDescriptor.
Make sure not break API backwards compatibility

lanking520 · 2018-07-24T20:51:22Z

After some days spent on the issues, I finally figure out the fix on the issues we have. @nswamy and @yzhliu thanks for your comments on the changes and things I am about to apply is:

Depreciate provideData and provideLabel methods
disable implicit transformation in IO to use DataDesc everywhere
@andrewfayres please join the review if you have time :-)
Please provide a secondary review before I apply these changes. There will be a final review session once changes are applied and everything I felt is settled.

nswamy · 2018-07-24T22:36:52Z

scala-package/core/src/main/scala/org/apache/mxnet/io/NDArrayIter.scala

@@ -59,10 +61,12 @@ class NDArrayIter(data: IndexedSeq[(String, NDArray)],
  def this(data: IndexedSeq[NDArray], label: IndexedSeq[NDArray] = IndexedSeq.empty,
           dataBatchSize: Int = 1, shuffle: Boolean = false,
           lastBatchHandle: String = "pad",
-           dataName: String = "data", labelName: String = "label") {
+           dataName: String = "data", labelName: String = "label",
+           dType: DType = MX_REAL_TYPE, dataLayout: String = "NCHW",


no need to add this here. add the defaults to the new constructor above in L47

create another this constructor with the old set of constructor args to not break backwards compatibility.

thinking about it, can you first add default to the class constructor and test

its ok to keep here. let all the defaults remain in one place.

nswamy · 2018-07-26T05:35:58Z

scala-package/core/src/main/scala/org/apache/mxnet/IO.scala

@@ -170,6 +180,10 @@ object DataBatch {
    private var label: IndexedSeq[NDArray] = null
    private var index: IndexedSeq[Long] = null
    private var pad: Int = 0
+    private var dataLayout: String = "NCHW"


For layouts I suggest to use extensible Enums similar to this https://blogs.oracle.com/darcy/mixing-in-an-enum.
free floating strings are generally not a good practice because it removes compile time type-checks.
WDYT? does it make sense?

lanking520 · 2018-07-26T17:54:12Z

Hi @benkamphaus, could you please take a look at the recent CI failure on Clojure. I believe these lines cause that failure: https://github.com/apache/incubator-mxnet/blob/master/contrib/clojure-package/src/org/apache/clojure_mxnet/io.clj#L194-L225. I have modified the input args for NDArrayIter and there will be more optional fields

nswamy · 2018-07-26T21:46:18Z

@benkamphaus, from Clojure perspective are we breaking backwards compatibility. I am curious to know how it is handled, since we added new parameters with defaults.

benkamphaus · 2018-07-30T02:53:37Z

It appears to be the case that the new constructor arglist for NDArrayIter breaks backwards compatibility, which is the cause of the current Clojure CI broken message:

No matching ctor found for class org.apache.mxnet.io.NDArrayIter, compiling:(org/apache/clojure_mxnet/io.clj:216:4)

The cause is straight forward -- the constructor call here is no longer valid due to the change to the args required for NDArrayIter as declared here.

This constructor call on the clojure side backs ndarray-iter, and there are tests, specs, and examples and the changes in required args to ndarray-iter would need those to change as well. (i.e. this impacts the API/contract level, not just an implementation concern below those).

Note that the keyword args in the Clojure API means that ndarray-iter could back a call to both the previous NDArrayIter constructor and the form with the new args (using additional, optional args to the same function in the Clojure API). Otherwise, I'm not sure what the expectation/desire is here without more context.

I'll discuss with @gigasquid as well.

lanking520 · 2018-07-30T03:01:57Z

@benkamphaus thanks for your detailed analysis. I can tell how hard it is if we change the args in here since I also changed a lot of examples to match this change in Scala. Let's be cautious on this change and discuss if it is necessary here to introduce more parameters.

lanking520 · 2018-08-01T18:46:25Z

I have reverted the changes back to Strings. Next step is to add a default value to Layout section so we can bypass the require check

lanking520 · 2018-08-07T04:30:15Z

In the recent two PR, add Backward Compatibility fix on DataIter as well as DataBatch. User can still go with the old defs...

It looks like if we want to force conversion from Float32 to Int32 will cause a crash on JVM. Need to be addressed.

This reverts commit abbe283.

…1844)" This reverts commit abbe283.

* add dataDesc * Add amend * add changes with dataLayout and labelLayout * add depreciate and example changes * Gan and Customop fixes * change the DType * add one more class to convert Strings to DTypes * convert layout to global * scala style fix * Revert to 8c7d1f8 * fix coding style issue * print full stacktraces * apply changes to new constructor * add databatch bcc * introduce undefined field * Fix crashes when change provideData to provideDataDesc It looks like if we want to force conversion from Float32 to Int32 will cause a crash on JVM. Need to be addressed. * change spacing and revert test * apply DataDesc on DataBatch * unit test for NDArrayIter and MXDataiter * apply changes on CR * change NDArrayIter and revert the rest * revert change on examples * apply final changes * remove the provideLabelShape * add TODO about the findings

lanking520 requested review from nswamy and yzhliu as code owners July 20, 2018 22:13

lanking520 mentioned this pull request Jul 20, 2018

NPE in BucketingModule when providedLabel of DataBatch is not set #11775

Closed

yzhliu reviewed Jul 23, 2018

View reviewed changes

lanking520 force-pushed the dataiter branch from 203d237 to 9332064 Compare July 23, 2018 16:43

nswamy reviewed Jul 23, 2018

View reviewed changes

lanking520 force-pushed the dataiter branch from 9332064 to a779124 Compare July 24, 2018 20:45

lanking520 changed the title ~~[MXNET-689][WIP] add DataDesc type for the Scala Package~~ [MXNET-689] add DataDesc type for the Scala Package Jul 24, 2018

nswamy reviewed Jul 24, 2018

View reviewed changes

lanking520 force-pushed the dataiter branch from 7340653 to 4ce87dd Compare July 25, 2018 06:39

nswamy reviewed Jul 26, 2018

View reviewed changes

lanking520 force-pushed the dataiter branch from c3d75bd to 18bd6b9 Compare July 27, 2018 18:30

nswamy mentioned this pull request Jul 30, 2018

[MXNET-836] RNN Example for Scala #11753

Merged

5 tasks

lanking520 closed this Jul 30, 2018

lanking520 deleted the dataiter branch July 30, 2018 21:42

lanking520 restored the dataiter branch July 30, 2018 21:43

lanking520 reopened this Jul 30, 2018

lanking520 force-pushed the dataiter branch from 5c3ca86 to 2f4e208 Compare July 30, 2018 21:48

lanking520 force-pushed the dataiter branch 3 times, most recently from 6c8289a to f49eeca Compare August 7, 2018 02:17

lanking520 added 21 commits August 15, 2018 21:29

add depreciate and example changes

b4fada3

Gan and Customop fixes

a2b6713

change the DType

ad3f73c

add one more class to convert Strings to DTypes

559ed96

convert layout to global

33acc7b

scala style fix

ff34f96

Revert to 8c7d1f8

1252d1f

fix coding style issue

a0609d0

print full stacktraces

75b518c

apply changes to new constructor

ed92e73

add databatch bcc

83b5826

introduce undefined field

0e170cf

Fix crashes when change provideData to provideDataDesc

67fed73

It looks like if we want to force conversion from Float32 to Int32 will cause a crash on JVM. Need to be addressed.

change spacing and revert test

46754f0

apply DataDesc on DataBatch

8c7fac1

unit test for NDArrayIter and MXDataiter

70aa7f4

apply changes on CR

e8d1a40

change NDArrayIter and revert the rest

44bb97e

revert change on examples

09db459

apply final changes

3f862e4

remove the provideLabelShape

ad28059

lanking520 force-pushed the dataiter branch from 9a669a7 to ad28059 Compare August 16, 2018 04:30

add TODO about the findings

93329a1

nswamy merged commit abbe283 into apache:master Aug 17, 2018

marcoabreu added a commit that referenced this pull request Aug 17, 2018

Revert "[MXNET-689] add DataDesc type for the Scala Package (#11844)"

1ac4f42

This reverts commit abbe283.

piyushghai added a commit to piyushghai/incubator-mxnet that referenced this pull request Aug 17, 2018

Revert "[MXNET-689] add DataDesc type for the Scala Package (apache#1…

dd9f5e5

…1844)" This reverts commit abbe283.

gigasquid mentioned this pull request Aug 28, 2018

MXNET-873 - Bring Clojure Package Inline with New DataDesc and Layout in Scala Package #12387

Merged

5 tasks

lanking520 mentioned this pull request Aug 30, 2018

Scala: DataDesc IllegalArgumentException with simple example #12409

Closed

lanking520 deleted the dataiter branch September 19, 2018 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-689] add DataDesc type for the Scala Package #11844

[MXNET-689] add DataDesc type for the Scala Package #11844

lanking520 commented Jul 20, 2018 •

edited

Loading

lanking520 commented Jul 20, 2018

yzhliu Jul 23, 2018

nswamy Jul 22, 2018

nswamy Jul 22, 2018

nswamy Jul 22, 2018

nswamy Jul 22, 2018

nswamy Jul 23, 2018

lanking520 commented Jul 24, 2018

nswamy Jul 24, 2018

nswamy Jul 24, 2018 •

edited

Loading

nswamy Jul 24, 2018

nswamy Jul 26, 2018

lanking520 commented Jul 26, 2018

nswamy commented Jul 26, 2018

benkamphaus commented Jul 30, 2018

lanking520 commented Jul 30, 2018

lanking520 commented Aug 1, 2018

lanking520 commented Aug 7, 2018

[MXNET-689] add DataDesc type for the Scala Package #11844

[MXNET-689] add DataDesc type for the Scala Package #11844

Conversation

lanking520 commented Jul 20, 2018 • edited Loading

Description

Test

Historical issues

Checklist

Essentials

lanking520 commented Jul 20, 2018

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

lanking520 commented Jul 24, 2018

Choose a reason for hiding this comment

nswamy Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lanking520 commented Jul 26, 2018

nswamy commented Jul 26, 2018

benkamphaus commented Jul 30, 2018

lanking520 commented Jul 30, 2018

lanking520 commented Aug 1, 2018

lanking520 commented Aug 7, 2018

lanking520 commented Jul 20, 2018 •

edited

Loading

nswamy Jul 24, 2018 •

edited

Loading