mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From YiZhi Liu <eazhi....@gmail.com>
Subject Re: Feedback request for new Java API
Date Sun, 30 Sep 2018 02:10:13 GMT
It also makes sense to me if we have it under namespace NDArray, not
creating new JavaNDArray. But again, uniform experience is important.

What I responded is your comment "keep scala macros minimum", I don't
think "scala macro" equals "cryptic code". Even though it does, what
we need to do is to find an alternative way to do code generation, not
making code generation minimum.

Since you agree to have 30+ operators have Builder, what prevents from
having all of them have Builder?
- They're auto-generated, the auto-generation "cryptic" code is anyway
there. And "two different paths of code" (though I don't totally
agree) is anyway there.
- What else? 200+ classes is a very tiny increasing in file size
(~3MB) compare to current status. And won't have any performance issue
on modern JVM.

Just remind, technical discussion is not about who gives who a lecture.
On Sat, Sep 29, 2018 at 6:41 PM Naveen Swamy <mnnaveen@gmail.com> wrote:
>
> Well, I am not sure(I don't think) we need Builder for every API in
> NDArray. For APIs that take long list of parameters, I agree to add Builder.
> Look at the API distribution based on number of arguments here:
> https://gist.github.com/nswamy/2dea72e514cc7bfc675f68aef9fe78bb
> about 30 APIs have 7 or more arguments.. I agree to add Builders for these
> APIs not separately but to the existing Scala APIs but not separately only
> for Java.
> APIs sorted by number of arguments is here, take a look :
> https://gist.github.com/nswamy/e941cb94658b3960eec40bf00b970ac5
>
> Many of the arguments i think are actually mandatory but incorrectly
> declared optional on the backend, for example look at SwapAxis
> "def SwapAxis (data : NDArray, dim1 : Option[Int] = None, dim2 :
> Option[Int] = None, out : Option[NDArray] = None) : NDArrayFuncReturn"
> Why is dim1 and dim2 Optional, this is an error in the declaration on the
> backend, I think there might be many of these?
>
> My answers to your other responses are below inline:
>
> On Sat, Sep 29, 2018 at 3:37 PM YiZhi Liu <eazhi.liu@gmail.com> wrote:
>
> > Some of my comments inline:
> >
> > > Why can we not create the builder just for these APIs( which we
> > discussed), why is it necessary to add 200 Apis
> > It is about unified user-experience. And we get rid of annoying extra
> > "out=null" in every operator.
> >
> > > Are you suggesting to create builder for each and every API?
> > Only for those are necessary. For NDArray.XXX, yes.
> >
> I think this is a ridiculous list of Builders, I think we can keep the
> 'out' parameter
>
> > 1) The NDArray APIs in question are not following functional style of
> > programming, in fact they are just static methods defined on an
> > NDArray object - so Scala users are not losing much by using null in
> > place of None.
> > You can create a implicit to maintain backward compatibility
> > - I doubt implicit can work in such case from None -> null.
> >
>
> It is just writing getOrElse in your implicit, so it will work.
> scala> implicit def optionStringToString(a: Option[String]): String = {
>      | a.getOrElse(null)
>      | }
>
> 2) It is adding 220+ APIs(I understand it is generated) for NDArray alone
> > - As I explained how it can improve user experiences
> >
> I don't think we need to write builders for 221 APIs we have, may be for 30
> or so. Uniform experience is good goal but it also has to be practical and
> make sense.
>
> 3) this is adding another 100s of APIs unnecessarily, we are starting with
> > NDArray but we can't stop there, we will have to do this for Symbol,
> > Executor, Iterators, etc., .
> > - This is a good point, actually I prefer not to make JavaExecutor,
> > JavaIterators
> >
> What I was aiming is also users have the same experience across Interfaces
> - now you are forgoing uniform experience, so like you said its all
> trade-off and a good trade-off doesn't cause too much overhead/
>
>
> > 4) I don't want to be fixing bugs and maintaining code in 2 places.
> > - Type-safe parsing is shared. I think Qing is more qualified to comment.
>
> It creates two different paths of code for Scala and Java - how is it going
> to be shared. I am afraid we are going to make it more complicated than
> necessary by duplicating code.
>
> >
>
> 5) I want the cryptic code(# scala macros) to a minimum.
> > - MXNet decides to do operator generation in frontend bindings. It's
> > the developers' responsibility to understand the techniques they are
> > using. Maybe not a so proper analogy - "I don't know RL / RL is hard
> > to tune / ..." is not a reason for "I want to keep RL implementation
> > in MXNet as a small part as possible"
> >
> > Now, this is a response I don't like. I don't know where you were going
> with your analogy but know that it sounds condescending - I am going to
> ignore(assuming good intentions) that and explain what I mean.
> I here is I the developer/user who deploys MXNet code in production and
> have to deal with the aftermath myself not you(MXNet developers).
> From your comment it occurs to me you probably have never been on
> pager-duty. I have been on pager-duty both for the code I wrote and those
> that was written by others and thrown over the fence.
> If you get woken up by a beep at the middle of the night, that is not the
> time to prove your intelligence. Its time to mitigate the issue asap for
> that your code needs to be easy to follow, should follow well defined
> patterns, etc., -- i don't need to give you a lecture.
> IMHO It is extremely important for frameworks like Apache MXNet which are
> used by others for their production to keep code simple and *cryptic code
> to a minimum* and yes you(we - MXNet developers) are not answering the
> beepers when your(MXNet) users deploy their code in their production so
> make their life simple.
>
> 6) increased compilation time & bad developer experience - the time to
> > compile has gone up quite a bit since we added the APIs last release on my
> > 3 year old laptop already.. I think adding 400+ APIs unnecessarily would
> > significantly increase build time and bad developer experience
> > - I don't think increasing such a bit compilation time is a problem
> > compared to bad user experience.
>
> I am not suggesting bad user experience but to take a practical approach -
> having a bad developer experience is not great either.
>
> >
> >
> 7) I want to keep the core of the framework to be in Scala - because it
> > allows you to write concise code - Yes it has a bit of learning curve, not
> > everyone needs to know. I would rather invest in solidifying the Scala APIs
> > and add more features in Scala(RNN, Support GluonHybridizedBlock...there is
> > quite bit of work ) - do you want to rewrite everything in Scala and Java.
> > - I agree with "don't rewrite everything in Scala and Java", IMO
> > JavaNDArray is the only one good to have. JShape, JContext, etc. are
> > not so necessary.
> >
> > Either you go all Java or make accommodation in Scala code to work for
> APIs so your users know what to expect(uniform experience across).
>
> > 8) Also, the discussion is not creating NDArray class for Java, just
> > generate certain APIs to cater for Java incompatibility.
> > - Yes I agree it's about "generate certain APIs to cater for Java
> > incompatibility", though I think NDArray.api.XXX does not meet Java
> > users' demands.
> >
> On Sat, Sep 29, 2018 at 12:05 PM Naveen Swamy <mnnaveen@gmail.com> wrote:
> > >
> > > I know it is about trade-off.  I am suggesting a trade-off , how many
> > apis do we have that takes too many parameters ?
> > > From what I recall its around 20. Why can we not create the builder just
> > for these APIs( which we discussed), why is it necessary to add 200 Apis ?
> > > Are you suggesting to create builder for each and every API?
> > >
> > > I disagree with your opinion that they are not important and would like
> > to hear from others.
> > >
> > > I am curious to see how the #2 looks like compared to #1
> > > Andrew/Qing, can you paste the generated Apis that you have for both
> > Scala and Java in a gist please.
> > >
> > > > On Sep 29, 2018, at 2:41 PM, YiZhi Liu <eazhi.liu@gmail.com> wrote:
> > > >
> > > > Naveen, software designing is all about tradeoff, every feature we
> > > > introduce causes more compiling time, more efforts to maintain, etc.
> > > >
> > > > The main difference is.
> > > >
> > > > Option #1: Java users do
> > > > NDArray.BatchNorm(data, gamma, beta, null, null, null, null, null,
> > > > null, null, null, null, null, null);
> > > > (and because every operator has an argument "out", users need to add
> > > > an extra "null" to the function call almost every time.)
> > > >
> > > > Option #2, Java users do
> > > > JavaNDArray.BatchNorm(data).setGamma(gamma).setBeta(beta).invoke();
> > > >
> > > > I don't think any of the reasons you listed is so important as the
> > > > benefit above we got from option #2.
> > > >> On Sat, Sep 29, 2018 at 8:24 AM Naveen Swamy <mnnaveen@gmail.com>
> > wrote:
> > > >>
> > > >> Java APIs are not like Clojure - The current proposal is only to
> > build a
> > > >> few thin wrappers for Inference.
> > > >>
> > > >> To better represent the two cases and this discussion in particular,
> > here
> > > >> is an example API
> > > >>
> > > >> 1) def Activation (data : org.apache.mxnet.NDArray, act_type :
> > String, out
> > > >> : Option[NDArray] = None) : org.apache.mxnet.NDArrayFuncReturn
> > > >> or
> > > >> 2) def Activation (data : org.apache.mxnet.NDArray, act_type :
> > String, out
> > > >> : NDArray) : org.apache.mxnet.NDArrayFuncReturn
> > > >>
> > > >> The discussion is should we add(generate) 200+ APIs to make it Java
> > > >> compatible, ie., remove the Option class and the None default value
> > which
> > > >> Java does not understand from Option 1)
> > > >>
> > > >> my suggestion was to remove the Option class and create a implicit
for
> > > >> backward compatibility and use null instead of None, Andrew and I
> > disagreed
> > > >> on this, so I suggested to raise a discussion on dev@ to get more
> > opinions
> > > >> and one of us will disagree and commit. Thanks for raising it :)
> > > >>
> > > >> | * def Activation (data : org.apache.mxnet.NDArray, act_type :
> > String, out
> > > >> : NDArray = null) : org.apache.mxnet.NDArrayFuncReturn |
> > > >> --
> > > >>
> > > >> 1) It is not true that Scala users will lose *default/optional*
> > arguments -
> > > >> if we followed the above, they will use null or None, though I do
not
> > like
> > > >> using nulls, this is a fine compromise.
> > > >> To keep backward compatibility we can create a implicit to convert
> > > >> Option.None to nulls and Option.Some-> Option.get(), so you are
not
> > going
> > > >> to break users who might have been using the APIs that were released
> > in
> > > >> 1.3. The current incompatibility is only this w.r.t. NDArrays.
> > > >>
> > > >> 2) Now about the Scala Macros - they are not simple to read or use,
> > When I
> > > >> and Qing started working on the #Scala Macros to improve the APIs,
it
> > took
> > > >> us a good amount of time to get a hang of it. I don't want to add
> > > >> additional code when not necessary.
> > > >>
> > > >> My suggestion and vote is to modify existing Macro(i.e., #1 from the
> > > >> original email with the necessary clarification above) and make it
> > > >> compatible with Java
> > > >> Here are my reasons
> > > >> 1) The NDArray APIs in question are not following functional style
of
> > > >> programming, in fact they are just static methods defined on an
> > NDArray
> > > >> object - so Scala users are not losing much by using null in place
of
> > None.
> > > >> You can create a implicit to maintain backward compatibility
> > > >> 2) It is adding 220+ APIs(I understand it is generated) for NDArray
> > alone
> > > >> 3) this is adding another 100s of APIs unnecessarily, we are starting
> > with
> > > >> NDArray but we can't stop there, we will have to do this for Symbol,
> > > >> Executor, Iterators, etc., .
> > > >> 3) I don't want to be fixing bugs and maintaining code in 2 places.
> > > >> 4) I want the cryptic code(# scala macros) to a minimum.
> > > >> 5) increased compilation time & bad developer experience - the
time to
> > > >> compile has gone up quite a bit since we added the APIs last release
> > on my
> > > >> 3 year old laptop already.. I think adding 400+ APIs unnecessarily
> > would
> > > >> significantly increase build time and bad developer experience
> > > >> 6) I want to keep the core of the framework to be in Scala - because
> > it
> > > >> allows you to write concise code - Yes it has a bit of learning
> > curve, not
> > > >> everyone needs to know. I would rather invest in solidifying the
> > Scala APIs
> > > >> and add more features in Scala(RNN, Support
> > GluonHybridizedBlock...there is
> > > >> quite bit of work ) - do you want to rewrite everything in Scala and
> > Java.
> > > >> 7) Also, the discussion is not creating NDArray class for Java, just
> > > >> generate certain APIs to cater for Java incompatibility.
> > > >>
> > > >> @Andrew: To your response to Qing's comments - you cannot just
> > consider it
> > > >> as just generating NDArray's APIs and instead I suggest to take a
> > wholistic
> > > >> view of all the various implications.
> > > >>
> > > >> @Chris: Yes, Scala has a bit of learning curve - the goal is not
> > having
> > > >> every developer to deal with how these APIs are generated,
> > > >> the problem exists either ways with the above proposal. I might agree
> > if we
> > > >> were to move away completely(with a thorough discussion and valid
> > reasons)
> > > >> and instead use AspectJ or similar to write these APIs, the
> > discussion is
> > > >> about using Scala Macros to generate 2 different types of APIs which
> > are
> > > >> functionally not different and usability wise are very very similar,
> > look
> > > >> at the example.
> > > >> Thanks for your input, I will deposit your 0.02$ in our JIRA bank
:)
> > > >>
> > > >> @Carin: It requires more effort to use AspectJ or similar to generate
> > APIs
> > > >> using reflection or at compile time, here we need to generate at
> > compile
> > > >> time so Java users have the API signature on their IDEs.
> > > >>
> > > >> Thanks, Naveen
> > > >>
> > > >> P.S: I am traveling and my responses will be delayed.
> > > >>
> > > >>
> > > >>> On Fri, Sep 28, 2018 at 10:25 AM Carin Meier <carinmeier@gmail.com>
> > wrote:
> > > >>>
> > > >>> Sorry bad paste on the gist - here is the good one
> > > >>> https://gist.github.com/gigasquid/01cd48f563db4739910592dd9ac9db20
> > > >>>
> > > >>>> On Fri, Sep 28, 2018 at 10:24 AM Carin Meier <carinmeier@gmail.com>
> > wrote:
> > > >>>>
> > > >>>> +1 on option #2
> > > >>>>
> > > >>>> In the case of minimizing the the overhead for code maintenance,
I
> > wanted
> > > >>>> to suggest the option of investigating generating code from
the Java
> > > >>>> Reflection for the Java APIs.  I did a quick gist from Clojure
of
> > what
> > > >>> the
> > > >>>> generated classes look like from the current Scala Symbol.api
for
> > > >>>> FullyConnected here
> > > >>>> https://gist.github.com/gigasquid/01cd48f563db4739910592
> > > >>>>
> > > >>>> I looks like that there is always a base Java class generated
will
> > all
> > > >>> the
> > > >>>> arguments. If this is the case, then there is a possibility
to
> > generate a
> > > >>>> Java api based on this Java method automatically with just
a
> > conversion
> > > >>> for
> > > >>>> the Scala option and it might be reusable for all the packages.
> > > >>>>
> > > >>>> Not sure if it will work for this use case, but thought I
would
> > bring it
> > > >>>> up in case it's helpful.
> > > >>>>
> > > >>>> - Carin
> > > >>>>
> > > >>>> On Fri, Sep 28, 2018 at 7:05 AM Davydenko, Denis
> > <dden@amazon.com.invalid
> > > >>>>
> > > >>>> wrote:
> > > >>>>
> > > >>>>> +1 on option #2. Having clear Java interface for NDArray,
from my
> > > >>>>> perspective, would be a better experience for Java users
as it
> > won't
> > > >>>>> require them to deal with Scala code in any capacity.
Overhead of
> > extra
> > > >>>>> code for additional macros is justified, in my mind, as
it will be
> > > >>>>> introduced with option #1 either way, just in a different
place.
> > > >>>>>
> > > >>>>> --
> > > >>>>> Thanks,
> > > >>>>> Denis
> > > >>>>>
> > > >>>>> On 9/27/18, 6:14 PM, "YiZhi Liu" <eazhi.liu@gmail.com>
wrote:
> > > >>>>>
> > > >>>>>  I vote for "2.) Leave the existing macro in place and
add another
> > > >>>>>  which generates a Java friendly version"
> > > >>>>>
> > > >>>>>  @Qing @Andrew, could you give some examples, so that
people can
> > > >>> better
> > > >>>>>  understand how it provides "best possible experience"
to Java
> > users.
> > > >>>>>
> > > >>>>>  I have no strong preference between having JavaShape
& JavaContext
> > > >>> or
> > > >>>>> not.
> > > >>>>>  On Thu, Sep 27, 2018 at 5:56 PM Andrew Ayres <
> > > >>>>> andrew.f.ayres@gmail.com> wrote:
> > > >>>>>>
> > > >>>>>> That's not really the conversation I'm wanting to
have. I want a
> > > >>>>> discussion
> > > >>>>>> about the macros with respect to NDArray so that we
can get
> > > >>>>> agreement on
> > > >>>>>> our path forward with respect to implementing the
NDArray wrapper.
> > > >>>>>>
> > > >>>>>> The design that was put forth and agreed to was for
a a Java
> > > >>>>> wrapper around
> > > >>>>>> the Scala API. Adding a bunch of Java friendly methods
inside the
> > > >>>>> Scala
> > > >>>>>> code would create a mess for users. Maintenance would
be
> > > >>>>> essentially the
> > > >>>>>> same for both because either way you're going to be
updating Java
> > > >>>>> methods
> > > >>>>>> when you make Scala changes.
> > > >>>>>>
> > > >>>>>> Let's please stick with the issue in the original
email.
> > > >>>>>>
> > > >>>>>> Thanks,
> > > >>>>>> Andrew
> > > >>>>>>
> > > >>>>>>> On Thu, Sep 27, 2018 at 5:22 PM Qing Lan <lanking520@live.com>
> > > >>>>>> wrote:
> > > >>>>>>
> > > >>>>>>> I would like to loop this back a layer. Current,
there is a
> > > >>>>> discussion in
> > > >>>>>>> the MXNet Scala community on the ways to implement
the Java
> > > >>> APIs.
> > > >>>>> Currently
> > > >>>>>>> there are two thoughts:
> > > >>>>>>>
> > > >>>>>>> 1. Make Scala Java Friendly (Create Java compatible
methods in
> > > >>>>> the Scala
> > > >>>>>>> Class. such as NDArray with Java compatible constructor)
> > > >>>>>>> 2. Make Java friendly wrappers in Scala (Andrew's
explanation
> > > >>>>> below)
> > > >>>>>>>
> > > >>>>>>> The first approach require minimum input from
our side to
> > > >>>>> implement
> > > >>>>>>> however bring user a bunch of useless api they
may not want to
> > > >>>>> use. It also
> > > >>>>>>> makes Scala package heavier. The good thing is
these two
> > > >>> packages
> > > >>>>> require
> > > >>>>>>> minimum maintenance cost. As a tradeoff, if any
time in the
> > > >>>>> future we want
> > > >>>>>>> to make Java big (make Java as the primary language
supported by
> > > >>>>> MXNet),
> > > >>>>>>> then the migration from Scala to Java will be
harmful. Spark
> > > >>>>> consider this
> > > >>>>>>> carefully and decide not to change much on their
Scala code base
> > > >>>>> to make it
> > > >>>>>>> more Java.
> > > >>>>>>>
> > > >>>>>>> The second approach will make unique NDArray,
Shape, Context and
> > > >>>>> more. The
> > > >>>>>>> good thing about this is we can always holds a
version control
> > > >>> on
> > > >>>>> Java.
> > > >>>>>>> Some breaking changes on Scala may not influence
much on Java.
> > > >>> It
> > > >>>>> did the
> > > >>>>>>> best way to decouple the module and good for us
to build unique
> > > >>>>> pipeline
> > > >>>>>>> for Java. The bad thing with this design is the
maintenance cost
> > > >>>>> as we need
> > > >>>>>>> to keep two code bases, but it also make Java
side easy to
> > > >>> change
> > > >>>>> to make
> > > >>>>>>> it better compatible with users.
> > > >>>>>>>
> > > >>>>>>> Thanks,
> > > >>>>>>> Qing
> > > >>>>>>>
> > > >>>>>>> On 9/27/18, 3:25 PM, "Andrew Ayres" <andrew.f.ayres@gmail.com>
> > > >>>>> wrote:
> > > >>>>>>>
> > > >>>>>>>  Hi,
> > > >>>>>>>
> > > >>>>>>>  Currently, we're working to implement a new Java
API and
> > > >>>>> would like
> > > >>>>>>> some
> > > >>>>>>>  feedback from the community on an implementation
detail. In
> > > >>>>> short, the
> > > >>>>>>> new
> > > >>>>>>>  Java API will use the existing Scala API (in
a manner
> > > >>> similar
> > > >>>>> to how
> > > >>>>>>> the
> > > >>>>>>>  current Clojure API works). This basically means
that we're
> > > >>>>> making Java
> > > >>>>>>>  friendly wrappers to call the existing Scala
API.
> > > >>>>>>>
> > > >>>>>>>  The feedback we're looking for is on the implementation
of
> > > >>>>> NDArray.
> > > >>>>>>> Scala's
> > > >>>>>>>  NDArray has a significant amount of code which
is generated
> > > >>>>> via macros
> > > >>>>>>> and
> > > >>>>>>>  we've got two viable paths to move forward:
> > > >>>>>>>
> > > >>>>>>>  1.) Change the macro to generate Java friendly
methods  - To
> > > >>>>> do this
> > > >>>>>>> we'll
> > > >>>>>>>  modify the macro so that the generated methods
won't have
> > > >>>>>>> default/optional
> > > >>>>>>>  arguments. There may also have to be some changes
to
> > > >>>>> parameter types to
> > > >>>>>>>  make them Java friendly. The big advantage here
is that
> > > >>>>> ongoing
> > > >>>>>>> maintenance
> > > >>>>>>>  will easier. The disadvantages are that we'll
be changing
> > > >>> the
> > > >>>>> existing
> > > >>>>>>>  Scala NDArray Infer API (it's marked experimental)
and Scala
> > > >>>>> users will
> > > >>>>>>>  lose the ability to use the default and optional
arguments.
> > > >>>>>>>
> > > >>>>>>>  2.) Leave the existing macro in place and add
another which
> > > >>>>> generates a
> > > >>>>>>>  Java friendly version - The biggest issue here
is that we'll
> > > >>>>> be
> > > >>>>>>> doubling
> > > >>>>>>>  the number of macros that we've got to maintain.
It'll
> > > >>> become
> > > >>>>> even more
> > > >>>>>>>  overhead once we start expanding the Java API
with more
> > > >>>>> classes that
> > > >>>>>>> use
> > > >>>>>>>  generated code like this. The advantages are
that the
> > > >>>>> existing Scala
> > > >>>>>>>  NDArray Infer API would remain unchanged for
Scala users and
> > > >>>>> that the
> > > >>>>>>> new
> > > >>>>>>>  macro could be optimized to give the best possible
> > > >>> experience
> > > >>>>> to the
> > > >>>>>>> Java
> > > >>>>>>>  API.
> > > >>>>>>>
> > > >>>>>>>  Thanks,
> > > >>>>>>>  Andrew
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>  --
> > > >>>>>  Yizhi Liu
> > > >>>>>  DMLC member
> > > >>>>>  Amazon Web Services
> > > >>>>>  Vancouver, Canada
> > > >
> > > >
> > > >
> > > > --
> > > > Yizhi Liu
> > > > DMLC member
> > > > Amazon Web Services
> > > > Vancouver, Canada
> >
> >
> >
> > --
> > Yizhi Liu
> > DMLC member
> > Amazon Web Services
> > Vancouver, Canada
> >
> >



-- 
Yizhi Liu
DMLC member
Amazon Web Services
Vancouver, Canada

Mime
View raw message