-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
ping @lanking520 |
@mxnet-label-bot [ pr-awaiting-review, Scala ] |
@mdespriee Thanks for the attempt to fix CI errors but it seems it is still failing. Request you to take a look. |
af2d29a
to
704ab60
Compare
by the way, the full generated code is readable here: |
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:
|
@lanking520 Ok, there has been some confusion. I'll try. |
Sorry, it was my bad. Let's keep Symbol there. Ask some expert in here to see their opinions about make Random Symbol: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
@@ -39,6 +39,7 @@ object NDArray extends NDArrayBase { | |||
private val functions: Map[String, NDArrayFunction] = initNDArrayModule() | |||
|
|||
val api = NDArrayAPI | |||
val random = NDArrayRandomAPI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider moving this part to random.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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also this part go to random.scala too...
@mxnet-label-bot update [pr-awaiting-response, scala] |
@lanking520 you mean putting both within Random object ? How to disambiguate NDArray vs Symbol ? |
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>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problems with generateAPIDocFromBackend were addressed elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@mdespriee could you please trigger the test again? |
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. |
That's strange. It looks like the code-gen doesn't work somehow on Linux |
Actually the code gen fails this way when the |
@lanking520 It seems that the following function (randint) is not returned by
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 ? |
@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. |
@apeforest @samskalicky Could you please help to locate this op by any chances in C++? |
@ChaiBapchya wrote the randint operator |
@lanking520 Specifically, Operator definition can be found here - RandInt kernel function definition - CPU and GPU specific code can be found in files - |
@ChaiBapchya what is the data type of |
It supports int32 and int64 (default to int32) |
@lanking520 I reviewed my |
@mdespriee For sure! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and looks good!
@mdespriee one last quest, could you please add the generated file into |
NVM now, I will do it in my next PR. @ChaiBapchya Please find the information above and see how you can fix |
* 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
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
, andNDArrayOrValue
. See https://github.com/apache/incubator-mxnet/blob/704ab6030c9803ac70d2726618885cef323a4420/scala-package/core/src/main/scala/org/apache/mxnet/Base.scala#L158-L171This allow this kind of calls:
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
vsrandom_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.
Changes
Comments