[MXNET-918] Random module #13039

mdespriee · 2018-10-30T14:39:52Z

Description

This PR introduces Symbol.random and NDArray.random modules in scala API.
This work is a further refactoring of code generation + introduction of Random module.

Functions in random module are generic and accept the following "union types": SymbolOrValue, and NDArrayOrValue. See /~https://github.com/apache/incubator-mxnet/blob/704ab6030c9803ac70d2726618885cef323a4420/scala-package/core/src/main/scala/org/apache/mxnet/Base.scala#L158-L171

This allow this kind of calls:

    // no need to import anything special, or any implicit
    val lam = Symbol.Variable("lam")
    val rnd = Symbol.random.poisson(lam = Some(lam), shape = Some(Shape(2, 2)))
    val rnd2 = Symbol.random.poisson(lam = Some(1f), shape = Some(Shape(2, 2)))

For multi argument functions, mixing types of params is not allowed (fails at compile time).

The code to generate Random module is more hacky than the rest, due to some discrepancies (eg. sample_normal vs random_normal). I tried to isolate that as much as possible in dedicated objects and mixin.

Following @zachgk comment in the previous PR, I also tried to organise code generation in a way that the essential of function implementations is as much as possible readable and linear.

Last thing to discuss is: should we turn off function generation of random stuff in Symbol and NDArray
(eg Symbol.sample_exponential)

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

Symbol.random api generated by macros
NDArray.random api generated by macros

Comments

This is a continuation of work started in PR [WIP][MXNET-918] Random api #12489
Related: [MXNET-918] Introduce Random module / Refact code generation #13038

mdespriee · 2018-10-30T14:40:33Z

ping @lanking520

ankkhedia · 2018-10-30T17:15:00Z

@mxnet-label-bot [ pr-awaiting-review, Scala ]
@mdespriee Thanks for your contribution!
Could you please look into CI failures for the PR

ankkhedia · 2018-10-30T20:40:13Z

@mdespriee Thanks for the attempt to fix CI errors but it seems it is still failing. Request you to take a look.

mdespriee · 2018-11-05T21:36:08Z

by the way, the full generated code is readable here:
https://gist.github.com/mdespriee/4b170ebf4dd52df90f30579e3aac510b
https://gist.github.com/mdespriee/ced69d2b38b6aec8f192e0ec325f8e7e

lanking520 · 2018-11-05T22:15:22Z

Hi @mdespriee, thanks for your rebase on the code and glad to see you bring a better solution to the existing problems. Here are two recommendation I can think of:

Is that necessary to bring this new API to Symbol? If users would like to use the Symbolic graph, they can call the existing random and Sample function to work and the data passing between should be strict as Symbols. So my ways of thinking is that these dynamic types should not be in the "everything is fixed" world.
Do you think if that possible to extend the Existing Random package instead of creating new stuff in the Symbol API?

mdespriee · 2018-11-05T22:28:20Z

@lanking520 Ok, there has been some confusion. I'll try.
(from our past conversations, didn't we talk about Symbol ?? #12489 (comment)...)

lanking520 · 2018-11-06T00:16:12Z

@lanking520 Ok, there has been some confusion. I'll try.
(from our past conversations, didn't we talk about Symbol ?? #12489 (comment)...)

Sorry, it was my bad. Let's keep Symbol there.

Ask some expert in here to see their opinions about make Random Symbol:
@nswamy @yzhliu What do you guys think?

lanking520

Some general thinking: do you think it would be better to move the functions that specifically used for random to somewhere else? Consider there would be more extensions in the future...

lanking520 · 2018-11-10T01:51:33Z

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

@@ -39,6 +39,7 @@ object NDArray extends NDArrayBase {
  private val functions: Map[String, NDArrayFunction] = initNDArrayModule()

  val api = NDArrayAPI
+  val random = NDArrayRandomAPI


Can we consider moving this part to random.scala?

lanking520 · 2018-11-10T01:52:22Z

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

@@ -841,6 +841,7 @@ object Symbol extends SymbolBase {
  private val bindReqMap = Map("null" -> 0, "write" -> 1, "add" -> 3)

  val api = SymbolAPI
+  val random = SymbolRandomAPI


Maybe also this part go to random.scala too...

stu1130 · 2018-11-20T22:23:16Z

@mxnet-label-bot update [pr-awaiting-response, scala]
@mdespriee could you please address the comments from @lanking520? feel free to ask the question if you need any help!

mdespriee · 2018-11-27T16:06:11Z

@lanking520 you mean putting both within Random object ? How to disambiguate NDArray vs Symbol ?

zachgk · 2018-11-29T03:14:11Z

scala-package/macros/src/main/scala/org/apache/mxnet/APIDocGenerator.scala

      absFuncs)
  }

  def generateAPIDocFromBackend(func: Func, withParam: Boolean = true): String = {
    val desc = func.desc.split("\n")
-      .mkString("  * <pre>\n", "\n  * ", "  * </pre>\n")
+      .mkString("  * <pre>", "\n  * ", "\n  * </pre>")


The problems with generateAPIDocFromBackend were addressed elsewhere

zachgk

One thing was fixed elsewhere. Otherwise LGTM

# Conflicts: # scala-package/macros/src/main/scala/org/apache/mxnet/APIDocGenerator.scala # scala-package/macros/src/main/scala/org/apache/mxnet/GeneratorBase.scala

lanking520 · 2018-12-05T07:03:12Z

@mdespriee could you please trigger the test again?

mdespriee · 2018-12-05T15:23:27Z

I'm a bit confused with these failing tests, as I can't reproduce the failure locally. I also don't get why it is environment dependant. I'll have a look asap.

lanking520 · 2018-12-05T17:43:18Z

That's strange. It looks like the code-gen doesn't work somehow on Linux

mdespriee · 2018-12-05T21:23:21Z

That's strange. It looks like the code-gen doesn't work somehow on Linux

Actually the code gen fails this way when the getBackEndFunctions returns a function with no arg being either Symbol or Float (which are changed to generic T). I can write something more robust. But is it possible that the backend returns a different function list depending on the environment ?
(which would be a bug in a sense...)

mdespriee · 2018-12-11T12:30:09Z

@lanking520 It seems that the following function (randint) is not returned by LIB.nnGetOpHandle in all environments. Or not in the same way and get filtered out.

Func: randint, args=[Arg(low,Any,Lower bound of the distribution.,false),Arg(high,Any,Upper bound of the distribution.,false),Arg(shape,org.apache.mxnet.Shape,Shape of the output.,true),Arg(ctx,String,Context of output, in format [cpu|gpu|cpu_pinned](n). Only used for imperative calls.,true),Arg(dtype,String,DType of the output in case this can't be inferred. Defaults to int32 if not defined (dtype=None).,true)]

Locally, I don't get this func to be generated by macros. On CI, it is generated on 3 envs, but not on centos-gpu.

Any clue on what's going on ?

lanking520 · 2018-12-11T19:40:44Z

@mdespriee Thanks for debugging through, you seemed to hit a rock: /~https://github.com/apache/incubator-mxnet/blob/master/scala-package/macros/src/main/scala/org/apache/mxnet/utils/CToScalaUtils.scala#L38.
Some of the params cannot be intepreted

lanking520 · 2018-12-11T19:56:08Z

@apeforest @samskalicky Could you please help to locate this op by any chances in C++? randint

samskalicky · 2018-12-11T20:14:16Z

@ChaiBapchya wrote the randint operator

ChaiBapchya · 2018-12-11T22:53:53Z

@lanking520
For reference, here's the PR - #12749

Specifically, Operator definition can be found here -
/~https://github.com/apache/incubator-mxnet/blob/449e17dbf2ec671037d4b127a28897b157f80bf3/src/operator/random/sample_op.h#L257

RandInt kernel function definition -
/~https://github.com/apache/incubator-mxnet/blob/449e17dbf2ec671037d4b127a28897b157f80bf3/src/operator/random/sampler.h#L97

CPU and GPU specific code can be found in files -
/~https://github.com/apache/incubator-mxnet/blob/449e17dbf2ec671037d4b127a28897b157f80bf3/src/operator/random/sample_op.cc#L181

/~https://github.com/apache/incubator-mxnet/blob/449e17dbf2ec671037d4b127a28897b157f80bf3/src/operator/random/sample_op.cu#L42

lanking520 · 2018-12-11T22:56:56Z

@ChaiBapchya what is the data type of low and high? It seemed not intepreted correctly in Scala?
@mdespriee please try to pull --rebase upstream master and then make clean and build the backend all over again to see if you can reproduce this issue

ChaiBapchya · 2018-12-11T22:59:49Z

It supports int32 and int64 (default to int32)

mdespriee · 2018-12-12T16:40:50Z

@lanking520 I reviewed my SymbolOrScalar and NDarrayOrScalar, it's now a cleaner implem of typeclass pattern, it's more robust and not failing on edge cases.
We still have an issue with randint expecting Any instead of proper types, but it may be addressed in a separate PR. What do you think ?

lanking520 · 2018-12-13T19:15:33Z

@mdespriee For sure!

lanking520

Tested locally and looks good!

lanking520 · 2018-12-14T18:42:37Z

@mdespriee one last quest, could you please add the generated file into .gitignore?

lanking520 · 2018-12-14T22:21:58Z

NVM now, I will do it in my next PR. @ChaiBapchya Please find the information above and see how you can fix randint.

* introduce random API * revert useless changes * shorter types in APIDoc gen code * fix after merge from master * Trigger CI * temp code / diag on CI * cleanup type-class code * cleanup type-class code * fix scalastyle

mdespriee requested review from nswamy and yzhliu as code owners October 30, 2018 14:39

mdespriee mentioned this pull request Oct 30, 2018

[MXNET-918] Introduce Random module / Refact code generation #13038

Merged

5 tasks

mdespriee mentioned this pull request Oct 30, 2018

[WIP][MXNET-918] Random api #12489

Closed

5 tasks

marcoabreu added pr-awaiting-review PR is waiting for code review Scala labels Oct 30, 2018

introduce random API

704ab60

mdespriee force-pushed the random_module_2 branch from af2d29a to 704ab60 Compare November 5, 2018 21:14

revert useless changes

c6cb9ff

shorter types in APIDoc gen code

5fdcd62

lanking520 reviewed Nov 10, 2018

View reviewed changes

marcoabreu added pr-awaiting-response PR is reviewed and waiting for contributor to respond Scala and removed Scala pr-awaiting-review PR is waiting for code review labels Nov 20, 2018

zachgk reviewed Nov 29, 2018

View reviewed changes

zachgk approved these changes Nov 29, 2018

View reviewed changes

mdespriee added 2 commits November 29, 2018 14:57

Merge branch 'master' into random_module_2

0b8837e

# Conflicts: # scala-package/macros/src/main/scala/org/apache/mxnet/APIDocGenerator.scala # scala-package/macros/src/main/scala/org/apache/mxnet/GeneratorBase.scala

fix after merge from master

526f78c

Trigger CI

d9695c5

temp code / diag on CI

d4cb629

mdespriee added 4 commits December 12, 2018 08:41

cleanup type-class code

c6af793

Merge branch 'master' into random_module_2

ee66735

cleanup type-class code

1f7d0fe

fix scalastyle

a87c113

lanking520 approved these changes Dec 13, 2018

View reviewed changes

lanking520 merged commit fc5fce7 into apache:master Dec 14, 2018

zachgk mentioned this pull request Jan 12, 2019

Impossible to provide arguments to random_normal in scala ? #12140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-918] Random module #13039

[MXNET-918] Random module #13039

mdespriee commented Oct 30, 2018 •

edited

Loading

mdespriee commented Oct 30, 2018

ankkhedia commented Oct 30, 2018 •

edited

Loading

ankkhedia commented Oct 30, 2018

mdespriee commented Nov 5, 2018

lanking520 commented Nov 5, 2018

mdespriee commented Nov 5, 2018

lanking520 commented Nov 6, 2018 •

edited

Loading

lanking520 left a comment

lanking520 Nov 10, 2018

lanking520 Nov 10, 2018

stu1130 commented Nov 20, 2018 •

edited

Loading

mdespriee commented Nov 27, 2018

zachgk Nov 29, 2018

zachgk left a comment

lanking520 commented Dec 5, 2018

mdespriee commented Dec 5, 2018

lanking520 commented Dec 5, 2018

mdespriee commented Dec 5, 2018

mdespriee commented Dec 11, 2018

lanking520 commented Dec 11, 2018 •

edited

Loading

lanking520 commented Dec 11, 2018 •

edited

Loading

samskalicky commented Dec 11, 2018

ChaiBapchya commented Dec 11, 2018 •

edited

Loading

lanking520 commented Dec 11, 2018

ChaiBapchya commented Dec 11, 2018 •

edited

Loading

mdespriee commented Dec 12, 2018

lanking520 commented Dec 13, 2018

lanking520 left a comment

lanking520 commented Dec 14, 2018

lanking520 commented Dec 14, 2018

[MXNET-918] Random module #13039

[MXNET-918] Random module #13039

Conversation

mdespriee commented Oct 30, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

mdespriee commented Oct 30, 2018

ankkhedia commented Oct 30, 2018 • edited Loading

ankkhedia commented Oct 30, 2018

mdespriee commented Nov 5, 2018

lanking520 commented Nov 5, 2018

mdespriee commented Nov 5, 2018

lanking520 commented Nov 6, 2018 • edited Loading

lanking520 left a comment

Choose a reason for hiding this comment

lanking520 Nov 10, 2018

Choose a reason for hiding this comment

lanking520 Nov 10, 2018

Choose a reason for hiding this comment

stu1130 commented Nov 20, 2018 • edited Loading

mdespriee commented Nov 27, 2018

zachgk Nov 29, 2018

Choose a reason for hiding this comment

zachgk left a comment

Choose a reason for hiding this comment

lanking520 commented Dec 5, 2018

mdespriee commented Dec 5, 2018

lanking520 commented Dec 5, 2018

mdespriee commented Dec 5, 2018

mdespriee commented Dec 11, 2018

lanking520 commented Dec 11, 2018 • edited Loading

lanking520 commented Dec 11, 2018 • edited Loading

samskalicky commented Dec 11, 2018

ChaiBapchya commented Dec 11, 2018 • edited Loading

lanking520 commented Dec 11, 2018

ChaiBapchya commented Dec 11, 2018 • edited Loading

mdespriee commented Dec 12, 2018

lanking520 commented Dec 13, 2018

lanking520 left a comment

Choose a reason for hiding this comment

lanking520 commented Dec 14, 2018

lanking520 commented Dec 14, 2018

mdespriee commented Oct 30, 2018 •

edited

Loading

ankkhedia commented Oct 30, 2018 •

edited

Loading

lanking520 commented Nov 6, 2018 •

edited

Loading

stu1130 commented Nov 20, 2018 •

edited

Loading

lanking520 commented Dec 11, 2018 •

edited

Loading

lanking520 commented Dec 11, 2018 •

edited

Loading

ChaiBapchya commented Dec 11, 2018 •

edited

Loading

ChaiBapchya commented Dec 11, 2018 •

edited

Loading