RE: [Discuss] Next MXNet release
Thanks, Steffen. Regarding the next release note, two items from our side: 1. (-remove) MKL-DNN integration is done. I think we can remove this item. 2. (+add) MKL-DNN based graph optimization and quantization by subgraph Design doc: https://cwiki.apache.org/confluence/display/MXNET/MXNet+Graph+Optimization+and+Quantization+based+on+subgraph+and+MKL-DNN Lead Contributor: Patric Zhao, https://github.com/pengzhao-intel/ Regarding the Roadmap (+add) Q1 2019: MKL-DNN RNN API supports BR, Thanks, --Patric > -Original Message- > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > Sent: Saturday, September 29, 2018 11:31 AM > To: dev@mxnet.incubator.apache.org > Subject: Re: [Discuss] Next MXNet release > > Sorry I meant to say next 'Regarding the *minor* release'. > > On Sat, Sep 29, 2018 at 5:27 AM kellen sunderland < > kellen.sunderl...@gmail.com> wrote: > > > Thanks for transparently setting a rough timeline Steffen. I think > > this will go a long way in helping the community plan their work, even > > if the details change somewhat on the road to the release. > > > > Regarding the major release: I would propose we unify TensorRT with > > the subgraph operator work. > > > > Regarding the patch release: There were a few minor stack/buffer > > overflows exposed by ASAN that have been addressed. It's probably a > > good idea to include them in a patch release, as they at best result > > in non-deterministic behaviour. > > > > -Kellen > > > > > > On Sat, Sep 29, 2018 at 1:39 AM Steffen Rochel > > > > wrote: > > > >> I updated > >> > >> https://cwiki.apache.org/confluence/display/MXNET/Project+Proposals+f > >> or+next+MXNet+Release > >> , > >> removed the completed items from 1.3 release and would like to kick > >> off discussion about the next release. Please suggest what you would > >> like to see included in the next release together with link to design > >> proposal (appropriately for the size and complexity of the proposal) > >> or suggest changes. > >> I suggest to target the next release for December 2018 to frame the > >> discussion. > >> Lets include review of > >> https://cwiki.apache.org/confluence/display/MXNET/MXNet+Roadmap - > >> time to update and discuss changes. > >> > >> From the 1.3 release we had discussion regarding > >> https://github.com/apache/incubator-mxnet/issues/11849 and resolution > >> in > >> https://github.com/apache/incubator-mxnet/pull/12412 . > >> Are you aware of critical issues and feedback from user which we > >> should consider for a potential 1.3.1 patch release. Should we > >> include PR 12412 in a potential patch release? > >> > >> Regards, > >> Steffen > >> > >
Re: Which merge option to use on the Import Julia binding PR?
I believe so, but if someone wants to confirm it would be great. Unfortunately, I just came down with a cold/flu so I will be out of communication for a bit On Fri, Sep 28, 2018 at 9:51 PM Marco de Abreu wrote: > Are we sure that this is due to lacking permissions and not because of some > technical limitation? If we are certain, we can ask out mentors to create a > ticket with Apache Infra to make that switch. > > -Marco > > Carin Meier schrieb am Sa., 29. Sep. 2018, 01:17: > > > I made a test regular merge commit into a copy of master. It seemed to go > > fine. Here is a listing of what it will look like for everyone. > > > > > https://github.com/apache/incubator-mxnet/commits/test-merge-julia-import > > > > Although, I would be happy to push the merge button. I think the most > > important thing is to get the PR merged, so whatever way is the best to > > make that happen, let's do it. > > > > So - Does the regular merge seem like a good option? > > If so, what is the best way to make that happen? > > > > > > On Fri, Sep 28, 2018 at 6:05 PM Chiyuan Zhang wrote: > > > > > Agreed with Pedro. Maybe the merge-commit option from the github > > interface > > > was disabled for a reason. But as Pedro said, maybe it is good to > > > temporarily enable it for this PR and merge using that. > > > > > > > > >- It should be technically easier than rebasing due to the > > >git-subtree-import issue we are currently having > > >- It also avoid stacking a huge commit history on *top* of current > > >history > > >- The downside is probably the history of the project is not linear > > >anymore, but I think this is actually what we would like to have for > > > this > > >particular case, because the contents of the main repo and the julia > > > branch > > >actually does not overlap. So it makes sense to have two tails with > > > their > > >own history. > > > > > > Carin: I guess if someone with admin permission on the github could > > > temporarily enable the merge-commit option, then pushing the button on > > the > > > web might simply work. > > > > > > Best, > > > Chiyuan > > > > > > On Fri, Sep 28, 2018 at 2:53 PM Carin Meier > > wrote: > > > > > > > Pedro - Maybe a merge commit is a better answer in this case. I > > > originally > > > > ruled it out since it wasn't an option in the github web interface, > but > > > > since this looks like it is going to have to be done outside it > because > > > of > > > > the subtrees anyway, it might be a better fit. > > > > > > > > On Fri, Sep 28, 2018 at 5:07 PM Carin Meier > > > wrote: > > > > > > > > > We are actually running into troubles with using the subtree and > the > > > > > rebase. Since it looks like this is not going to be a simple, > "click > > > the > > > > > button" through the web page merge, I rather hand this task off to > > > > someone > > > > > with more context in making sure it gets in there correctly. > > > > > > > > > > Chiyuan or any others, would you be willing to take this over? > > > > > > > > > > Thanks, > > > > > Carin > > > > > > > > > > On Fri, Sep 28, 2018 at 5:00 PM Naveen Swamy > > > wrote: > > > > > > > > > >> Should we try to first being into a branch and then try merge that > > > > >> branch? > > > > >> > > > > >> > On Sep 28, 2018, at 4:40 PM, Pedro Larroy < > > > > pedro.larroy.li...@gmail.com> > > > > >> wrote: > > > > >> > > > > > >> > I'm not familiar with the specifics of this contribution, as a > > > general > > > > >> > approach my understanding is that if the list of commits is big > > and > > > > you > > > > >> > want to preserve history, usually merging is better so you keep > > > > history > > > > >> and > > > > >> > causality, if you rebase all the commits on top of master you > are > > > > >> changing > > > > >> > the history of these commits which can't be individually > reverted > > as > > > > >> some > > > > >> > have suggested before. Maybe is because I come from a mercurial > > > > >> background, > > > > >> > but my initial impression would be either to: > > > > >> > 1. squash everything and rebase > > > > >> > 2. or merge without rebasing or squashing. > > > > >> > > > > > >> > Pedro. > > > > >> > > > > > >> >> On Thu, Sep 27, 2018 at 3:10 PM Carin Meier < > > carinme...@gmail.com> > > > > >> wrote: > > > > >> >> > > > > >> >> Thanks everyone for the input. I'll try to summarize the > feedback > > > > from > > > > >> the > > > > >> >> responses: > > > > >> >> > > > > >> >> Using Squash-Merge is the project standard for very good > reasons. > > > > >> However, > > > > >> >> in the case of this PR to bring in the Julia language from its > > > > sibling > > > > >> >> repo, we want to preserve all the individual commits of the > many > > > > >> >> contributors that have worked over multiple years to make this > a > > > > great > > > > >> >> language binding. We will use Rebase-Merge for it. > > > > >> >> > > > > >> >> Chiyuan - thanks for the suggestion of using a tag. I think we > > can
Re: Subscription to the ASF mxnet channel
Invite sent, Welcome to Apache MXNet. Please join #mxnet channel on slack > On Sep 28, 2018, at 5:21 PM, Javier Rodriguez Zaurin > wrote: > > Hi there, I am Javier, I was posting before on slack and I was advised to > subscribe to that channel > > just that :) > > Ciao > J.
Re: Feedback request for new Java API
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 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 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 curre
Subscription
Hi, I would like to subscribe to the ASF mxnet channel. *Cosmin Catalin SANDA* Data Scientist & Engineer Phone: +45.27.30.60.35 Web: https://cosminsanda.com
Re: Subscription
Invite sent. Welcome to Apache MXNet Cosmin :). On Sat, Sep 29, 2018 at 11:38 AM Cosmin Cătălin Sanda < cosmincata...@gmail.com> wrote: > Hi, I would like to subscribe to the ASF mxnet channel. > > *Cosmin Catalin SANDA* > Data Scientist & Engineer > Phone: +45.27.30.60.35 > Web: https://cosminsanda.com >
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
Kellen,
Could you please explain why you think range loops are better and how it
improves readability? this is a relatively new feature, many of them are
used to the old syntax, shouldn't we leave it for the developers to choose
the one that best suits the need and their familiarity.
In general I support the notion of standardizing where necessary, enforcing
rules on loops seems little bit like micro-managing how you should write
C++ code for MXNet.
-1(open to change based on new information)
On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier wrote:
> ok then, my vote is still -1, however, because it’s just adding needless
> friction for developers imho.
>
> On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland <
> kellen.sunderl...@gmail.com> wrote:
>
> > "Range loops aren’t always the most performant way" Do you have an
> example
> > where there's a perf difference?
> >
> > "In addition, sometimes you want the index. Or maybe you want to iterate
> > backwards, or not start from the first, etc. Maybe you want the iterator
> > because you remove it from the list at the bottom of the loop Seems
> > like a rule for the sake of having a rule."
> >
> > I should have been more clear about this point. If you're using the
> index
> > in the loop, doing reverse iteration, or not iterating from start-to-end
> > this inspection is smart enough to realize it and will not suggest
> > optimizing that type of loop. The loops that would be changes are _only_
> > the loops which are detected as equivalent to range-loops. Examples can
> be
> > found here:
> >
> https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
> > or you can look at what's been changed in the ref PR. I've initially set
> > our confidence level at 'reasonable' but we could also set to 'safe'
> which
> > would further reduce the number of loops the check would apply to.
> >
> > -Kellen
> >
> > On Fri, Sep 28, 2018 at 3:54 PM Chris Olivier
> > wrote:
> >
> > > -1
> > >
> > > Range loops aren’t always the most performant way. In addition,
> sometimes
> > > you want the index. Or maybe you want to iterate backwards, or not
> start
> > > from the first, etc. Maybe you want the iterator because you remove it
> > from
> > > the list at the bottom of the loop Seems like a rule for the sake
> of
> > > having a rule.
> > >
> > > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland <
> > > kellen.sunderl...@gmail.com> wrote:
> > >
> > > > Hello MXNet devs,
> > > >
> > > > I'd like to discuss uniformly adopting C++11 range loops in the MXNet
> > > > project. The benefits I see are:
> > > >
> > > > * Improved C++ readability (examples below).
> > > > * Consistency with other languages. The range-loops are quite
> similar
> > > to
> > > > loops almost all other programming languages. Given we're a project
> > that
> > > > supports many languages this language consistency could be positive
> for
> > > our
> > > > community.
> > > > * Consistency within the same project. Currently different authors
> > have
> > > > different loops styles which hurts codebase readability.
> > > > * Best available performance. There are often multiple ways to
> write
> > > > loops in C++ with subtle differences in performance and memory usage
> > > > between loop methods. Using range-loops ensures we get the best
> > possible
> > > > perf using an intuitive loop pattern.
> > > > * Slightly lower chance for bugs / OOB accesses when dealing with
> > > indexing
> > > > in an array for example.
> > > >
> > > > If we decide to enable this uniformly throughout the project we can
> > > enable
> > > > this policy with a simple clang-tidy configuration change. There
> would
> > > be
> > > > no need for reviewers to have to manually provide feedback when
> someone
> > > > uses an older C++ loops style.
> > > >
> > > > -Kellen
> > > >
> > > > Reference PR: https://github.com/apache/incubator-mxnet/pull/12356/
> > > > Previous clang-tidy discussion on the list:
> > > >
> > > >
> > >
> >
> https://lists.apache.org/thread.html/b0ae5a9df5dfe0d9074cb2ebe432264db4fa2175b89fa43a5f6e36be@%3Cdev.mxnet.apache.org%3E
> > > >
> > > > -
> > > > Examples:
> > > > for (auto axis_iter = param.axis.begin() ; axis_iter!=
> > param.axis.end();
> > > > ++axis_iter) {
> > > > CHECK_LT(*axis_iter, static_cast(ishape.ndim()));
> > > > stride_[reverse_index] = ishape[*axis_iter];
> > > > ...
> > > > -->
> > > > for (int axis : param.axis) {
> > > > CHECK_LT(axis, static_cast(ishape.ndim()));
> > > > stride_[reverse_index] = ishape[axis];
> > > > ...
> > > > --
> > > > for (size_t i = 0; i < in_array.size(); i++) {
> > > > auto &nd = in_array[i];
> > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true,
> nd.dtype());
> > > > }
> > > > -->
> > > > for (auto & nd : in_array) {
> > > > pre_temp_buf_.emplace_back(nd.shape(), nd.ctx(), true,
> nd.dtype());
> > > > }
> > > >
> > >
> >
>
Re: Feedback request for new Java API
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 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 :) >
Re: Time out for Travis CI
Kellen, I think this PR introduces extra runtime in CI, thus causes the timeout. Which means, once merged, every PR later will see same timeout in travis. So shall we modify the changes to decrease the test running time? or just disable the Travis CI? On Fri, Sep 28, 2018 at 9:17 PM Qin, Zhennan wrote: > > Hi Kellen, > > Thanks for your explanation. Do you have a time plan to solve the timeout > issue? Rebasing can't work for my case. Or shall we run it silently to > disallow it voting X for overall CI result? Because most developers are used > to ignore the PRs with 'X'. > > Thanks, > Zhennan > > -Original Message- > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > Sent: Friday, September 28, 2018 10:38 PM > To: dev@mxnet.incubator.apache.org > Subject: Re: Time out for Travis CI > > Hey Zhennan, you're safe to ignore Travis failures for now. They're just > informational. > > The reason you sometimes see quick builds and sometimes see slow builds is > that we're making use of ccache in between builds. If your PR is similar to > what's in master you should build very quickly, if not it's going to take a > while and likely time out. If you see timeouts rebasing may speed things up. > Unfortunately the timeouts are global and we're not able to increase them. > I'm hoping that adding artifact caching will speed up future builds to the > point that test runs and builds can be executed in under the global limit > (which is ~50 minutes). > > -Kellen > > > On Fri, Sep 28, 2018 at 4:05 PM Qin, Zhennan wrote: > > > Hi MXNet devs, > > > > I'm struggled with new Travis CI for a while, it always run time out > > for this PR: > > https://github.com/apache/incubator-mxnet/pull/12530 > > > > Most of the time, Jenkins CI can pass, while Travis can't be finished > > within 50 minutes. For this PR, it shouldn't affect much on the build > > time or unit test time. Also, I saw other PR has same problem, eg. > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088?utm_sour > > ce=github_status&utm_medium=notification > > > > https://travis-ci.org/apache/incubator-mxnet/builds/434404305?utm_sour > > ce=github_status&utm_medium=notification > > > > According to the time stamp from Travis, all passed PR are within > > small code change, and can complete `make -j2` within 25s. But for > > timeout case, 'make -j2' will need about 1600s. Does Travis do > > incremental build for each test? Shall we increase time limit for > > large PR? Can we add more time stamp for build and unites stage to help > > understand what's going on there? > > > > Thanks in advance, > > Zhennan > > -- Yizhi Liu DMLC member Amazon Web Services Vancouver, Canada
Re: Feedback request for new Java API
Moreover, regarding "add 200+ APIs" - this was exactly what we did for type-safe APIs in Scala, we have NDArray/Symbol.BatchNorm and NDArray/Symbol.api.BatchNorm - why did we decide to do that? Because we thought type-safe could provide better user-experience. Given the example I listed above, I think such user-experience benefits are even greater than what we got from type-safe. On Sat, Sep 29, 2018 at 11:41 AM YiZhi Liu 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 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. > > > >
Re: Feedback request for new Java API
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 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 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
Re: Feedback request for new Java API
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. 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. 2) It is adding 220+ APIs(I understand it is generated) for NDArray alone - As I explained how it can improve user experiences 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 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. 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" 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. 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. 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 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 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 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
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
It's more readable because it's concise and it's consistent for many types you're looping over (i.e. primitive arrays, stl iterators, etc all work the same way). It's also useful because it's consistent with other programming languages, making C++ codebases much easier to read for novice and intermediate developers. IMO it also leads to better naming in loop bodies as the concise style means you're less likely to have important 1 letter variable names describing loop elements (e.g. no int i =0 or it ...). More motivation can be found in the cpp standards proposals for C++11 http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1868.html and http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm. On Sat, Sep 29, 2018 at 6:38 PM Naveen Swamy wrote: > Kellen, > > Could you please explain why you think range loops are better and how it > improves readability? this is a relatively new feature, many of them are > used to the old syntax, shouldn't we leave it for the developers to choose > the one that best suits the need and their familiarity. > In general I support the notion of standardizing where necessary, enforcing > rules on loops seems little bit like micro-managing how you should write > C++ code for MXNet. > > -1(open to change based on new information) > > > > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier > wrote: > > > ok then, my vote is still -1, however, because it’s just adding needless > > friction for developers imho. > > > > On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland < > > kellen.sunderl...@gmail.com> wrote: > > > > > "Range loops aren’t always the most performant way" Do you have an > > example > > > where there's a perf difference? > > > > > > "In addition, sometimes you want the index. Or maybe you want to > iterate > > > backwards, or not start from the first, etc. Maybe you want the > iterator > > > because you remove it from the list at the bottom of the loop Seems > > > like a rule for the sake of having a rule." > > > > > > I should have been more clear about this point. If you're using the > > index > > > in the loop, doing reverse iteration, or not iterating from > start-to-end > > > this inspection is smart enough to realize it and will not suggest > > > optimizing that type of loop. The loops that would be changes are > _only_ > > > the loops which are detected as equivalent to range-loops. Examples > can > > be > > > found here: > > > > > > https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html > > > or you can look at what's been changed in the ref PR. I've initially > set > > > our confidence level at 'reasonable' but we could also set to 'safe' > > which > > > would further reduce the number of loops the check would apply to. > > > > > > -Kellen > > > > > > On Fri, Sep 28, 2018 at 3:54 PM Chris Olivier > > > wrote: > > > > > > > -1 > > > > > > > > Range loops aren’t always the most performant way. In addition, > > sometimes > > > > you want the index. Or maybe you want to iterate backwards, or not > > start > > > > from the first, etc. Maybe you want the iterator because you remove > it > > > from > > > > the list at the bottom of the loop Seems like a rule for the sake > > of > > > > having a rule. > > > > > > > > On Fri, Sep 28, 2018 at 2:12 AM kellen sunderland < > > > > kellen.sunderl...@gmail.com> wrote: > > > > > > > > > Hello MXNet devs, > > > > > > > > > > I'd like to discuss uniformly adopting C++11 range loops in the > MXNet > > > > > project. The benefits I see are: > > > > > > > > > > * Improved C++ readability (examples below). > > > > > * Consistency with other languages. The range-loops are quite > > similar > > > > to > > > > > loops almost all other programming languages. Given we're a > project > > > that > > > > > supports many languages this language consistency could be positive > > for > > > > our > > > > > community. > > > > > * Consistency within the same project. Currently different authors > > > have > > > > > different loops styles which hurts codebase readability. > > > > > * Best available performance. There are often multiple ways to > > write > > > > > loops in C++ with subtle differences in performance and memory > usage > > > > > between loop methods. Using range-loops ensures we get the best > > > possible > > > > > perf using an intuitive loop pattern. > > > > > * Slightly lower chance for bugs / OOB accesses when dealing with > > > > indexing > > > > > in an array for example. > > > > > > > > > > If we decide to enable this uniformly throughout the project we can > > > > enable > > > > > this policy with a simple clang-tidy configuration change. There > > would > > > > be > > > > > no need for reviewers to have to manually provide feedback when > > someone > > > > > uses an older C++ loops style. > > > > > > > > > > -Kellen > > > > > > > > > > Reference PR: > https://github.com/apache/incubator-mxnet/pull/12356/ > > > > > Previous clang-tidy discussion on the list: > > > >
Re: Time out for Travis CI
Reading over the PR I don't see what aspects would cause extra runtime YiZhi, could you point them out? On Sat, Sep 29, 2018 at 8:46 PM YiZhi Liu wrote: > Kellen, I think this PR introduces extra runtime in CI, thus causes > the timeout. Which means, once merged, every PR later will see same > timeout in travis. > > So shall we modify the changes to decrease the test running time? or > just disable the Travis CI? > > > On Fri, Sep 28, 2018 at 9:17 PM Qin, Zhennan > wrote: > > > > Hi Kellen, > > > > Thanks for your explanation. Do you have a time plan to solve the > timeout issue? Rebasing can't work for my case. Or shall we run it silently > to disallow it voting X for overall CI result? Because most developers are > used to ignore the PRs with 'X'. > > > > Thanks, > > Zhennan > > > > -Original Message- > > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > > Sent: Friday, September 28, 2018 10:38 PM > > To: dev@mxnet.incubator.apache.org > > Subject: Re: Time out for Travis CI > > > > Hey Zhennan, you're safe to ignore Travis failures for now. They're > just informational. > > > > The reason you sometimes see quick builds and sometimes see slow builds > is that we're making use of ccache in between builds. If your PR is > similar to what's in master you should build very quickly, if not it's > going to take a while and likely time out. If you see timeouts rebasing > may speed things up. Unfortunately the timeouts are global and we're not > able to increase them. I'm hoping that adding artifact caching will speed > up future builds to the point that test runs and builds can be executed in > under the global limit (which is ~50 minutes). > > > > -Kellen > > > > > > On Fri, Sep 28, 2018 at 4:05 PM Qin, Zhennan > wrote: > > > > > Hi MXNet devs, > > > > > > I'm struggled with new Travis CI for a while, it always run time out > > > for this PR: > > > https://github.com/apache/incubator-mxnet/pull/12530 > > > > > > Most of the time, Jenkins CI can pass, while Travis can't be finished > > > within 50 minutes. For this PR, it shouldn't affect much on the build > > > time or unit test time. Also, I saw other PR has same problem, eg. > > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088?utm_sour > > > ce=github_status&utm_medium=notification > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/434404305?utm_sour > > > ce=github_status&utm_medium=notification > > > > > > According to the time stamp from Travis, all passed PR are within > > > small code change, and can complete `make -j2` within 25s. But for > > > timeout case, 'make -j2' will need about 1600s. Does Travis do > > > incremental build for each test? Shall we increase time limit for > > > large PR? Can we add more time stamp for build and unites stage to > help understand what's going on there? > > > > > > Thanks in advance, > > > Zhennan > > > > > > > -- > Yizhi Liu > DMLC member > Amazon Web Services > Vancouver, Canada >
Re: Time out for Travis CI
Honestly I don't know yet. I can help to investigate. Just given the evidence that, travis timeout every time it gets re-triggered - 2 times at least. Correct me if I'm wrong @ Zhennan On Sat, Sep 29, 2018 at 1:54 PM kellen sunderland wrote: > > Reading over the PR I don't see what aspects would cause extra runtime > YiZhi, could you point them out? > > On Sat, Sep 29, 2018 at 8:46 PM YiZhi Liu wrote: > > > Kellen, I think this PR introduces extra runtime in CI, thus causes > > the timeout. Which means, once merged, every PR later will see same > > timeout in travis. > > > > So shall we modify the changes to decrease the test running time? or > > just disable the Travis CI? > > > > > > On Fri, Sep 28, 2018 at 9:17 PM Qin, Zhennan > > wrote: > > > > > > Hi Kellen, > > > > > > Thanks for your explanation. Do you have a time plan to solve the > > timeout issue? Rebasing can't work for my case. Or shall we run it silently > > to disallow it voting X for overall CI result? Because most developers are > > used to ignore the PRs with 'X'. > > > > > > Thanks, > > > Zhennan > > > > > > -Original Message- > > > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > > > Sent: Friday, September 28, 2018 10:38 PM > > > To: dev@mxnet.incubator.apache.org > > > Subject: Re: Time out for Travis CI > > > > > > Hey Zhennan, you're safe to ignore Travis failures for now. They're > > just informational. > > > > > > The reason you sometimes see quick builds and sometimes see slow builds > > is that we're making use of ccache in between builds. If your PR is > > similar to what's in master you should build very quickly, if not it's > > going to take a while and likely time out. If you see timeouts rebasing > > may speed things up. Unfortunately the timeouts are global and we're not > > able to increase them. I'm hoping that adding artifact caching will speed > > up future builds to the point that test runs and builds can be executed in > > under the global limit (which is ~50 minutes). > > > > > > -Kellen > > > > > > > > > On Fri, Sep 28, 2018 at 4:05 PM Qin, Zhennan > > wrote: > > > > > > > Hi MXNet devs, > > > > > > > > I'm struggled with new Travis CI for a while, it always run time out > > > > for this PR: > > > > https://github.com/apache/incubator-mxnet/pull/12530 > > > > > > > > Most of the time, Jenkins CI can pass, while Travis can't be finished > > > > within 50 minutes. For this PR, it shouldn't affect much on the build > > > > time or unit test time. Also, I saw other PR has same problem, eg. > > > > > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088?utm_sour > > > > ce=github_status&utm_medium=notification > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/434404305?utm_sour > > > > ce=github_status&utm_medium=notification > > > > > > > > According to the time stamp from Travis, all passed PR are within > > > > small code change, and can complete `make -j2` within 25s. But for > > > > timeout case, 'make -j2' will need about 1600s. Does Travis do > > > > incremental build for each test? Shall we increase time limit for > > > > large PR? Can we add more time stamp for build and unites stage to > > help understand what's going on there? > > > > > > > > Thanks in advance, > > > > Zhennan > > > > > > > > > > > > -- > > Yizhi Liu > > DMLC member > > Amazon Web Services > > Vancouver, Canada > > -- Yizhi Liu DMLC member Amazon Web Services Vancouver, Canada
Re: Time out for Travis CI
while other PRs are all good. On Sat, Sep 29, 2018 at 2:13 PM YiZhi Liu wrote: > > Honestly I don't know yet. I can help to investigate. Just given the > evidence that, travis timeout every time it gets re-triggered - 2 > times at least. Correct me if I'm wrong @ Zhennan > On Sat, Sep 29, 2018 at 1:54 PM kellen sunderland > wrote: > > > > Reading over the PR I don't see what aspects would cause extra runtime > > YiZhi, could you point them out? > > > > On Sat, Sep 29, 2018 at 8:46 PM YiZhi Liu wrote: > > > > > Kellen, I think this PR introduces extra runtime in CI, thus causes > > > the timeout. Which means, once merged, every PR later will see same > > > timeout in travis. > > > > > > So shall we modify the changes to decrease the test running time? or > > > just disable the Travis CI? > > > > > > > > > On Fri, Sep 28, 2018 at 9:17 PM Qin, Zhennan > > > wrote: > > > > > > > > Hi Kellen, > > > > > > > > Thanks for your explanation. Do you have a time plan to solve the > > > timeout issue? Rebasing can't work for my case. Or shall we run it > > > silently > > > to disallow it voting X for overall CI result? Because most developers are > > > used to ignore the PRs with 'X'. > > > > > > > > Thanks, > > > > Zhennan > > > > > > > > -Original Message- > > > > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > > > > Sent: Friday, September 28, 2018 10:38 PM > > > > To: dev@mxnet.incubator.apache.org > > > > Subject: Re: Time out for Travis CI > > > > > > > > Hey Zhennan, you're safe to ignore Travis failures for now. They're > > > just informational. > > > > > > > > The reason you sometimes see quick builds and sometimes see slow builds > > > is that we're making use of ccache in between builds. If your PR is > > > similar to what's in master you should build very quickly, if not it's > > > going to take a while and likely time out. If you see timeouts rebasing > > > may speed things up. Unfortunately the timeouts are global and we're not > > > able to increase them. I'm hoping that adding artifact caching will speed > > > up future builds to the point that test runs and builds can be executed in > > > under the global limit (which is ~50 minutes). > > > > > > > > -Kellen > > > > > > > > > > > > On Fri, Sep 28, 2018 at 4:05 PM Qin, Zhennan > > > wrote: > > > > > > > > > Hi MXNet devs, > > > > > > > > > > I'm struggled with new Travis CI for a while, it always run time out > > > > > for this PR: > > > > > https://github.com/apache/incubator-mxnet/pull/12530 > > > > > > > > > > Most of the time, Jenkins CI can pass, while Travis can't be finished > > > > > within 50 minutes. For this PR, it shouldn't affect much on the build > > > > > time or unit test time. Also, I saw other PR has same problem, eg. > > > > > > > > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088?utm_sour > > > > > ce=github_status&utm_medium=notification > > > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/434404305?utm_sour > > > > > ce=github_status&utm_medium=notification > > > > > > > > > > According to the time stamp from Travis, all passed PR are within > > > > > small code change, and can complete `make -j2` within 25s. But for > > > > > timeout case, 'make -j2' will need about 1600s. Does Travis do > > > > > incremental build for each test? Shall we increase time limit for > > > > > large PR? Can we add more time stamp for build and unites stage to > > > help understand what's going on there? > > > > > > > > > > Thanks in advance, > > > > > Zhennan > > > > > > > > > > > > > > > > > -- > > > 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
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
+1
Maybe it's not necessary to enforce usage of range-based for, but I would
highly encourage to to it due to already named advantages. If code would be
introduced using the old-style there could be a comment suggesting the new
way. But why do the manual work and not leave that to the automated tool?
And since it's already automated - wouldn't it be better to keep a unified
modern style?
Just to make this a trend - C++ evolves quickly and this will not be only
upgrade that would needed to be made. And the easier such upgrades get
accepted the easier in general is to upgrade the codebase.
Soon the standard will get ranges and concepts and this will change the way
C++ applications get written significantly. It is a good habit to be open
for changes and keep up with the trends. By using the new possibilities the
language can offer you prepare yourself for further changes and are more
likely to accept them, evolving your programming style.
Take a look at a new examples on modern usages (taken from [1]):
// since C++17
for (auto&& [first,second] : mymap) {
// use first and second
}
// since C++20
for (auto& x : foo().items()) { /* .. */ } // undefined behavior if foo()
returns by value
for (T thing = foo(); auto& x : thing.items()) { /* ... */ } // OK
// since C++11
struct cow_string { /* ... */ };
// a copy-on-write string cow_string str = /* ... */;
// for(auto x : str) { /* ... */ } // may cause deep copy
for(auto x : std::as_const(str)) { /* ... */ }
Regarding performance: it's really easy to prove that generated assembly is
not changing at all. There is a really handy tool for that [2]. You can
check online the assembly for different language constructs and different
compilers.
Best regards,
Anton
[1] https://en.cppreference.com/w/cpp/language/range-for
[2] https://gcc.godbolt.org
сб, 29 сент. 2018 г. в 13:15, kellen sunderland :
> It's more readable because it's concise and it's consistent for many types
> you're looping over (i.e. primitive arrays, stl iterators, etc all work the
> same way). It's also useful because it's consistent with other programming
> languages, making C++ codebases much easier to read for novice and
> intermediate developers. IMO it also leads to better naming in loop bodies
> as the concise style means you're less likely to have important 1 letter
> variable names describing loop elements (e.g. no int i =0 or it ...). More
> motivation can be found in the cpp standards proposals for C++11
> http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1868.html and
> http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm.
>
>
>
> On Sat, Sep 29, 2018 at 6:38 PM Naveen Swamy wrote:
>
> > Kellen,
> >
> > Could you please explain why you think range loops are better and how it
> > improves readability? this is a relatively new feature, many of them are
> > used to the old syntax, shouldn't we leave it for the developers to
> choose
> > the one that best suits the need and their familiarity.
> > In general I support the notion of standardizing where necessary,
> enforcing
> > rules on loops seems little bit like micro-managing how you should write
> > C++ code for MXNet.
> >
> > -1(open to change based on new information)
> >
> >
> >
> > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier
> > wrote:
> >
> > > ok then, my vote is still -1, however, because it’s just adding
> needless
> > > friction for developers imho.
> > >
> > > On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland <
> > > kellen.sunderl...@gmail.com> wrote:
> > >
> > > > "Range loops aren’t always the most performant way" Do you have an
> > > example
> > > > where there's a perf difference?
> > > >
> > > > "In addition, sometimes you want the index. Or maybe you want to
> > iterate
> > > > backwards, or not start from the first, etc. Maybe you want the
> > iterator
> > > > because you remove it from the list at the bottom of the loop
> Seems
> > > > like a rule for the sake of having a rule."
> > > >
> > > > I should have been more clear about this point. If you're using the
> > > index
> > > > in the loop, doing reverse iteration, or not iterating from
> > start-to-end
> > > > this inspection is smart enough to realize it and will not suggest
> > > > optimizing that type of loop. The loops that would be changes are
> > _only_
> > > > the loops which are detected as equivalent to range-loops. Examples
> > can
> > > be
> > > > found here:
> > > >
> > >
> >
> https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
> > > > or you can look at what's been changed in the ref PR. I've initially
> > set
> > > > our confidence level at 'reasonable' but we could also set to 'safe'
> > > which
> > > > would further reduce the number of loops the check would apply to.
> > > >
> > > > -Kellen
> > > >
> > > > On Fri, Sep 28, 2018 at 3:54 PM Chris Olivier <
> cjolivie...@apache.org>
> > > > wrote:
> > > >
> > > > > -1
> > > > >
> > > > > Range loops aren’t always the most performant way.
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
And if you want a more authoritative opinion on that check out what the C++
core guidelines are saying [1]:
> ES.71: Prefer a range-for-statement to a for-statement when there is a
choice
> Reason
> Readability. Error prevention. Efficiency.
Best regards
Anton
[1]
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-for-range
сб, 29 сент. 2018 г. в 16:13, Anton Chernov :
> +1
>
> Maybe it's not necessary to enforce usage of range-based for, but I would
> highly encourage to to it due to already named advantages. If code would be
> introduced using the old-style there could be a comment suggesting the new
> way. But why do the manual work and not leave that to the automated tool?
>
> And since it's already automated - wouldn't it be better to keep a unified
> modern style?
>
> Just to make this a trend - C++ evolves quickly and this will not be only
> upgrade that would needed to be made. And the easier such upgrades get
> accepted the easier in general is to upgrade the codebase.
>
> Soon the standard will get ranges and concepts and this will change the
> way C++ applications get written significantly. It is a good habit to be
> open for changes and keep up with the trends. By using the new
> possibilities the language can offer you prepare yourself for further
> changes and are more likely to accept them, evolving your programming style.
>
> Take a look at a new examples on modern usages (taken from [1]):
>
> // since C++17
> for (auto&& [first,second] : mymap) {
> // use first and second
> }
>
> // since C++20
> for (auto& x : foo().items()) { /* .. */ } // undefined behavior if foo()
> returns by value
> for (T thing = foo(); auto& x : thing.items()) { /* ... */ } // OK
>
> // since C++11
> struct cow_string { /* ... */ };
> // a copy-on-write string cow_string str = /* ... */;
> // for(auto x : str) { /* ... */ } // may cause deep copy
> for(auto x : std::as_const(str)) { /* ... */ }
>
> Regarding performance: it's really easy to prove that generated assembly
> is not changing at all. There is a really handy tool for that [2]. You can
> check online the assembly for different language constructs and different
> compilers.
>
> Best regards,
> Anton
>
> [1] https://en.cppreference.com/w/cpp/language/range-for
> [2] https://gcc.godbolt.org
>
> сб, 29 сент. 2018 г. в 13:15, kellen sunderland <
> kellen.sunderl...@gmail.com>:
>
>> It's more readable because it's concise and it's consistent for many types
>> you're looping over (i.e. primitive arrays, stl iterators, etc all work
>> the
>> same way). It's also useful because it's consistent with other
>> programming
>> languages, making C++ codebases much easier to read for novice and
>> intermediate developers. IMO it also leads to better naming in loop
>> bodies
>> as the concise style means you're less likely to have important 1 letter
>> variable names describing loop elements (e.g. no int i =0 or it ...).
>> More
>> motivation can be found in the cpp standards proposals for C++11
>> http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1868.html and
>> http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm.
>>
>>
>>
>> On Sat, Sep 29, 2018 at 6:38 PM Naveen Swamy wrote:
>>
>> > Kellen,
>> >
>> > Could you please explain why you think range loops are better and how it
>> > improves readability? this is a relatively new feature, many of them
>> are
>> > used to the old syntax, shouldn't we leave it for the developers to
>> choose
>> > the one that best suits the need and their familiarity.
>> > In general I support the notion of standardizing where necessary,
>> enforcing
>> > rules on loops seems little bit like micro-managing how you should write
>> > C++ code for MXNet.
>> >
>> > -1(open to change based on new information)
>> >
>> >
>> >
>> > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier
>> > wrote:
>> >
>> > > ok then, my vote is still -1, however, because it’s just adding
>> needless
>> > > friction for developers imho.
>> > >
>> > > On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland <
>> > > kellen.sunderl...@gmail.com> wrote:
>> > >
>> > > > "Range loops aren’t always the most performant way" Do you have an
>> > > example
>> > > > where there's a perf difference?
>> > > >
>> > > > "In addition, sometimes you want the index. Or maybe you want to
>> > iterate
>> > > > backwards, or not start from the first, etc. Maybe you want the
>> > iterator
>> > > > because you remove it from the list at the bottom of the loop
>> Seems
>> > > > like a rule for the sake of having a rule."
>> > > >
>> > > > I should have been more clear about this point. If you're using the
>> > > index
>> > > > in the loop, doing reverse iteration, or not iterating from
>> > start-to-end
>> > > > this inspection is smart enough to realize it and will not suggest
>> > > > optimizing that type of loop. The loops that would be changes are
>> > _only_
>> > > > the loops which are detected as equivalent to range-loops. Examples
RE: Time out for Travis CI
Hi YiZhi and Kellen, From my point of view, travis should be able to get passed from a scratch build. Pending result on ccache hit/miss is not a good idea. For this PR, as it changed many header file, lots of files need be recompiled, just like a scratch build. I think that's the reason that travis timeout. This should be fixed before enabling travis, as it will block any change to those base header file. Again, it's not a special case with this PR only, you can find same problem on other PRs: https://travis-ci.org/apache/incubator-mxnet/builds/433172088?utm_source=github_status&utm_medium=notification https://travis-ci.org/apache/incubator-mxnet/builds/434404305?utm_source=github_status&utm_medium=notification Thanks, Zhennan -Original Message- From: YiZhi Liu [mailto:eazhi@gmail.com] Sent: Sunday, September 30, 2018 5:15 AM To: eazhi@gmail.com Cc: dev@mxnet.incubator.apache.org Subject: Re: Time out for Travis CI while other PRs are all good. On Sat, Sep 29, 2018 at 2:13 PM YiZhi Liu wrote: > > Honestly I don't know yet. I can help to investigate. Just given the > evidence that, travis timeout every time it gets re-triggered - 2 > times at least. Correct me if I'm wrong @ Zhennan On Sat, Sep 29, 2018 > at 1:54 PM kellen sunderland wrote: > > > > Reading over the PR I don't see what aspects would cause extra > > runtime YiZhi, could you point them out? > > > > On Sat, Sep 29, 2018 at 8:46 PM YiZhi Liu wrote: > > > > > Kellen, I think this PR introduces extra runtime in CI, thus > > > causes the timeout. Which means, once merged, every PR later will > > > see same timeout in travis. > > > > > > So shall we modify the changes to decrease the test running time? > > > or just disable the Travis CI? > > > > > > > > > On Fri, Sep 28, 2018 at 9:17 PM Qin, Zhennan > > > > > > wrote: > > > > > > > > Hi Kellen, > > > > > > > > Thanks for your explanation. Do you have a time plan to solve > > > > the > > > timeout issue? Rebasing can't work for my case. Or shall we run it > > > silently to disallow it voting X for overall CI result? Because > > > most developers are used to ignore the PRs with 'X'. > > > > > > > > Thanks, > > > > Zhennan > > > > > > > > -Original Message- > > > > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > > > > Sent: Friday, September 28, 2018 10:38 PM > > > > To: dev@mxnet.incubator.apache.org > > > > Subject: Re: Time out for Travis CI > > > > > > > > Hey Zhennan, you're safe to ignore Travis failures for now. > > > > They're > > > just informational. > > > > > > > > The reason you sometimes see quick builds and sometimes see slow > > > > builds > > > is that we're making use of ccache in between builds. If your PR > > > is similar to what's in master you should build very quickly, if > > > not it's going to take a while and likely time out. If you see > > > timeouts rebasing may speed things up. Unfortunately the timeouts > > > are global and we're not able to increase them. I'm hoping that > > > adding artifact caching will speed up future builds to the point > > > that test runs and builds can be executed in under the global limit > > > (which is ~50 minutes). > > > > > > > > -Kellen > > > > > > > > > > > > On Fri, Sep 28, 2018 at 4:05 PM Qin, Zhennan > > > > > > > wrote: > > > > > > > > > Hi MXNet devs, > > > > > > > > > > I'm struggled with new Travis CI for a while, it always run > > > > > time out for this PR: > > > > > https://github.com/apache/incubator-mxnet/pull/12530 > > > > > > > > > > Most of the time, Jenkins CI can pass, while Travis can't be > > > > > finished within 50 minutes. For this PR, it shouldn't affect > > > > > much on the build time or unit test time. Also, I saw other PR has > > > > > same problem, eg. > > > > > > > > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088? > > > > > utm_sour ce=github_status&utm_medium=notification > > > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/434404305? > > > > > utm_sour ce=github_status&utm_medium=notification > > > > > > > > > > According to the time stamp from Travis, all passed PR are > > > > > within small code change, and can complete `make -j2` within > > > > > 25s. But for timeout case, 'make -j2' will need about 1600s. > > > > > Does Travis do incremental build for each test? Shall we > > > > > increase time limit for large PR? Can we add more time stamp > > > > > for build and unites stage to > > > help understand what's going on there? > > > > > > > > > > Thanks in advance, > > > > > Zhennan > > > > > > > > > > > > > > > > > -- > > > 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
Re: Time out for Travis CI
Hey Zhennan, yes this is the exact problem, and I agree with your points completely. This is why when we first added Travis we attempted to communicate that it would be informational only, and that we'd need to iterate on the config before it would be a test that people should consider 'required'. Apologies, we should have been more straightforward about those tradeoffs. The strong point in favour of adding Travis in informational mode was that we had a serious MacOS specific bug that we wanted to verify was fixed. The good news is I've opened a PR which I hope will speed up these builds to the point that they won't rely on caching. Once it is merged it would be very helpful if you could rebase on this PR and test to ensure that large changes no longer hit the global timeout without cache. https://github.com/apache/incubator-mxnet/pull/12706 On Sun, Sep 30, 2018 at 2:48 AM Qin, Zhennan wrote: > Hi YiZhi and Kellen, > > From my point of view, travis should be able to get passed from a scratch > build. Pending result on ccache hit/miss is not a good idea. For this PR, > as it changed many header file, lots of files need be recompiled, just like > a scratch build. I think that's the reason that travis timeout. This should > be fixed before enabling travis, as it will block any change to those base > header file. Again, it's not a special case with this PR only, you can find > same problem on other PRs: > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088?utm_source=github_status&utm_medium=notification > > https://travis-ci.org/apache/incubator-mxnet/builds/434404305?utm_source=github_status&utm_medium=notification > > > Thanks, > Zhennan > > -Original Message- > From: YiZhi Liu [mailto:eazhi@gmail.com] > Sent: Sunday, September 30, 2018 5:15 AM > To: eazhi@gmail.com > Cc: dev@mxnet.incubator.apache.org > Subject: Re: Time out for Travis CI > > while other PRs are all good. > On Sat, Sep 29, 2018 at 2:13 PM YiZhi Liu wrote: > > > > Honestly I don't know yet. I can help to investigate. Just given the > > evidence that, travis timeout every time it gets re-triggered - 2 > > times at least. Correct me if I'm wrong @ Zhennan On Sat, Sep 29, 2018 > > at 1:54 PM kellen sunderland wrote: > > > > > > Reading over the PR I don't see what aspects would cause extra > > > runtime YiZhi, could you point them out? > > > > > > On Sat, Sep 29, 2018 at 8:46 PM YiZhi Liu wrote: > > > > > > > Kellen, I think this PR introduces extra runtime in CI, thus > > > > causes the timeout. Which means, once merged, every PR later will > > > > see same timeout in travis. > > > > > > > > So shall we modify the changes to decrease the test running time? > > > > or just disable the Travis CI? > > > > > > > > > > > > On Fri, Sep 28, 2018 at 9:17 PM Qin, Zhennan > > > > > > > > wrote: > > > > > > > > > > Hi Kellen, > > > > > > > > > > Thanks for your explanation. Do you have a time plan to solve > > > > > the > > > > timeout issue? Rebasing can't work for my case. Or shall we run it > > > > silently to disallow it voting X for overall CI result? Because > > > > most developers are used to ignore the PRs with 'X'. > > > > > > > > > > Thanks, > > > > > Zhennan > > > > > > > > > > -Original Message- > > > > > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > > > > > Sent: Friday, September 28, 2018 10:38 PM > > > > > To: dev@mxnet.incubator.apache.org > > > > > Subject: Re: Time out for Travis CI > > > > > > > > > > Hey Zhennan, you're safe to ignore Travis failures for now. > > > > > They're > > > > just informational. > > > > > > > > > > The reason you sometimes see quick builds and sometimes see slow > > > > > builds > > > > is that we're making use of ccache in between builds. If your PR > > > > is similar to what's in master you should build very quickly, if > > > > not it's going to take a while and likely time out. If you see > > > > timeouts rebasing may speed things up. Unfortunately the timeouts > > > > are global and we're not able to increase them. I'm hoping that > > > > adding artifact caching will speed up future builds to the point > > > > that test runs and builds can be executed in under the global limit > (which is ~50 minutes). > > > > > > > > > > -Kellen > > > > > > > > > > > > > > > On Fri, Sep 28, 2018 at 4:05 PM Qin, Zhennan > > > > > > > > > wrote: > > > > > > > > > > > Hi MXNet devs, > > > > > > > > > > > > I'm struggled with new Travis CI for a while, it always run > > > > > > time out for this PR: > > > > > > https://github.com/apache/incubator-mxnet/pull/12530 > > > > > > > > > > > > Most of the time, Jenkins CI can pass, while Travis can't be > > > > > > finished within 50 minutes. For this PR, it shouldn't affect > > > > > > much on the build time or unit test time. Also, I saw other PR > has same problem, eg. > > > > > > > > > > > > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088? > > > > > > utm_sour ce=github_status&utm
Re: Feedback request for new Java API
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 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 i
Re: Which merge option to use on the Import Julia binding PR?
There is an option in the repo settings menu to disable or enable merge-commit for PR, see a screenshot below (from a different github project): [image: image.png] My guess is that this is disabled for the reason to avoid creating non-linear history for standard PRs (as oppose to technical problem). But this is only my guess, it would be great if someone could confirm. Best, Chiyuan On Sat, Sep 29, 2018 at 3:50 AM Carin Meier wrote: > I believe so, but if someone wants to confirm it would be great. > Unfortunately, I just came down with a cold/flu so I will be out of > communication for a bit > > On Fri, Sep 28, 2018 at 9:51 PM Marco de Abreu > wrote: > > > Are we sure that this is due to lacking permissions and not because of > some > > technical limitation? If we are certain, we can ask out mentors to > create a > > ticket with Apache Infra to make that switch. > > > > -Marco > > > > Carin Meier schrieb am Sa., 29. Sep. 2018, 01:17: > > > > > I made a test regular merge commit into a copy of master. It seemed to > go > > > fine. Here is a listing of what it will look like for everyone. > > > > > > > > > https://github.com/apache/incubator-mxnet/commits/test-merge-julia-import > > > > > > Although, I would be happy to push the merge button. I think the most > > > important thing is to get the PR merged, so whatever way is the best to > > > make that happen, let's do it. > > > > > > So - Does the regular merge seem like a good option? > > > If so, what is the best way to make that happen? > > > > > > > > > On Fri, Sep 28, 2018 at 6:05 PM Chiyuan Zhang > wrote: > > > > > > > Agreed with Pedro. Maybe the merge-commit option from the github > > > interface > > > > was disabled for a reason. But as Pedro said, maybe it is good to > > > > temporarily enable it for this PR and merge using that. > > > > > > > > > > > >- It should be technically easier than rebasing due to the > > > >git-subtree-import issue we are currently having > > > >- It also avoid stacking a huge commit history on *top* of current > > > >history > > > >- The downside is probably the history of the project is not > linear > > > >anymore, but I think this is actually what we would like to have > for > > > > this > > > >particular case, because the contents of the main repo and the > julia > > > > branch > > > >actually does not overlap. So it makes sense to have two tails > with > > > > their > > > >own history. > > > > > > > > Carin: I guess if someone with admin permission on the github could > > > > temporarily enable the merge-commit option, then pushing the button > on > > > the > > > > web might simply work. > > > > > > > > Best, > > > > Chiyuan > > > > > > > > On Fri, Sep 28, 2018 at 2:53 PM Carin Meier > > > wrote: > > > > > > > > > Pedro - Maybe a merge commit is a better answer in this case. I > > > > originally > > > > > ruled it out since it wasn't an option in the github web interface, > > but > > > > > since this looks like it is going to have to be done outside it > > because > > > > of > > > > > the subtrees anyway, it might be a better fit. > > > > > > > > > > On Fri, Sep 28, 2018 at 5:07 PM Carin Meier > > > > wrote: > > > > > > > > > > > We are actually running into troubles with using the subtree and > > the > > > > > > rebase. Since it looks like this is not going to be a simple, > > "click > > > > the > > > > > > button" through the web page merge, I rather hand this task off > to > > > > > someone > > > > > > with more context in making sure it gets in there correctly. > > > > > > > > > > > > Chiyuan or any others, would you be willing to take this over? > > > > > > > > > > > > Thanks, > > > > > > Carin > > > > > > > > > > > > On Fri, Sep 28, 2018 at 5:00 PM Naveen Swamy > > > > > wrote: > > > > > > > > > > > >> Should we try to first being into a branch and then try merge > that > > > > > >> branch? > > > > > >> > > > > > >> > On Sep 28, 2018, at 4:40 PM, Pedro Larroy < > > > > > pedro.larroy.li...@gmail.com> > > > > > >> wrote: > > > > > >> > > > > > > >> > I'm not familiar with the specifics of this contribution, as a > > > > general > > > > > >> > approach my understanding is that if the list of commits is > big > > > and > > > > > you > > > > > >> > want to preserve history, usually merging is better so you > keep > > > > > history > > > > > >> and > > > > > >> > causality, if you rebase all the commits on top of master you > > are > > > > > >> changing > > > > > >> > the history of these commits which can't be individually > > reverted > > > as > > > > > >> some > > > > > >> > have suggested before. Maybe is because I come from a > mercurial > > > > > >> background, > > > > > >> > but my initial impression would be either to: > > > > > >> > 1. squash everything and rebase > > > > > >> > 2. or merge without rebasing or squashing. > > > > > >> > > > > > > >> > Pedro. > > > > > >> > > > > > > >> >> On Thu, Sep 27, 2018 at 3:10 PM Carin Meier < > > > carinme...@gmail.com
Re: Which merge option to use on the Import Julia binding PR?
yes, AFAIK I think Apache Infra has disabled the merge option. If there is a valid reason(and this is one), we could ask our Mentors to help us create a INFRA ticket to temporarily enable this option and once we are done merging we can request to disable it again. On Sat, Sep 29, 2018 at 9:44 PM Chiyuan Zhang wrote: > There is an option in the repo settings menu to disable or enable > merge-commit for PR, see a screenshot below (from a different github > project): > > [image: image.png] > > My guess is that this is disabled for the reason to avoid creating > non-linear history for standard PRs (as oppose to technical problem). But > this is only my guess, it would be great if someone could confirm. > > Best, > Chiyuan > > On Sat, Sep 29, 2018 at 3:50 AM Carin Meier wrote: > >> I believe so, but if someone wants to confirm it would be great. >> Unfortunately, I just came down with a cold/flu so I will be out of >> communication for a bit >> >> On Fri, Sep 28, 2018 at 9:51 PM Marco de Abreu >> wrote: >> >> > Are we sure that this is due to lacking permissions and not because of >> some >> > technical limitation? If we are certain, we can ask out mentors to >> create a >> > ticket with Apache Infra to make that switch. >> > >> > -Marco >> > >> > Carin Meier schrieb am Sa., 29. Sep. 2018, >> 01:17: >> > >> > > I made a test regular merge commit into a copy of master. It seemed >> to go >> > > fine. Here is a listing of what it will look like for everyone. >> > > >> > > >> > >> https://github.com/apache/incubator-mxnet/commits/test-merge-julia-import >> > > >> > > Although, I would be happy to push the merge button. I think the most >> > > important thing is to get the PR merged, so whatever way is the best >> to >> > > make that happen, let's do it. >> > > >> > > So - Does the regular merge seem like a good option? >> > > If so, what is the best way to make that happen? >> > > >> > > >> > > On Fri, Sep 28, 2018 at 6:05 PM Chiyuan Zhang >> wrote: >> > > >> > > > Agreed with Pedro. Maybe the merge-commit option from the github >> > > interface >> > > > was disabled for a reason. But as Pedro said, maybe it is good to >> > > > temporarily enable it for this PR and merge using that. >> > > > >> > > > >> > > >- It should be technically easier than rebasing due to the >> > > >git-subtree-import issue we are currently having >> > > >- It also avoid stacking a huge commit history on *top* of >> current >> > > >history >> > > >- The downside is probably the history of the project is not >> linear >> > > >anymore, but I think this is actually what we would like to have >> for >> > > > this >> > > >particular case, because the contents of the main repo and the >> julia >> > > > branch >> > > >actually does not overlap. So it makes sense to have two tails >> with >> > > > their >> > > >own history. >> > > > >> > > > Carin: I guess if someone with admin permission on the github could >> > > > temporarily enable the merge-commit option, then pushing the button >> on >> > > the >> > > > web might simply work. >> > > > >> > > > Best, >> > > > Chiyuan >> > > > >> > > > On Fri, Sep 28, 2018 at 2:53 PM Carin Meier >> > > wrote: >> > > > >> > > > > Pedro - Maybe a merge commit is a better answer in this case. I >> > > > originally >> > > > > ruled it out since it wasn't an option in the github web >> interface, >> > but >> > > > > since this looks like it is going to have to be done outside it >> > because >> > > > of >> > > > > the subtrees anyway, it might be a better fit. >> > > > > >> > > > > On Fri, Sep 28, 2018 at 5:07 PM Carin Meier > > >> > > > wrote: >> > > > > >> > > > > > We are actually running into troubles with using the subtree and >> > the >> > > > > > rebase. Since it looks like this is not going to be a simple, >> > "click >> > > > the >> > > > > > button" through the web page merge, I rather hand this task off >> to >> > > > > someone >> > > > > > with more context in making sure it gets in there correctly. >> > > > > > >> > > > > > Chiyuan or any others, would you be willing to take this over? >> > > > > > >> > > > > > Thanks, >> > > > > > Carin >> > > > > > >> > > > > > On Fri, Sep 28, 2018 at 5:00 PM Naveen Swamy < >> mnnav...@gmail.com> >> > > > wrote: >> > > > > > >> > > > > >> Should we try to first being into a branch and then try merge >> that >> > > > > >> branch? >> > > > > >> >> > > > > >> > On Sep 28, 2018, at 4:40 PM, Pedro Larroy < >> > > > > pedro.larroy.li...@gmail.com> >> > > > > >> wrote: >> > > > > >> > >> > > > > >> > I'm not familiar with the specifics of this contribution, as >> a >> > > > general >> > > > > >> > approach my understanding is that if the list of commits is >> big >> > > and >> > > > > you >> > > > > >> > want to preserve history, usually merging is better so you >> keep >> > > > > history >> > > > > >> and >> > > > > >> > causality, if you rebase all the commits on top of master you >> > are >> > > > > >> changing >> > > > > >> >
Re: Which merge option to use on the Import Julia binding PR?
Could you upload the picture somewhere please? HTML is being stripped out on email lists. Chiyuan Zhang schrieb am So., 30. Sep. 2018, 03:44: > There is an option in the repo settings menu to disable or enable > merge-commit for PR, see a screenshot below (from a different github > project): > > [image: image.png] > > My guess is that this is disabled for the reason to avoid creating > non-linear history for standard PRs (as oppose to technical problem). But > this is only my guess, it would be great if someone could confirm. > > Best, > Chiyuan > > On Sat, Sep 29, 2018 at 3:50 AM Carin Meier wrote: > >> I believe so, but if someone wants to confirm it would be great. >> Unfortunately, I just came down with a cold/flu so I will be out of >> communication for a bit >> >> On Fri, Sep 28, 2018 at 9:51 PM Marco de Abreu >> wrote: >> >> > Are we sure that this is due to lacking permissions and not because of >> some >> > technical limitation? If we are certain, we can ask out mentors to >> create a >> > ticket with Apache Infra to make that switch. >> > >> > -Marco >> > >> > Carin Meier schrieb am Sa., 29. Sep. 2018, >> 01:17: >> > >> > > I made a test regular merge commit into a copy of master. It seemed >> to go >> > > fine. Here is a listing of what it will look like for everyone. >> > > >> > > >> > >> https://github.com/apache/incubator-mxnet/commits/test-merge-julia-import >> > > >> > > Although, I would be happy to push the merge button. I think the most >> > > important thing is to get the PR merged, so whatever way is the best >> to >> > > make that happen, let's do it. >> > > >> > > So - Does the regular merge seem like a good option? >> > > If so, what is the best way to make that happen? >> > > >> > > >> > > On Fri, Sep 28, 2018 at 6:05 PM Chiyuan Zhang >> wrote: >> > > >> > > > Agreed with Pedro. Maybe the merge-commit option from the github >> > > interface >> > > > was disabled for a reason. But as Pedro said, maybe it is good to >> > > > temporarily enable it for this PR and merge using that. >> > > > >> > > > >> > > >- It should be technically easier than rebasing due to the >> > > >git-subtree-import issue we are currently having >> > > >- It also avoid stacking a huge commit history on *top* of >> current >> > > >history >> > > >- The downside is probably the history of the project is not >> linear >> > > >anymore, but I think this is actually what we would like to have >> for >> > > > this >> > > >particular case, because the contents of the main repo and the >> julia >> > > > branch >> > > >actually does not overlap. So it makes sense to have two tails >> with >> > > > their >> > > >own history. >> > > > >> > > > Carin: I guess if someone with admin permission on the github could >> > > > temporarily enable the merge-commit option, then pushing the button >> on >> > > the >> > > > web might simply work. >> > > > >> > > > Best, >> > > > Chiyuan >> > > > >> > > > On Fri, Sep 28, 2018 at 2:53 PM Carin Meier >> > > wrote: >> > > > >> > > > > Pedro - Maybe a merge commit is a better answer in this case. I >> > > > originally >> > > > > ruled it out since it wasn't an option in the github web >> interface, >> > but >> > > > > since this looks like it is going to have to be done outside it >> > because >> > > > of >> > > > > the subtrees anyway, it might be a better fit. >> > > > > >> > > > > On Fri, Sep 28, 2018 at 5:07 PM Carin Meier > > >> > > > wrote: >> > > > > >> > > > > > We are actually running into troubles with using the subtree and >> > the >> > > > > > rebase. Since it looks like this is not going to be a simple, >> > "click >> > > > the >> > > > > > button" through the web page merge, I rather hand this task off >> to >> > > > > someone >> > > > > > with more context in making sure it gets in there correctly. >> > > > > > >> > > > > > Chiyuan or any others, would you be willing to take this over? >> > > > > > >> > > > > > Thanks, >> > > > > > Carin >> > > > > > >> > > > > > On Fri, Sep 28, 2018 at 5:00 PM Naveen Swamy < >> mnnav...@gmail.com> >> > > > wrote: >> > > > > > >> > > > > >> Should we try to first being into a branch and then try merge >> that >> > > > > >> branch? >> > > > > >> >> > > > > >> > On Sep 28, 2018, at 4:40 PM, Pedro Larroy < >> > > > > pedro.larroy.li...@gmail.com> >> > > > > >> wrote: >> > > > > >> > >> > > > > >> > I'm not familiar with the specifics of this contribution, as >> a >> > > > general >> > > > > >> > approach my understanding is that if the list of commits is >> big >> > > and >> > > > > you >> > > > > >> > want to preserve history, usually merging is better so you >> keep >> > > > > history >> > > > > >> and >> > > > > >> > causality, if you rebase all the commits on top of master you >> > are >> > > > > >> changing >> > > > > >> > the history of these commits which can't be individually >> > reverted >> > > as >> > > > > >> some >> > > > > >> > have suggested before. Maybe is because I come from a >> mer
Re: Time out for Travis CI
This makes sense. Thanks On Sat, Sep 29, 2018 at 6:36 PM kellen sunderland < kellen.sunderl...@gmail.com> wrote: > Hey Zhennan, yes this is the exact problem, and I agree with your points > completely. This is why when we first added Travis we attempted to > communicate that it would be informational only, and that we'd need to > iterate on the config before it would be a test that people should consider > 'required'. Apologies, we should have been more straightforward about > those tradeoffs. The strong point in favour of adding Travis in > informational mode was that we had a serious MacOS specific bug that we > wanted to verify was fixed. > > The good news is I've opened a PR which I hope will speed up these builds > to the point that they won't rely on caching. Once it is merged it would > be very helpful if you could rebase on this PR and test to ensure that > large changes no longer hit the global timeout without cache. > https://github.com/apache/incubator-mxnet/pull/12706 > > On Sun, Sep 30, 2018 at 2:48 AM Qin, Zhennan > wrote: > > > Hi YiZhi and Kellen, > > > > From my point of view, travis should be able to get passed from a scratch > > build. Pending result on ccache hit/miss is not a good idea. For this PR, > > as it changed many header file, lots of files need be recompiled, just > like > > a scratch build. I think that's the reason that travis timeout. This > should > > be fixed before enabling travis, as it will block any change to those > base > > header file. Again, it's not a special case with this PR only, you can > find > > same problem on other PRs: > > > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/433172088?utm_source=github_status&utm_medium=notification > > > > > https://travis-ci.org/apache/incubator-mxnet/builds/434404305?utm_source=github_status&utm_medium=notification > > > > > > Thanks, > > Zhennan > > > > -Original Message- > > From: YiZhi Liu [mailto:eazhi@gmail.com] > > Sent: Sunday, September 30, 2018 5:15 AM > > To: eazhi@gmail.com > > Cc: dev@mxnet.incubator.apache.org > > Subject: Re: Time out for Travis CI > > > > while other PRs are all good. > > On Sat, Sep 29, 2018 at 2:13 PM YiZhi Liu wrote: > > > > > > Honestly I don't know yet. I can help to investigate. Just given the > > > evidence that, travis timeout every time it gets re-triggered - 2 > > > times at least. Correct me if I'm wrong @ Zhennan On Sat, Sep 29, 2018 > > > at 1:54 PM kellen sunderland wrote: > > > > > > > > Reading over the PR I don't see what aspects would cause extra > > > > runtime YiZhi, could you point them out? > > > > > > > > On Sat, Sep 29, 2018 at 8:46 PM YiZhi Liu > wrote: > > > > > > > > > Kellen, I think this PR introduces extra runtime in CI, thus > > > > > causes the timeout. Which means, once merged, every PR later will > > > > > see same timeout in travis. > > > > > > > > > > So shall we modify the changes to decrease the test running time? > > > > > or just disable the Travis CI? > > > > > > > > > > > > > > > On Fri, Sep 28, 2018 at 9:17 PM Qin, Zhennan > > > > > > > > > > wrote: > > > > > > > > > > > > Hi Kellen, > > > > > > > > > > > > Thanks for your explanation. Do you have a time plan to solve > > > > > > the > > > > > timeout issue? Rebasing can't work for my case. Or shall we run it > > > > > silently to disallow it voting X for overall CI result? Because > > > > > most developers are used to ignore the PRs with 'X'. > > > > > > > > > > > > Thanks, > > > > > > Zhennan > > > > > > > > > > > > -Original Message- > > > > > > From: kellen sunderland [mailto:kellen.sunderl...@gmail.com] > > > > > > Sent: Friday, September 28, 2018 10:38 PM > > > > > > To: dev@mxnet.incubator.apache.org > > > > > > Subject: Re: Time out for Travis CI > > > > > > > > > > > > Hey Zhennan, you're safe to ignore Travis failures for now. > > > > > > They're > > > > > just informational. > > > > > > > > > > > > The reason you sometimes see quick builds and sometimes see slow > > > > > > builds > > > > > is that we're making use of ccache in between builds. If your PR > > > > > is similar to what's in master you should build very quickly, if > > > > > not it's going to take a while and likely time out. If you see > > > > > timeouts rebasing may speed things up. Unfortunately the timeouts > > > > > are global and we're not able to increase them. I'm hoping that > > > > > adding artifact caching will speed up future builds to the point > > > > > that test runs and builds can be executed in under the global limit > > (which is ~50 minutes). > > > > > > > > > > > > -Kellen > > > > > > > > > > > > > > > > > > On Fri, Sep 28, 2018 at 4:05 PM Qin, Zhennan > > > > > > > > > > > wrote: > > > > > > > > > > > > > Hi MXNet devs, > > > > > > > > > > > > > > I'm struggled with new Travis CI for a while, it always run > > > > > > > time out for this PR: > > > > > > > https://github.com/apache/incubator-mxnet/pull/12530 > > > > > > > > > > > > > > Most of the time,
Re: Which merge option to use on the Import Julia binding PR?
We can also request Infra directly to execute an override action on our behalf - that way, they don't have to change the configuration and it creates less work for them. That's something they did for us back then when we switched CI. If we are all aligned, it shouldn't be a big deal for them to just execute it. Could we maybe have a dry run on a test repository to show everyone what the outcome would look like? Best regards, Marco Naveen Swamy schrieb am So., 30. Sep. 2018, 03:49: > yes, AFAIK I think Apache Infra has disabled the merge option. > > If there is a valid reason(and this is one), we could ask our Mentors to > help us create a INFRA ticket to temporarily enable this option and once we > are done merging we can request to disable it again. > > On Sat, Sep 29, 2018 at 9:44 PM Chiyuan Zhang wrote: > > > There is an option in the repo settings menu to disable or enable > > merge-commit for PR, see a screenshot below (from a different github > > project): > > > > [image: image.png] > > > > My guess is that this is disabled for the reason to avoid creating > > non-linear history for standard PRs (as oppose to technical problem). But > > this is only my guess, it would be great if someone could confirm. > > > > Best, > > Chiyuan > > > > On Sat, Sep 29, 2018 at 3:50 AM Carin Meier > wrote: > > > >> I believe so, but if someone wants to confirm it would be great. > >> Unfortunately, I just came down with a cold/flu so I will be out of > >> communication for a bit > >> > >> On Fri, Sep 28, 2018 at 9:51 PM Marco de Abreu > >> wrote: > >> > >> > Are we sure that this is due to lacking permissions and not because of > >> some > >> > technical limitation? If we are certain, we can ask out mentors to > >> create a > >> > ticket with Apache Infra to make that switch. > >> > > >> > -Marco > >> > > >> > Carin Meier schrieb am Sa., 29. Sep. 2018, > >> 01:17: > >> > > >> > > I made a test regular merge commit into a copy of master. It seemed > >> to go > >> > > fine. Here is a listing of what it will look like for everyone. > >> > > > >> > > > >> > > >> > https://github.com/apache/incubator-mxnet/commits/test-merge-julia-import > >> > > > >> > > Although, I would be happy to push the merge button. I think the > most > >> > > important thing is to get the PR merged, so whatever way is the best > >> to > >> > > make that happen, let's do it. > >> > > > >> > > So - Does the regular merge seem like a good option? > >> > > If so, what is the best way to make that happen? > >> > > > >> > > > >> > > On Fri, Sep 28, 2018 at 6:05 PM Chiyuan Zhang > >> wrote: > >> > > > >> > > > Agreed with Pedro. Maybe the merge-commit option from the github > >> > > interface > >> > > > was disabled for a reason. But as Pedro said, maybe it is good to > >> > > > temporarily enable it for this PR and merge using that. > >> > > > > >> > > > > >> > > >- It should be technically easier than rebasing due to the > >> > > >git-subtree-import issue we are currently having > >> > > >- It also avoid stacking a huge commit history on *top* of > >> current > >> > > >history > >> > > >- The downside is probably the history of the project is not > >> linear > >> > > >anymore, but I think this is actually what we would like to > have > >> for > >> > > > this > >> > > >particular case, because the contents of the main repo and the > >> julia > >> > > > branch > >> > > >actually does not overlap. So it makes sense to have two tails > >> with > >> > > > their > >> > > >own history. > >> > > > > >> > > > Carin: I guess if someone with admin permission on the github > could > >> > > > temporarily enable the merge-commit option, then pushing the > button > >> on > >> > > the > >> > > > web might simply work. > >> > > > > >> > > > Best, > >> > > > Chiyuan > >> > > > > >> > > > On Fri, Sep 28, 2018 at 2:53 PM Carin Meier > > >> > > wrote: > >> > > > > >> > > > > Pedro - Maybe a merge commit is a better answer in this case. I > >> > > > originally > >> > > > > ruled it out since it wasn't an option in the github web > >> interface, > >> > but > >> > > > > since this looks like it is going to have to be done outside it > >> > because > >> > > > of > >> > > > > the subtrees anyway, it might be a better fit. > >> > > > > > >> > > > > On Fri, Sep 28, 2018 at 5:07 PM Carin Meier < > carinme...@gmail.com > >> > > >> > > > wrote: > >> > > > > > >> > > > > > We are actually running into troubles with using the subtree > and > >> > the > >> > > > > > rebase. Since it looks like this is not going to be a simple, > >> > "click > >> > > > the > >> > > > > > button" through the web page merge, I rather hand this task > off > >> to > >> > > > > someone > >> > > > > > with more context in making sure it gets in there correctly. > >> > > > > > > >> > > > > > Chiyuan or any others, would you be willing to take this over? > >> > > > > > > >> > > > > > Thanks, > >> > > > > > Carin > >> > > > > > > >> > > > > > On Fri, Sep 28, 2018 at
Re: Which merge option to use on the Import Julia binding PR?
Sorry, here is the image: https://imgur.com/V5wd2XB And here is the github document on the 3 different merge options for the web UI button: https://help.github.com/articles/about-pull-request-merges/ On Sat, Sep 29, 2018 at 6:48 PM Marco de Abreu wrote: > Could you upload the picture somewhere please? HTML is being stripped out > on email lists. > > Chiyuan Zhang schrieb am So., 30. Sep. 2018, 03:44: > > > There is an option in the repo settings menu to disable or enable > > merge-commit for PR, see a screenshot below (from a different github > > project): > > > > [image: image.png] > > > > My guess is that this is disabled for the reason to avoid creating > > non-linear history for standard PRs (as oppose to technical problem). But > > this is only my guess, it would be great if someone could confirm. > > > > Best, > > Chiyuan > > > > On Sat, Sep 29, 2018 at 3:50 AM Carin Meier > wrote: > > > >> I believe so, but if someone wants to confirm it would be great. > >> Unfortunately, I just came down with a cold/flu so I will be out of > >> communication for a bit > >> > >> On Fri, Sep 28, 2018 at 9:51 PM Marco de Abreu > >> wrote: > >> > >> > Are we sure that this is due to lacking permissions and not because of > >> some > >> > technical limitation? If we are certain, we can ask out mentors to > >> create a > >> > ticket with Apache Infra to make that switch. > >> > > >> > -Marco > >> > > >> > Carin Meier schrieb am Sa., 29. Sep. 2018, > >> 01:17: > >> > > >> > > I made a test regular merge commit into a copy of master. It seemed > >> to go > >> > > fine. Here is a listing of what it will look like for everyone. > >> > > > >> > > > >> > > >> > https://github.com/apache/incubator-mxnet/commits/test-merge-julia-import > >> > > > >> > > Although, I would be happy to push the merge button. I think the > most > >> > > important thing is to get the PR merged, so whatever way is the best > >> to > >> > > make that happen, let's do it. > >> > > > >> > > So - Does the regular merge seem like a good option? > >> > > If so, what is the best way to make that happen? > >> > > > >> > > > >> > > On Fri, Sep 28, 2018 at 6:05 PM Chiyuan Zhang > >> wrote: > >> > > > >> > > > Agreed with Pedro. Maybe the merge-commit option from the github > >> > > interface > >> > > > was disabled for a reason. But as Pedro said, maybe it is good to > >> > > > temporarily enable it for this PR and merge using that. > >> > > > > >> > > > > >> > > >- It should be technically easier than rebasing due to the > >> > > >git-subtree-import issue we are currently having > >> > > >- It also avoid stacking a huge commit history on *top* of > >> current > >> > > >history > >> > > >- The downside is probably the history of the project is not > >> linear > >> > > >anymore, but I think this is actually what we would like to > have > >> for > >> > > > this > >> > > >particular case, because the contents of the main repo and the > >> julia > >> > > > branch > >> > > >actually does not overlap. So it makes sense to have two tails > >> with > >> > > > their > >> > > >own history. > >> > > > > >> > > > Carin: I guess if someone with admin permission on the github > could > >> > > > temporarily enable the merge-commit option, then pushing the > button > >> on > >> > > the > >> > > > web might simply work. > >> > > > > >> > > > Best, > >> > > > Chiyuan > >> > > > > >> > > > On Fri, Sep 28, 2018 at 2:53 PM Carin Meier > > >> > > wrote: > >> > > > > >> > > > > Pedro - Maybe a merge commit is a better answer in this case. I > >> > > > originally > >> > > > > ruled it out since it wasn't an option in the github web > >> interface, > >> > but > >> > > > > since this looks like it is going to have to be done outside it > >> > because > >> > > > of > >> > > > > the subtrees anyway, it might be a better fit. > >> > > > > > >> > > > > On Fri, Sep 28, 2018 at 5:07 PM Carin Meier < > carinme...@gmail.com > >> > > >> > > > wrote: > >> > > > > > >> > > > > > We are actually running into troubles with using the subtree > and > >> > the > >> > > > > > rebase. Since it looks like this is not going to be a simple, > >> > "click > >> > > > the > >> > > > > > button" through the web page merge, I rather hand this task > off > >> to > >> > > > > someone > >> > > > > > with more context in making sure it gets in there correctly. > >> > > > > > > >> > > > > > Chiyuan or any others, would you be willing to take this over? > >> > > > > > > >> > > > > > Thanks, > >> > > > > > Carin > >> > > > > > > >> > > > > > On Fri, Sep 28, 2018 at 5:00 PM Naveen Swamy < > >> mnnav...@gmail.com> > >> > > > wrote: > >> > > > > > > >> > > > > >> Should we try to first being into a branch and then try merge > >> that > >> > > > > >> branch? > >> > > > > >> > >> > > > > >> > On Sep 28, 2018, at 4:40 PM, Pedro Larroy < > >> > > > > pedro.larroy.li...@gmail.com> > >> > > > > >> wrote: > >> > > > > >> > > >> > > > > >> > I'm not familiar with the specifics of thi
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
Thanks Kellen & Anton, for your detailed explanation and links to
advantages, appreciate it.
changing my vote to *-0*, I suggest to show as warnings.
On Sat, Sep 29, 2018 at 8:06 PM Anton Chernov wrote:
> And if you want a more authoritative opinion on that check out what the C++
> core guidelines are saying [1]:
>
> > ES.71: Prefer a range-for-statement to a for-statement when there is a
> choice
> > Reason
> > Readability. Error prevention. Efficiency.
>
> Best regards
> Anton
>
> [1]
>
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-for-range
>
>
> сб, 29 сент. 2018 г. в 16:13, Anton Chernov :
>
> > +1
> >
> > Maybe it's not necessary to enforce usage of range-based for, but I would
> > highly encourage to to it due to already named advantages. If code would
> be
> > introduced using the old-style there could be a comment suggesting the
> new
> > way. But why do the manual work and not leave that to the automated tool?
> >
> > And since it's already automated - wouldn't it be better to keep a
> unified
> > modern style?
> >
> > Just to make this a trend - C++ evolves quickly and this will not be only
> > upgrade that would needed to be made. And the easier such upgrades get
> > accepted the easier in general is to upgrade the codebase.
> >
> > Soon the standard will get ranges and concepts and this will change the
> > way C++ applications get written significantly. It is a good habit to be
> > open for changes and keep up with the trends. By using the new
> > possibilities the language can offer you prepare yourself for further
> > changes and are more likely to accept them, evolving your programming
> style.
> >
> > Take a look at a new examples on modern usages (taken from [1]):
> >
> > // since C++17
> > for (auto&& [first,second] : mymap) {
> > // use first and second
> > }
> >
> > // since C++20
> > for (auto& x : foo().items()) { /* .. */ } // undefined behavior if foo()
> > returns by value
> > for (T thing = foo(); auto& x : thing.items()) { /* ... */ } // OK
> >
> > // since C++11
> > struct cow_string { /* ... */ };
> > // a copy-on-write string cow_string str = /* ... */;
> > // for(auto x : str) { /* ... */ } // may cause deep copy
> > for(auto x : std::as_const(str)) { /* ... */ }
> >
> > Regarding performance: it's really easy to prove that generated assembly
> > is not changing at all. There is a really handy tool for that [2]. You
> can
> > check online the assembly for different language constructs and different
> > compilers.
> >
> > Best regards,
> > Anton
> >
> > [1] https://en.cppreference.com/w/cpp/language/range-for
> > [2] https://gcc.godbolt.org
> >
> > сб, 29 сент. 2018 г. в 13:15, kellen sunderland <
> > kellen.sunderl...@gmail.com>:
> >
> >> It's more readable because it's concise and it's consistent for many
> types
> >> you're looping over (i.e. primitive arrays, stl iterators, etc all work
> >> the
> >> same way). It's also useful because it's consistent with other
> >> programming
> >> languages, making C++ codebases much easier to read for novice and
> >> intermediate developers. IMO it also leads to better naming in loop
> >> bodies
> >> as the concise style means you're less likely to have important 1 letter
> >> variable names describing loop elements (e.g. no int i =0 or it ...).
> >> More
> >> motivation can be found in the cpp standards proposals for C++11
> >> http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1868.html and
> >> http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm.
> >>
> >>
> >>
> >> On Sat, Sep 29, 2018 at 6:38 PM Naveen Swamy
> wrote:
> >>
> >> > Kellen,
> >> >
> >> > Could you please explain why you think range loops are better and how
> it
> >> > improves readability? this is a relatively new feature, many of them
> >> are
> >> > used to the old syntax, shouldn't we leave it for the developers to
> >> choose
> >> > the one that best suits the need and their familiarity.
> >> > In general I support the notion of standardizing where necessary,
> >> enforcing
> >> > rules on loops seems little bit like micro-managing how you should
> write
> >> > C++ code for MXNet.
> >> >
> >> > -1(open to change based on new information)
> >> >
> >> >
> >> >
> >> > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier
> >> > wrote:
> >> >
> >> > > ok then, my vote is still -1, however, because it’s just adding
> >> needless
> >> > > friction for developers imho.
> >> > >
> >> > > On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland <
> >> > > kellen.sunderl...@gmail.com> wrote:
> >> > >
> >> > > > "Range loops aren’t always the most performant way" Do you have an
> >> > > example
> >> > > > where there's a perf difference?
> >> > > >
> >> > > > "In addition, sometimes you want the index. Or maybe you want to
> >> > iterate
> >> > > > backwards, or not start from the first, etc. Maybe you want the
> >> > iterator
> >> > > > because you remove it from the list at the bottom of the loop
> >> Seems
> >> > > >
Re: Feedback request for new Java API
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 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 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 prob
Re: Feedback request for new Java API
And if we find incorrect declaration, we fix it, not simply assuming
many of them also has problem and we cannot rely on them - otherwise
the type-safe APIs in Scala also does not make sense.
On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu wrote:
>
> 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 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 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
Re: Feedback request for new Java API
I think we have had enough of an debate between the two of us and I have
already listed my reasons, I will stop here and see what others say given
my reasoning.
-1 to #2)
Also, by lecture I meant to say "I don't want to list all the problems
with unnecessary complications and talk about how to design software"
On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu wrote:
> And if we find incorrect declaration, we fix it, not simply assuming
> many of them also has problem and we cannot rely on them - otherwise
> the type-safe APIs in Scala also does not make sense.
> On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu wrote:
> >
> > 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 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 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.
> > >
Re: Feedback request for new Java API
No you haven't answered my question "Since you agree to have 30+
operators have Builder, what prevents from
having all of them have Builder?"
On Sat, Sep 29, 2018 at 7:30 PM Naveen Swamy wrote:
>
> I think we have had enough of an debate between the two of us and I have
> already listed my reasons, I will stop here and see what others say given
> my reasoning.
>
> -1 to #2)
>
> Also, by lecture I meant to say "I don't want to list all the problems
> with unnecessary complications and talk about how to design software"
>
> On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu wrote:
>
> > And if we find incorrect declaration, we fix it, not simply assuming
> > many of them also has problem and we cannot rely on them - otherwise
> > the type-safe APIs in Scala also does not make sense.
> > On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu wrote:
> > >
> > > 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 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 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
> > > > >
> >
Re: Feedback request for new Java API
Also sometimes people may not be at the same page when talking about option
#2. What I insist is the builder classes for each operator. Otherwise I
actually more support Naveen’s approach - not to totally separate java and
scala objects.
On Sat, Sep 29, 2018 at 7:35 PM YiZhi Liu wrote:
> No you haven't answered my question "Since you agree to have 30+
> operators have Builder, what prevents from
> having all of them have Builder?"
> On Sat, Sep 29, 2018 at 7:30 PM Naveen Swamy wrote:
> >
> > I think we have had enough of an debate between the two of us and I have
> > already listed my reasons, I will stop here and see what others say
> given
> > my reasoning.
> >
> > -1 to #2)
> >
> > Also, by lecture I meant to say "I don't want to list all the problems
> > with unnecessary complications and talk about how to design software"
> >
> > On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu wrote:
> >
> > > And if we find incorrect declaration, we fix it, not simply assuming
> > > many of them also has problem and we cannot rely on them - otherwise
> > > the type-safe APIs in Scala also does not make sense.
> > > On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu wrote:
> > > >
> > > > 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
> 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
> 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
> > > > > >
> > >
Re: Feedback request for new Java API
I am not sure what makes you think that I am suggesting we should not fix
it. I am pointing out that many of those are incorrectly optional so don't
take that into consideration to make a decision whether we need builders
for all.
On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu wrote:
> And if we find incorrect declaration, we fix it, not simply assuming
> many of them also has problem and we cannot rely on them - otherwise
> the type-safe APIs in Scala also does not make sense.
> On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu wrote:
> >
> > 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 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 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
Re: Feedback request for new Java API
IMO, its unnecessary. this is only my opinion and you are free to disagree.
With more opinions one or some of us will have to commit to the majority of
the opinion.
On Sat, Sep 29, 2018 at 10:35 PM YiZhi Liu wrote:
> No you haven't answered my question "Since you agree to have 30+
> operators have Builder, what prevents from
> having all of them have Builder?"
> On Sat, Sep 29, 2018 at 7:30 PM Naveen Swamy wrote:
> >
> > I think we have had enough of an debate between the two of us and I have
> > already listed my reasons, I will stop here and see what others say
> given
> > my reasoning.
> >
> > -1 to #2)
> >
> > Also, by lecture I meant to say "I don't want to list all the problems
> > with unnecessary complications and talk about how to design software"
> >
> > On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu wrote:
> >
> > > And if we find incorrect declaration, we fix it, not simply assuming
> > > many of them also has problem and we cannot rely on them - otherwise
> > > the type-safe APIs in Scala also does not make sense.
> > > On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu wrote:
> > > >
> > > > 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
> 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
> 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
>
Re: Feedback request for new Java API
Ah! we agree on something :) lets get more opinions, I am happy to go with it. On Sat, Sep 29, 2018 at 10:40 PM YiZhi Liu wrote: > Also sometimes people may not be at the same page when talking about option > #2. What I insist is the builder classes for each operator. Otherwise I > actually more support Naveen’s approach - not to totally separate java and > scala objects. > > On Sat, Sep 29, 2018 at 7:35 PM YiZhi Liu wrote: > > > No you haven't answered my question "Since you agree to have 30+ > > operators have Builder, what prevents from > > having all of them have Builder?" > > On Sat, Sep 29, 2018 at 7:30 PM Naveen Swamy wrote: > > > > > > I think we have had enough of an debate between the two of us and I > have > > > already listed my reasons, I will stop here and see what others say > > given > > > my reasoning. > > > > > > -1 to #2) > > > > > > Also, by lecture I meant to say "I don't want to list all the problems > > > with unnecessary complications and talk about how to design software" > > > > > > On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu > wrote: > > > > > > > And if we find incorrect declaration, we fix it, not simply assuming > > > > many of them also has problem and we cannot rely on them - otherwise > > > > the type-safe APIs in Scala also does not make sense. > > > > On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu > wrote: > > > > > > > > > > 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 > > 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 > > 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 j
Re: Feedback request for new Java API
Yes agreement and disagreement stay at technical level only:) Back to the problem, they are unnecessary but good in terms of, 1. Still not good for java users to write 3 nulls in a function call with 5 or 4 args 2. Every function call with a “tail” null for arg “out”. I would say, makes it seems not a serious api design to our users 3. Users have uniformed experience, nothing surprising. Given the reasons I listed before, I don’t see things bad for this. I agree we can vote. I suggest to have two votes, one for builder, one for separating java and scala objects. On Sat, Sep 29, 2018 at 7:43 PM Naveen Swamy wrote: > Ah! we agree on something :) lets get more opinions, I am happy to go with > it. > > On Sat, Sep 29, 2018 at 10:40 PM YiZhi Liu wrote: > > > Also sometimes people may not be at the same page when talking about > option > > #2. What I insist is the builder classes for each operator. Otherwise I > > actually more support Naveen’s approach - not to totally separate java > and > > scala objects. > > > > On Sat, Sep 29, 2018 at 7:35 PM YiZhi Liu wrote: > > > > > No you haven't answered my question "Since you agree to have 30+ > > > operators have Builder, what prevents from > > > having all of them have Builder?" > > > On Sat, Sep 29, 2018 at 7:30 PM Naveen Swamy > wrote: > > > > > > > > I think we have had enough of an debate between the two of us and I > > have > > > > already listed my reasons, I will stop here and see what others say > > > given > > > > my reasoning. > > > > > > > > -1 to #2) > > > > > > > > Also, by lecture I meant to say "I don't want to list all the > problems > > > > with unnecessary complications and talk about how to design software" > > > > > > > > On Sat, Sep 29, 2018 at 10:15 PM YiZhi Liu > > wrote: > > > > > > > > > And if we find incorrect declaration, we fix it, not simply > assuming > > > > > many of them also has problem and we cannot rely on them - > otherwise > > > > > the type-safe APIs in Scala also does not make sense. > > > > > On Sat, Sep 29, 2018 at 7:10 PM YiZhi Liu > > wrote: > > > > > > > > > > > > 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 > > > > 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 > > > > 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
Re: [DISCUSS] Use modernized C++11 range loops uniformly throughout the project
How many errors exist in the code base right now if it were to be enabled?
On Sat, Sep 29, 2018 at 7:03 PM Naveen Swamy wrote:
> Thanks Kellen & Anton, for your detailed explanation and links to
> advantages, appreciate it.
> changing my vote to *-0*, I suggest to show as warnings.
>
> On Sat, Sep 29, 2018 at 8:06 PM Anton Chernov wrote:
>
> > And if you want a more authoritative opinion on that check out what the
> C++
> > core guidelines are saying [1]:
> >
> > > ES.71: Prefer a range-for-statement to a for-statement when there is a
> > choice
> > > Reason
> > > Readability. Error prevention. Efficiency.
> >
> > Best regards
> > Anton
> >
> > [1]
> >
> >
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-for-range
> >
> >
> > сб, 29 сент. 2018 г. в 16:13, Anton Chernov :
> >
> > > +1
> > >
> > > Maybe it's not necessary to enforce usage of range-based for, but I
> would
> > > highly encourage to to it due to already named advantages. If code
> would
> > be
> > > introduced using the old-style there could be a comment suggesting the
> > new
> > > way. But why do the manual work and not leave that to the automated
> tool?
> > >
> > > And since it's already automated - wouldn't it be better to keep a
> > unified
> > > modern style?
> > >
> > > Just to make this a trend - C++ evolves quickly and this will not be
> only
> > > upgrade that would needed to be made. And the easier such upgrades get
> > > accepted the easier in general is to upgrade the codebase.
> > >
> > > Soon the standard will get ranges and concepts and this will change the
> > > way C++ applications get written significantly. It is a good habit to
> be
> > > open for changes and keep up with the trends. By using the new
> > > possibilities the language can offer you prepare yourself for further
> > > changes and are more likely to accept them, evolving your programming
> > style.
> > >
> > > Take a look at a new examples on modern usages (taken from [1]):
> > >
> > > // since C++17
> > > for (auto&& [first,second] : mymap) {
> > > // use first and second
> > > }
> > >
> > > // since C++20
> > > for (auto& x : foo().items()) { /* .. */ } // undefined behavior if
> foo()
> > > returns by value
> > > for (T thing = foo(); auto& x : thing.items()) { /* ... */ } // OK
> > >
> > > // since C++11
> > > struct cow_string { /* ... */ };
> > > // a copy-on-write string cow_string str = /* ... */;
> > > // for(auto x : str) { /* ... */ } // may cause deep copy
> > > for(auto x : std::as_const(str)) { /* ... */ }
> > >
> > > Regarding performance: it's really easy to prove that generated
> assembly
> > > is not changing at all. There is a really handy tool for that [2]. You
> > can
> > > check online the assembly for different language constructs and
> different
> > > compilers.
> > >
> > > Best regards,
> > > Anton
> > >
> > > [1] https://en.cppreference.com/w/cpp/language/range-for
> > > [2] https://gcc.godbolt.org
> > >
> > > сб, 29 сент. 2018 г. в 13:15, kellen sunderland <
> > > kellen.sunderl...@gmail.com>:
> > >
> > >> It's more readable because it's concise and it's consistent for many
> > types
> > >> you're looping over (i.e. primitive arrays, stl iterators, etc all
> work
> > >> the
> > >> same way). It's also useful because it's consistent with other
> > >> programming
> > >> languages, making C++ codebases much easier to read for novice and
> > >> intermediate developers. IMO it also leads to better naming in loop
> > >> bodies
> > >> as the concise style means you're less likely to have important 1
> letter
> > >> variable names describing loop elements (e.g. no int i =0 or it ...).
> > >> More
> > >> motivation can be found in the cpp standards proposals for C++11
> > >> http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1868.html
> and
> > >> http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm.
> > >>
> > >>
> > >>
> > >> On Sat, Sep 29, 2018 at 6:38 PM Naveen Swamy
> > wrote:
> > >>
> > >> > Kellen,
> > >> >
> > >> > Could you please explain why you think range loops are better and
> how
> > it
> > >> > improves readability? this is a relatively new feature, many of
> them
> > >> are
> > >> > used to the old syntax, shouldn't we leave it for the developers to
> > >> choose
> > >> > the one that best suits the need and their familiarity.
> > >> > In general I support the notion of standardizing where necessary,
> > >> enforcing
> > >> > rules on loops seems little bit like micro-managing how you should
> > write
> > >> > C++ code for MXNet.
> > >> >
> > >> > -1(open to change based on new information)
> > >> >
> > >> >
> > >> >
> > >> > On Fri, Sep 28, 2018 at 5:20 PM Chris Olivier <
> cjolivie...@gmail.com>
> > >> > wrote:
> > >> >
> > >> > > ok then, my vote is still -1, however, because it’s just adding
> > >> needless
> > >> > > friction for developers imho.
> > >> > >
> > >> > > On Fri, Sep 28, 2018 at 7:42 AM kellen sunderland <
> > >> > > kellen.sunderl...@gmail.com> wro
