I am looking forward to see 208 merged, *soon* please. From my tests it seems that this should be the priority.
I think 702 has merits (but I’ve used it less) and deserves to be merged too once ready. Ultimately after a period of "real road” testing we will be able to understand what we really need. E.g. from past discussions I am not convinced that either PR would, as-it-is, support fully the needs of a multi-user Zeppelin Server approach (something similar to RStudio Server Professional to get an idea). A period of use and gradual evolution (and possibly merge?) will be required. The sooner we start the better. Enzo e...@smartinsightsfromdata.com > On 29 Mar 2016, at 07:08, Jeff Steinmetz <jeffrey.steinm...@gmail.com> wrote: > > I’m not affiliated to either author nor have any attachment to an specific > outcome - and happy to continue being as objective and unbiased as possible. > > > I would say they now have different philosophical approaches (as of the March > 23rd merge of datalayer#7 to 702). > I agree with Amos Elberg that 702 has changed directions a few times. > > Re: commits to 702 by Leemoonsoo on March 23 via datalayer#7: > I found the current state of the 702 PR to be succinct, in terms of it’s > code base, via its use of the SparkR dependency - which is already baked into > spark distribution. > > The 702 code base appears to be easier to maintain using this approach (less > code, no rscala source, no BSD licensing additions required, easier to read). > 702 packages correctly with -Pbuild-distr as expected - i.e. it works out of > gate from the distribution directory. > The build profile -Psparkr worked as expected, and the addition of this > profile felt intuitive to me. > > > Myself and a colleague that uses R extensively noticed (as Amos Elberg > reminded us): > 208 handles passing arrays and other data types between scala & R more > gracefully than 702. > 208 handles the output of intermediate R calls more gracefully than 702. > > Beyond that: > 208 Requires SPARK_HOME to be set or the interpreter hangs with no error. > It’s been mentioned by the 208 author that the requirement to set SPARK_HOME > is a feature. I think we could revisit this assumption now that I see how > 702 handles this with defaults via a graceful fallback. > 702 works fine with zero configuration, I.e for those that want to test > locally with no separate spark distribution installed (SPARK_HOME does not > need to be set). > 702 having just an %r interpreter, and having it as part of the spark > interpreter (same subdirectory) feels like a cleaner approach (this is > arguably a philosophical difference again). > > > It feels like an exhaustive list of `.z.show.googleVis(Motion)` type calls in > 208 could bloom into unnecessary code maintenance overhead and required > additions every time a new chart library is introduced, vs. a more generic > show method. Perhaps a follow on improvement post merge (applies to both > PRs). > This same chart rendering works in 702 with `print(Motion, tag='chart’)` > which isn’t necessarily better or worse, again, a different philosophical > approach. > > They both have merit in different regards. It’s doesn’t feel appropriate to > make a broad statement that "no-one supported 702”. > If I had a magic wand, it would be a hybrid of the two approaches. > > I look forward to continuing the review of each PR individually or both > collaboratively. > > Regards, > Jeff > > > On 3/28/16, 4:13 PM, "Sourav Mazumder" <sourav.mazumde...@gmail.com> wrote: > >> All said and done we had enough discussion on this point for many months >> now. As far as I know, 208 is the PR which community/people have so far >> used mostly and successfully (at least me and whoever I introduced to 208 >> for SparkR support). I thought it was going to be merged a long time ago. >> May be what will make sense is to first integrate the 208. After that, a >> new PR can be created on that and if 702 has anything extra then that >> feature can be added. >> >> Regards, >> Sourav >> >> >> On Mon, Mar 28, 2016 at 12:37 AM, Eran Witkon <eranwit...@gmail.com> wrote: >> >>> @Elberg, If I were you I would ask myself why isn't the community taking >>> part in this debate? >>> Personally I prefer a team player as a contributor over the best developer. >>> just my 2c >>> Eran >>> >>> On Mon, 28 Mar 2016 at 09:52 Amos B. Elberg <amos.elb...@gmail.com> wrote: >>> >>>> Moon - I opened this discussion so it could take place with the community >>>> as a whole, not just you. >>>> >>>> Suffice it to say, I disagree with every one of the technical claims >>>> you've just made, and I don't trust your intent. >>>> >>>> Let the community process happen. >>>> >>>>> On Mar 28, 2016, at 2:47 AM, moon soo Lee <m...@apache.org> wrote: >>>>> >>>>> Hi, >>>>> >>>>> Simply put, >>>>> >>>>> - 702 and/or 208 will can merged as they're ready. [1] >>>>> - 208 will not be merged while it does not pass CI. If you think code >>> in >>>>> 208 is not a problem but CI itself or other part of Zeppelin is >>> problem, >>>>> then that particular problem be fixed before merge 208. >>>>> - 702 has proper integration test [2] >>>>> >>>>> I'm not sure why you're so hard at devaluating 702. >>>>> 702 is not something you need to beat and win. 702 is something you >>> need >>>> to >>>>> help / learn / collaborate. >>>>> >>>>> Will you able to show your ability to collaborate with other community >>>>> members? >>>>> >>>>> Thanks, >>>>> moon >>>>> >>>>> [1] >>>>> >>>> >>> http://apache-zeppelin-incubating-dev-mailing-list.75694.x6.nabble.com/R-interpreter-in-Zeppelin-further-steps-tp6967.html >>>>> [2] >>>>> >>>> >>> https://github.com/apache/incubator-zeppelin/pull/702/files#diff-64a9440e811c5fba6ac1b61157fa6912R87 >>>>> >>>>> >>>>>> On Sun, Mar 27, 2016 at 7:11 PM Amos Elberg <amos.elb...@gmail.com> >>>> wrote: >>>>>> >>>>>> I am saddened to have to start this thread *again*. While I thought >>> we >>>> had >>>>>> reached consensus on this, several times over, apparently some people >>>>>> disagree. I hope this will be the last time. >>>>>> >>>>>> With this thread, I am asking the community to reach consensus (1) >>> That >>>> 208 >>>>>> should be merged this week, without further delay; and (2) That Moon >>> Lee >>>>>> Soo and Felix Cheung take no further part in the discussions of 208 >>> and >>>>>> 702. >>>>>> >>>>>> This PR has been pending since August. It has been stalled that entire >>>> time >>>>>> for no technical reason. >>>>>> >>>>>> We reached agreement to merge 208 in November, again in December, and >>>> again >>>>>> in February -- when Moon agreed to stay out of the issue. At that >>>> point, >>>>>> Alex, I, and others, began working on it, and appeared to be making >>>>>> substantial progress. >>>>>> >>>>>> And then Alex just stopped. Instead, he commenced the thread saying >>>> that a >>>>>> consensus had to be reached on 208 and 702. Until that point, >>>> essentially >>>>>> no-one had paid attention to 702. In the discussion that followed, we >>>>>> reached a consensus to merge 208 as soon as possible. After the >>> thread >>>> had >>>>>> died, Alex asked if anyone had additional comments, and Moon popped-in >>>> to >>>>>> insist that both PRs be merged. Again, no-one supported 702. At all. >>>>>> >>>>>> Each time I said "we had a consensus before, does anyone want to >>> change >>>>>> it," Alex or Moon steered the discussion away. The final vote was not >>>> to >>>>>> merge 702 or merge "both" -- it was to treat them as normal PRs. >>>> (Although >>>>>> one person did want both merged simultaneously.) That would mean >>>>>> completing 208 on its merits and then evaluating 702. >>>>>> >>>>>> At the time, I objected to the discussion, because I thought the whole >>>>>> thing was a contrived excuse for Moon to reject 208 by pushing 702. >>>> That >>>>>> is exactly what he is now seeking to do. >>>>>> >>>>>> *Status of 208 & 702* >>>>>> >>>>>> PR 208 has been feature-complete and testable since early September. >>> It >>>>>> has been adopted by more than 1000 users, who I have been supporting >>> for >>>>>> more than six months. The code has not undergone any major changes >>>> since >>>>>> September. There are no known bugs, and no outstanding feature >>> requests >>>>>> that can be satisfied without major changes to the Zeppelin >>>> architecture. >>>>>> >>>>>> 208 does *not* fail CI. 208 includes extensive unit tests of the >>>> R-Spark >>>>>> integration because this turned out to get broken by changes in >>> Zeppelin >>>>>> often. Because CI is unable at present to provide a consistent >>>>>> environment, 208's *OWN UNIT TESTS*, which pass when run on an >>> ordinary >>>>>> machine, fail when run on CI. >>>>>> >>>>>> 208 does need a push for compatibility with a recently adopted PR -- >>>> that >>>>>> is work I've essentially completed, but have not pushed. >>>>>> >>>>>> PR 702 is a re-design based on 208 -- not just architecture, but right >>>> down >>>>>> to the choice of demo images, which were taken from 208's >>> documentation. >>>>>> In fact, 702 has had been re-engineered several times to more closely >>>>>> conform to 208's architecture and feature set. But 702 still remains >>>>>> feature-incomplete -- it cannot handle the range of visualizations, R >>>>>> classes, etc., that 208 can. It is not stable code, and shows no signs >>>> of >>>>>> stabilizing any time soon. >>>>>> >>>>>> No-one has adopted 702. It has changed radically, fundamentally, at >>>> least >>>>>> 4 times over the past two months since it was submitted. One of those >>>>>> changes was only days ago. >>>>>> >>>>>> 702 also has no proper tests, which is the excuse for not merging 208. >>>> 702 >>>>>> has things labelled "tests," but they don't actually attempt to >>> connect >>>> to >>>>>> R or Spark, which are the things that break and which therefore need >>>>>> testing. >>>>>> >>>>>> *** >>>>>> >>>>>> I would like credit for my own work and design. I think I have more >>> than >>>>>> earned that. >>>>>> >>>> >>> > >
