[MXNET-357] New Scala API Design (Symbol) by lanking520 · Pull Request #10660 · apache/mxnet

lanking520 · 2018-04-23T22:43:56Z

Description

See full design document
@nswamy @yzhliu
This PR is the Addition for new Symbol functions of Scala API

Checklist

Essentials

Use QuasiQuote to replace original API Implementation (Reduce lines)
MakeAtomicFunction change to support String type parameter type in
User New namespace for New API (temporarily Symbol.api.function_name)
Default args using None to pass in as default
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

yzhliu · 2018-04-24T23:29:38Z

-          )
-        )
-      )
+    val newFunctionDefs = newSymbolFunctions map { symbolfunction =>


remove if it is not used currently.

It will be implemented in the next commit

I know, but remove this for now.

yzhliu · 2018-04-24T23:32:18Z

+
+  private def argumentCleaner(argType : String) : (String, Boolean) = {
+    val spaceRemoved = argType.replaceAll("\\s+", "")
+    var commaRemoved : Array[String] = new Array[String](0)


looks like you can write

val commaRemoved = if ... else ...

Seemed not applicable as I need to change one of the element in the Array

yzhliu · 2018-04-24T23:34:46Z

    // scalastyle:on println
-    (aliasName, new SymbolFunction(handle, keyVarNumArgs.value))
+    val argList = argNames zip argTypes map { case (argName, argType) =>
+        val tup = argumentCleaner(argType)


name the return tuple.

yzhliu · 2018-04-24T23:37:20Z

+      val endIdx = spaceRemoved.indexOf('}')
+      commaRemoved = spaceRemoved.substring(endIdx + 1).split(",")
+      // commaRemoved(0) = spaceRemoved.substring(0, endIdx+1)
+      commaRemoved(0) = "string"


could you explain more about the process logic here? I don't quite get the point.

The input can be in the format:
e.g: stype : {'csr', 'default', 'row_sparse'}
In which case we need to get rid of the "{}" and set the type as string. This part is just a part of the data cleaning

yzhliu · 2018-04-24T23:37:59Z

+      commaRemoved = spaceRemoved.split(",")
+    }
+    // Optional Field
+    if (commaRemoved.length >= 3) {


2 for name and attrs ?

There are current different format for this, usually:
arg : Type
arg: Type, required
arg: Type, optional, default = Null
The logic here is trying to handle all of these cases

We'd better make the pattern clear. For example, do assertion

if (commaRemoved.length >= 3) { // arg: Type, optional, default = Null require(commaRemoved[1] == "required") require(commaRemoved[2].startsWith("default = ")

so that if some other patterns appear one day, we can fail immediately and have chance to fix it before it leaks to public and causes strange error.

yzhliu · 2018-04-24T23:39:54Z

    val opNames = ListBuffer.empty[String]
    _LIB.mxListAllOpNames(opNames)
-    opNames.map(opName => {
+    opNames.filter(!_.startsWith("_")).map(opName => {


why filter _ ?

The reason for filter _ is to remove the internal function to be compiled. The Documentation for Internal function has not been updated for a long time.

Can we have a list of those internal functions? Since it is a broken for api compatibility, we need to review whether it is safe to remove.

btw, it removes _contrib_ as well, you don't mean that, right?

Yeah, not mean't to remove contrib

lanking520 · 2018-04-25T20:35:56Z

The recent commit including the new API functions, currently we call them "New". To access them, build the package and call <Function_Name>New and you will be able to access.

In order to merge this PR, Example is required to prove the API will functioned normally. At least one example will be added in this case

yzhliu · 2018-05-01T00:52:00Z

+        val funcName = symbolfunction.name
+        val tName = TermName(funcName)
+        q"""
+            @Deprecated


suggest to deprecate them later, when the new api is proved to be stable.

Will remove it in the next commit

yzhliu · 2018-05-01T00:53:45Z

+      impl += "createSymbolGeneral(\"" + symbolfunction.name + "\", name, attr, Seq(), map.toMap)"
+      // Combine and build the function string
+      val returnType = "org.apache.mxnet.Symbol"
+      var finalStr = s"def ${symbolfunction.name}New"


How about postfix 'Ex' for 'Expand', which is also consistent with those in c_api.h

Instead of adding postfix, we decided to call the API as Symbol.api.Function name

yzhliu · 2018-05-01T01:07:43Z

+    if (spaceRemoved.charAt(0)== '{') {
+      val endIdx = spaceRemoved.indexOf('}')
+      commaRemoved = spaceRemoved.substring(endIdx + 1).split(",")
+      commaRemoved(0) = "string"


if argType = {'csr', 'default', 'row_sparse'}, then we will do typeConversion("string", "{'csr', 'default', 'row_sparse'}") ?

if so, then these two lines are pretty confusing. why not simply commaRemoved = Array("string")

Previously I was thinking adding these into the default field, but just found them unnecessary. The reason not doing Array("string") is we are not sure if this arg contains optional field. We need to do a split "," to make sure of that

Adding more comments here to avoid misunderstanding

yzhliu · 2018-05-01T01:18:41Z

+      commaRemoved = spaceRemoved.split(",")
+    }
+    // Optional Field
+    if (commaRemoved.length >= 3) {


We'd better make the pattern clear. For example, do assertion

if (commaRemoved.length >= 3) { // arg: Type, optional, default = Null require(commaRemoved[1] == "required") require(commaRemoved[2].startsWith("default = ")

so that if some other patterns appear one day, we can fail immediately and have chance to fix it before it leaks to public and causes strange error.

nswamy

Great Job! 💯 . This is using advanced features and scala macros are really cryptic, I want to encourage you to add lots of comments so someone else do not have to break their head like you had to.

nswamy · 2018-05-01T21:57:26Z

+ */
+package org.apache.mxnet
+@AddNewSymbolFunctions(false)
+object NewSymbol {


NewSymbol=> SymbolAPI
?

nswamy · 2018-05-01T22:19:07Z

  private val functions: Map[String, SymbolFunction] = initSymbolModule()
  private val bindReqMap = Map("null" -> 0, "write" -> 1, "add" -> 3)

+  val api = NewSymbol


val api : SymbolBase = SymbolAPI

nswamy · 2018-05-01T22:20:35Z

  private[mxnet] def macroTransform(annottees: Any*) = macro SymbolImplMacros.addDefs
 }

+private[mxnet] class AddNewSymbolFunctions(isContrib: Boolean) extends StaticAnnotation {


AddNewSymbolFunctions-> AddSymbolAPIs ?

nswamy · 2018-05-01T22:24:45Z

+ * limitations under the License.
+ */
+package org.apache.mxnet
+@AddNewSymbolFunctions(false)


AddNewSymbolFunctions->GenerateSymbolAPIs ?

nswamy · 2018-05-01T22:26:36Z

  }
-  // scalastyle:off havetype
+  def addNewDefs(c: blackbox.Context)(annottees: c.Expr[Any]*) = {
+    impl(c)(false, true, annottees: _*)


Please create a new method for generating the new APIs

I think we should keep using impl to avoid duplicated code, since the new API implementation is just a small component in

nswamy · 2018-05-01T23:52:23Z

+        argDef += "attr : Map[String, String] = null"
+        // Construct Implementation field
+        var impl = ListBuffer[String]()
+        impl += "val map = scala.collection.mutable.Map[String, Any]()"


Can you not do this in the above loop

nswamy · 2018-05-01T23:54:39Z

+  private def typeConversion(in : String, argType : String = "") : String = {
+    in match {
+      case "Shape(tuple)" | "ShapeorNone" => "org.apache.mxnet.Shape"
+      case "Symbol" | "NDArray" | "NDArray-or-Symbol" => "org.apache.mxnet.Symbol"


Should NDArray also return Symbol?

Yes, they share the same Function name

nswamy · 2018-05-01T23:55:39Z

+      case "double" => "Double"
+      case "string" => "String"
+      case "boolean" => "Boolean"
+      case "tupleof<float>" => "Any"


Why Any? Any will remove type checks

nswamy · 2018-05-02T00:10:42Z

    val opNames = ListBuffer.empty[String]
    _LIB.mxListAllOpNames(opNames)
-    opNames.map(opName => {
+    opNames.filter(op => !op.startsWith("_") || op.startsWith("_contrib_")).map(opName => {


We are filtering all operators that start with _ unfortunately the sparse, linear algebra and other operators also start with _. Look at this https://github.com/apache/incubator-mxnet/blob/4fb5241b47c8147690fd6408b55cb694d544656e/python/mxnet/base.py#L455 We need to revisit this again

Sure, let's add a TODO in here to make sure adding more functions.

nswamy · 2018-05-02T00:18:55Z

+
+
+
+  private def argumentCleaner(argType : String) : (String, Boolean) = {


Please add comments of what this method is trying to achieve. Also, please add how the structure of the API looks like when extracted from C++ and at the end when you are done cleaning up.

lanking520 · 2018-05-02T23:17:05Z

Now is turn out to be fun. I removed the underscore filter and adding support for underscore function generation. Please kindly review the code parser section and find if there are possible ways to convert some "Any"s to actual Scala types

lanking520 · 2018-05-07T22:51:44Z

In the latest commit, unit test were added for testing Scala API. In order to help init-native/base find the correct path to import the library, a environment variable called BASEDIR were used to help user customize the base directory they need. Unit test specifically focus on the Argument cleaner to make sure it can correctly handle different argument type descriptions

nswamy · 2018-05-08T18:13:32Z

+    var baseDir = System.getProperty("user.dir") + "/init-native"
+    if (System.getenv().containsKey("BASEDIR")) {
+      baseDir = sys.env("BASEDIR")
+    }


what are you expecting BASEDIR variable to be?

I think you should have a else
else { baseDir System.getProperty("user.dir") } baseDir = baseDir + "/init-native"

BASEDIR locate in the pom file (set as an environment variable) which determine the current base directory.

for system environment var, better to specify it is mxnet-related, e.g., MXNET_SCALA_MACRO_BASEDIR or something like it.

yzhliu · 2018-05-10T21:29:32Z

  }
-  // scalastyle:off havetype
+  def addNewDefs(c: blackbox.Context)(annottees: c.Expr[Any]*) = {
+    newAPIImpl(c)(annottees: _*)


can we have a more meaningful name, for example, typeSafeAPI?

yzhliu · 2018-05-10T21:44:54Z

+
+      // Construct argument field
+      var argDef = ListBuffer[String]()
+      symbolfunction.listOfArgs.foreach(symbolarg => {


can we combine this with the next foreach, i.e., line 120?

yzhliu · 2018-05-10T21:48:55Z

+    if (spaceRemoved.charAt(0)== '{') {
+      val endIdx = spaceRemoved.indexOf('}')
+      commaRemoved = spaceRemoved.substring(endIdx + 1).split(",")
+      commaRemoved(0) = "string"


yzhliu · 2018-05-10T22:12:48Z

+    // Optional Field
+    if (commaRemoved.length >= 3) {
+      // arg: Type, optional, default = Null
+      require(commaRemoved(1).equals("optional"))


just remind, better to use == in scala. == behaves the same as equals in Java, and equals in Scala behaves the same as == in Java... sigh... Since String is immutable, equals here is fine.

Understood, will note this.

yzhliu · 2018-05-10T22:14:42Z

+
+  override def afterAll(): Unit = {
+
+  }


can be removed?

yzhliu · 2018-05-10T22:15:39Z

+ */
+package org.apache.mxnet
+@AddSymbolAPIs(false)
+object SymbolAPI {


comment a bit for this placeholder.

yzhliu · 2018-05-10T22:19:16Z

  @throws(classOf[UnsatisfiedLinkError])
  private def tryLoadInitLibrary(): Unit = {
-    val baseDir = System.getProperty("user.dir") + "/init-native"
+    // val baseDir = System.getProperty("user.dir") + "/init-native"


remove this line.

yzhliu · 2018-05-10T22:20:43Z

+    var baseDir = System.getProperty("user.dir") + "/init-native"
+    if (System.getenv().containsKey("BASEDIR")) {
+      baseDir = sys.env("BASEDIR")
+    }


for system environment var, better to specify it is mxnet-related, e.g., MXNET_SCALA_MACRO_BASEDIR or something like it.

nswamy

minor comments, please address them. we can merge it.

nswamy · 2018-05-11T23:17:30Z

  private def tryLoadInitLibrary(): Unit = {
-    val baseDir = System.getProperty("user.dir") + "/init-native"
+    var baseDir = System.getProperty("user.dir") + "/init-native"
+    if (System.getenv().containsKey("MXNET_SCALA_MACRO_BASEDIR")) {


can we change this to MXNET_BASEDIR and we can append the macro dir location relative to it. I don't think the user needs to understand what macros are or find out macro dir.

nswamy · 2018-05-11T23:18:25Z

+        <artifactId>scalatest-maven-plugin</artifactId>
+        <configuration>
+          <environmentVariables>
+            <MXNET_SCALA_MACRO_BASEDIR>${project.parent.basedir}/init-native</MXNET_SCALA_MACRO_BASEDIR>


MXNET_BASEDIR

nswamy · 2018-05-11T23:33:05Z

-      if (isContrib) symbolFunctions.filter(_._1.startsWith("_contrib_"))
-      else symbolFunctions.filter(!_._1.startsWith("_contrib_"))
+      if (isContrib) symbolFunctions.filter(
+        func => func.name.startsWith("_contrib_") || !func.name.startsWith("_"))


Thinking of how this flag would be used, one possible use-case is we pull all the contrib APIs into a separate Object to make it more explicit as in ?
Symbol.api.foo --> Stable APIs
Symbol.contrib.bar--> Contrib APIs

We can do this as a separate PR, thoughts?

nswamy · 2018-05-11T23:39:52Z

-        if (isContrib) funcName.substring(funcName.indexOf("_contrib_") + "_contrib_".length())
-        else funcName
+
+    val functionDefs = newSymbolFunctions map { symbolfunction =>


I like this code way better than the old, but I think we introducing new way of generating code for old and new APIs, if there is any bug that we haven't caught it will break both old and new APIs. I think its a risk.

nswamy · 2018-05-11T23:43:03Z

+
+    val newSymbolFunctions = {
+      if (isContrib) symbolFunctions.filter(
+        func => func.name.startsWith("_contrib_") || !func.name.startsWith("_"))


same as above, thoughts? can be done as a separate PR

nswamy · 2018-05-12T00:00:05Z

+      argDef += "name : String = null"
+      argDef += "attr : Map[String, String] = null"
+      // scalastyle:off
+      impl += "org.apache.mxnet.Symbol.createSymbolGeneral(\"" + symbolfunction.name + "\", name, attr, Seq(), map.toMap)"


Can we make a note to Seq() when we deprecate old APIs

Add relative path to MXNET_BASEDIR

nswamy · 2018-05-12T01:09:50Z

-    val baseDir = System.getProperty("user.dir") + "/init-native"
+    var baseDir = System.getProperty("user.dir") + "/init-native"
+    if (System.getenv().containsKey("MXNET_BASEDIR")) {
+      baseDir = sys.env("MXNET_BASEDIR") + "/scala-package/init-native"


just FYI, I updated this line from
baseDir = sys.env("MXNET_BASEDIR")
to
baseDir = sys.env("MXNET_BASEDIR") + "/scala-package/init-native"

* Simplfied current Macros impl to Quasiquote * Change the Symbol Function Field, add SymbolArg * Fix the Macros problem, disable the hidden function _ * Add Implementation for New API * Add examples and comments * Add _contrib_ support * New namespace for Symbol API * Change names and add comments * add TODOs and name changes * Add relative path to MXNET_BASEDIR * Update Base.scala

Simplfied current Macros impl to Quasiquote

23082eb

lanking520 requested a review from yzhliu as a code owner April 23, 2018 22:43

lanking520 added 2 commits April 23, 2018 17:31

Change the Symbol Function Field, add SymbolArg

9b74fc9

Fix the Macros problem, disable the hidden function _

2485146

yzhliu suggested changes Apr 24, 2018

View reviewed changes

Add Implementation for New API

e61e13a

lanking520 added 2 commits April 25, 2018 15:47

Trigger CI

ff4e24e

Trigger CI

704be92

lanking520 changed the title ~~[MXNET-357][WIP] New Scala API Design~~ [MXNET-357] New Scala API Design (Symbol) Apr 26, 2018

lanking520 added 4 commits April 26, 2018 13:32

Add examples and comments

eae7bd6

Add _contrib_ support

8de007d

Resolve Style issues

51581e3

Add Depreciated to the old APIs

e370c70

yzhliu suggested changes May 1, 2018

View reviewed changes

lanking520 added 4 commits May 1, 2018 13:04

New namespace for Symbol API

6d7d4bc

Add require tracker to check the input format

c1cbf16

Trigger CI

0d34b94

Trigger CI

4268c7e

nswamy suggested changes May 2, 2018

View reviewed changes

lanking520 added 2 commits May 2, 2018 14:08

Change names and add comments

b6fe942

Add underscore support

f3b1d96

lanking520 added 4 commits May 2, 2018 16:32

Disable underscore function from generation

5c05c8d

Trigger CI

006d3b5

use different impl method for new API

0dd3ee1

add unit test to Macros

80753d6

retrigger CI

57984a2

nswamy reviewed May 8, 2018

View reviewed changes

reTrigger CI

e836480

yzhliu suggested changes May 10, 2018

View reviewed changes

Name and minor fixes

788ddde

lanking520 mentioned this pull request May 11, 2018

[MXNET-357] New Scala API Design (NDArray) #10787

Merged

9 tasks

yzhliu approved these changes May 11, 2018

View reviewed changes

nswamy approved these changes May 12, 2018

View reviewed changes

lanking520 and others added 2 commits May 11, 2018 17:15

add TODOs and name changes

ba926fc

Update Base.scala

0e8a174

Add relative path to MXNET_BASEDIR

nswamy reviewed May 12, 2018

View reviewed changes

Update Base.scala

c194066

nswamy merged commit b011ecc into apache:master May 14, 2018

lanking520 deleted the scala-macros-impl branch May 18, 2018 22:47




		private def argumentCleaner(argType : String) : (String, Boolean) = {

Conversation

lanking520 commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

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!

lanking520 Apr 25, 2018 • 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!

lanking520 commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanking520 May 1, 2018 • 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!

lanking520 May 1, 2018 • 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!

nswamy 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

lanking520 commented Apr 23, 2018 •

edited

Loading

lanking520 Apr 25, 2018 •

edited

Loading

lanking520 commented Apr 25, 2018 •

edited

Loading

lanking520 May 1, 2018 •

edited

Loading

lanking520 May 1, 2018 •

edited

Loading