Kos & Moon -- The gist of this thread, is that people disagree with what Moon has said regarding code quality, whether 208 breaks CI (and if so, why), and whether its appropriate to merge 702, as Moon has proposed.
Since this saga started, we've had 5 threads to get the sense of the community on what to do. All of those came out the same way. More than a dozen people have asked for the same thing. Isn't it time to just get this done so we can all move on? (If Moon believes there's a real CI issue here, I have no doubt that it will be solved an hour after merge --- as Moon undertook to do back in December.) On Tue, Mar 29, 2016 at 1:53 PM, moon soo Lee <m...@apache.org> wrote: > Hi, > > Regarding CI test about 208, > > Zeppelin have several profiles for CI test. Each profile tests Zeppelin > with different Spark Version. Also it different profiles different level of > tests (integration test using selenium). > > Current status of 208 in CI test is, passing single profile, fails all > other profiles. Which is exactly the same status that i have helped 208 few > months ago by the way. see. > > https://github.com/apache/incubator-zeppelin/pull/208#issuecomment-173423103 > > 208 has some code interacts with Spark. And 7 CI profile out of 8 are for > test code against various version of Spark. While Zeppelin used to supports > multiple version of Spark, from range of 1.1 ~ 1.6. > > SparkInterpreter (scala) > PySparkInterpreter (python) > SqlInterpreter (spark sql) > > supports all versions of spark in the profile (pyspark supports from 1.2). > I think it's very strait forward to expect the same quality for R > interpreter. > > I can suggest two possible way, > > - Keep working on make all profile of CI green. While 208 already passes > one profile and test in all other profiles are the same but only against > different spark version, it won't be that difficult to make it pass all > other profile. > - Or activate 208 only for spark 1.6 and mark/document which is minimum > version requirement of spark. Like Pyspark interpreter did (requires spark > 1.2 or newer). > > > Regarding code merge policy, > > Zeppelin community had been make sure CI pass before merge in to master, > since it's beginning, until now. That's i believe another consensus that we > believed we have in the community. > > That's only reason keep spoken why 208 is not merged for last 4 months. > And only reason for all other PR forced to make CI green before it get's > merged. > > Personally i think not breaking master branch is valuable while that makes > any contributor start contribution safely at any point from master branch. > And users who want to try latest community work can safely test Zeppelin > from master branch. > > But if anyone think Zeppelin community need to discuss about it, please > start a discussion. I'm happy to see discussion happens. > > Thanks, > moon > > > On Tue, Mar 29, 2016 at 9:31 AM Konstantin Boudnik <c...@apache.org> wrote: > > > hmm.... that's getting weird again. So, far I failed to see: > > - CI issues being addressed. If the consensus of the community to merge > in > > something, break the CI and collect the technical debts - that's fine > > and > > that's your choice (I am not here to pass the judgement on the quality > > of > > the code) > > - a consensus to keep anyone away from _anything_ in the project > matters. > > Please do not impose your personal desires on the others. While you're > > entitled to express them (free speech and all that), no one is > entitled > > to > > listen, less oblige by it (based on the same principles of individual > > rights). > > > > So, please keep it civil and find a way to improve the code, if needed, > > and get > > it in once the committers are satisfied with its quality. > > > > Cos > > > > On Tue, Mar 29, 2016 at 11:51AM, Amos B. Elberg wrote: > > > Moon - no. That is the opposite of what people are saying. > > > > > > I started this thread because I feel that you are disregarding the > > consensus > > > of the community. > > > > > > The thread asks for two things - 208 to be merged without further > delay, > > and > > > for you to stay out of the issue of R interpreters entirely. 702 can > be > > > addressed after 208 is merged. > > > > > > How many more people do you need to hear from? > > > > > > > On Mar 29, 2016, at 5:40 AM, moon soo Lee <m...@apache.org> wrote: > > > > > > > > Hi folks, > > > > > > > > I didn't see anyone disagreement merge 208 and/or 702 in this thread > > and > > > > previous thread [1], as they're ready. while they both have technical > > > > merits as Jeff summarized really well. > > > > > > > > Now i can see 208 finally made some progress on CI [2]. Hope continue > > the > > > > work and make CI green. Also I can see 702 is trying to finishing up > > and > > > > waiting for CI become green. > > > > > > > > I don't want to merge something that breaks CI. If then, it becomes > > make > > > > very difficult to verify all other contributions. Other contributions > > are > > > > as important as these two. Hope community can understand that. > > > > > > > > Considering recent progress of both contributions, i expect they'll > be > > > > ready anytime soon. And then we can finally merge them. > > > > > > > > About merging 702, 208 contributions, does this sounds clear? > > > > > > > > If they're both merged, It's possible to improve both RInterpreter by > > > > taking each others advantage. Therefore, no reason to worry at this > > point > > > > about which one is better, which one has advantages, which one will > > merge > > > > before the other, etc. Both have pros and cons and both will help > > Zeppelin > > > > thankfully. > > > > > > > > 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/208#issuecomment-202682652 > > > > > > > > > > > >> On Tue, Mar 29, 2016 at 1:45 AM enzo < > e...@smartinsightsfromdata.com> > > wrote: > > > >> > > > >> 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. > > > >> > > > >> > > >
