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

gigasquid · 2018-08-28T23:22:48Z

Description

The Scala package has updated the DataDesc to include Layout. The Clojure package has been updated to move inline with it.

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

A layout namespace has been created and used in DataDesc
The Clojure package no longer infers the layout from the shape. It handles it the same way as the Scala package by having it be undefined
The original module.fit interop code has been restored. The work arounds required by the old DataDesc has been resolved with this PR [MXNET-689] add DataDesc type for the Scala Package #11844
Most of the examples have moved to use provide-data-desc instead of provide-data
Formatting fixes
Tweak to Char RNN example to run for less epochs before showing the pre-loaded result

…ckage

…if fixed - update Module example to use provide-data-desc and provide-label-desc

gigasquid · 2018-08-28T23:26:30Z

@lanking520 please give a look when you get a chance

lanking520

Thanks for your effort pushing the same changes on Clojure! Overall looks good

lanking520 · 2018-08-29T03:46:31Z

contrib/clojure-package/examples/pre-trained-models/src/pre_trained_models/predict_image.clj

@@ -92,7 +92,7 @@

 (comment

-  (predict "https://raw.githubusercontent.com/dmlc/web-data/master/mxnet/doc/tutorials/python/predict_image/cat.jpg")
+  (predict "https://raw.githubusercontent.com/dmlc/web-data/master/mxnet/doc/tutorials/python/predict_image/cat.jpg" true)


What does this true mean in here?

It's for the example - it controls whether the image url displays in a popup for you view
https://github.com/apache/incubator-mxnet/blob/6fd245d1ce28a7bc09b83aeb2a1233f085210037/contrib/clojure-package/examples/pre-trained-models/src/pre_trained_models/predict_image.clj#L42

nit: not a big deal since it's a helper fn in an example, but it'd make sense to use an options map or kw arg ({:display true} or :display true). [outside this PR though, also]

lanking520 · 2018-08-29T03:49:57Z

contrib/clojure-package/src/org/apache/clojure_mxnet/symbol.clj

@@ -144,7 +144,7 @@
   which must be known from the rest of the net."
  ([start {:keys [step repeat dtype]
           :or {step (float 1) repeat (int 1) dtype base/MX_REAL_TYPE}
-          :as opts}]
+           :as opts}]


space issue...

In this case the cljfmt tool is fixing my earlier mistake :)

gigasquid · 2018-08-29T23:41:07Z

Thanks for the feedback @lanking520 😸

lanking520 · 2018-08-30T00:41:07Z

@gigasquid Sorry about not being so helpful there... I will try to learn Clojure grammer and review it again... Generally, the Scala DataDesc want user to pass in as DataDesc type instead of ListMap. Then most of the iterators needs to change a little bit based on this change.

gigasquid · 2018-08-30T00:56:36Z

Thanks @lanking520 I did change the NDarrayIter to reflect this change. You can probably see it easiest in the test case https://github.com/apache/incubator-mxnet/pull/12387/files#diff-93817a3c2dc0abcdbb709a400bddc997R194

Please feel free to ping me with any questions.

benkamphaus · 2018-08-31T14:50:57Z

contrib/clojure-package/src/org/apache/clojure_mxnet/io.clj

-        last-batch-handle
-        data-name
-        label-name))
+   (let [specify-data-desc? (map? data)]


nit: is this just in the let for a self-documenting name/rationale?

I think it can be dropped - thanks

benkamphaus

just a nit, otherwise LGTM

gigasquid · 2018-08-31T18:25:28Z

Thanks for the feedback @benkamphaus.

@nswamy/ @szha - I think this is ready for a final review

lanking520 · 2018-09-02T19:03:17Z

@nswamy @yzhliu Please take a look since it's been for a while

yzhliu

good to me for the rest.

yzhliu · 2018-09-11T20:39:30Z

contrib/clojure-package/src/org/apache/clojure_mxnet/io.clj

-        last-batch-handle
-        data-name
-        label-name))
+   (if (map? data)


does it mean if data is map? what if it is not? I don't know clojure well, just want to make sure it is intended.

Yes - it checks to see if the data is a map, if so, it is in the form of having a DataDesc associated with it and will be dispatched to the correct Java function signature and with scala interop. If it is not a map, it will dispatch to the original Java function signature without DataDesc.

The argument checking for the correct data structures can be improved by using core.spec in Clojure. It adds gradual type checking. It is in use in the module api, but it hasn't been added in yet in this namespace. I added a line item in the TODO page for the Clojure package to capture it for later improvement work.

gigasquid · 2018-09-11T21:40:14Z

Thanks @yzhliu for the feedback 😸

Unless anyone has any objections, I will go ahead a merge shortly.

… in Scala Package (apache#12387) * Bring clojure package inline with new DataDesc and Layout in Scala package * formatting cljfmt * revert the implementation of module fit back now that DataDesc issue if fixed - update Module example to use provide-data-desc and provide-label-desc * update to provide-data-desc and provide-label-desc * decrease epochs to speed example * Add tests and docstrings * remove let

gigasquid added 6 commits August 26, 2018 16:33

Bring clojure package inline with new DataDesc and Layout in Scala pa…

637e10e

…ckage

formatting cljfmt

5db1306

revert the implementation of module fit back now that DataDesc issue …

44fa15c

…if fixed - update Module example to use provide-data-desc and provide-label-desc

update to provide-data-desc and provide-label-desc

a1aa0f2

decrease epochs to speed example

25959cc

Add tests and docstrings

6fd245d

gigasquid added the Clojure label Aug 28, 2018

gigasquid changed the title ~~MXNET-873 - Bring Clojure Package inline with new DataDesc and Layout in Scala Package~~ MXNET-873 - Bring Clojure Package Inline with New DataDesc and Layout in Scala Package Aug 28, 2018

lanking520 reviewed Aug 29, 2018

View reviewed changes

benkamphaus reviewed Aug 31, 2018

View reviewed changes

benkamphaus approved these changes Aug 31, 2018

View reviewed changes

remove let

a53f869

Merge branch 'master' into update-data-desc-clojure

f8a125b

szha requested a review from yzhliu September 2, 2018 19:08

gigasquid added the pr-awaiting-review PR is waiting for code review label Sep 3, 2018

yzhliu reviewed Sep 11, 2018

View reviewed changes

gigasquid removed the pr-awaiting-review PR is waiting for code review label Sep 13, 2018

gigasquid merged commit b8153f6 into apache:master Sep 13, 2018

gigasquid deleted the update-data-desc-clojure branch September 13, 2018 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gigasquid commented Aug 28, 2018

gigasquid commented Aug 28, 2018

lanking520 left a comment

lanking520 Aug 29, 2018

gigasquid Aug 30, 2018

benkamphaus Aug 31, 2018

gigasquid Aug 31, 2018

lanking520 Aug 29, 2018

gigasquid Aug 31, 2018

gigasquid commented Aug 29, 2018

lanking520 commented Aug 30, 2018

gigasquid commented Aug 30, 2018

benkamphaus Aug 31, 2018

gigasquid Aug 31, 2018

benkamphaus left a comment

gigasquid commented Aug 31, 2018

lanking520 commented Sep 2, 2018

yzhliu left a comment

yzhliu Sep 11, 2018

gigasquid Sep 11, 2018

gigasquid commented Sep 11, 2018

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

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

Conversation

gigasquid commented Aug 28, 2018

Description

Checklist

Essentials

Changes

gigasquid commented Aug 28, 2018

lanking520 left a 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

gigasquid commented Aug 29, 2018

lanking520 commented Aug 30, 2018

gigasquid commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benkamphaus left a comment

Choose a reason for hiding this comment

gigasquid commented Aug 31, 2018

lanking520 commented Sep 2, 2018

yzhliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gigasquid commented Sep 11, 2018