From dev-return-3227-archive-asf-public=cust-asf.ponee.io@mxnet.incubator.apache.org Fri Jun 15 23:36:17 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id D51AF180636 for ; Fri, 15 Jun 2018 23:36:16 +0200 (CEST) Received: (qmail 58335 invoked by uid 500); 15 Jun 2018 21:36:15 -0000 Mailing-List: contact dev-help@mxnet.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mxnet.incubator.apache.org Delivered-To: mailing list dev@mxnet.incubator.apache.org Received: (qmail 58323 invoked by uid 99); 15 Jun 2018 21:36:15 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd4-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 15 Jun 2018 21:36:15 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd4-us-west.apache.org (ASF Mail Server at spamd4-us-west.apache.org) with ESMTP id CA85CC044E for ; Fri, 15 Jun 2018 21:36:14 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd4-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.442 X-Spam-Level: * X-Spam-Status: No, score=1.442 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_ENVFROM_END_DIGIT=0.25, FREEMAIL_REPLY=1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-1.697, SPF_PASS=-0.001, T_DKIMWL_WL_MED=-0.01] autolearn=disabled Authentication-Results: spamd4-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd4-us-west.apache.org [10.40.0.11]) (amavisd-new, port 10024) with ESMTP id EzfrI8Jcv2AC for ; Fri, 15 Jun 2018 21:36:12 +0000 (UTC) Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTPS id 5935B5F1EB for ; Fri, 15 Jun 2018 21:36:12 +0000 (UTC) Received: by mail-wr0-f195.google.com with SMTP id x4-v6so11176014wro.11 for ; Fri, 15 Jun 2018 14:36:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=fLoTsEl3RZD3A4ab0Zmv2KfygsxpZLI7MkKrfyozpMM=; b=OrH0T3/SpyOj9wqJduIu739Bd4e/pe2Yy9UuWsGlCRV+uCQB3yjrobWnHEJ1F3fblh F4t8r5xSNvQLKeFC80NJQ6vPfSzO91yfB4vbxTwLPsJ/vjWzFVt0+mVjqbJlvwbfuH5H ul2XS/5DAmWvazWTIF3JULm53IUkxgwB4y/EYHL8KrXGQxn9KrRqTgzhOSbtLUrcnfL1 Rz8CvKXI1+1WR6GoA3HIQt3lvQP52fV7i4AsGVqkPvU2q73k2oF7FqSZYje2dmdr8i18 uIqSp/Jot+vb8FCQZ7lCmxi//UY1GCnh8F1I8xirGy00WalXUwNnRpM808lP3CZhToXM Ns6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=fLoTsEl3RZD3A4ab0Zmv2KfygsxpZLI7MkKrfyozpMM=; b=Sp/yH3sTQvvPFuEFmBlA5h47ScBXIFQLkhf0SspjebL8BO6ma9VrLeIpwGJnwVdx32 YwwteAwusR3mlmimAiIUcmi3JPUvC0GKcWCAZA/DWmvYpyIP6Jzn7Dm/2vIvuexvWq3R JxEjHNWzAlxRRbm0afvKryy6aRv6yfipBCc9QIgNSEc5MJfnlPcMVC/Jxhn+cdsFPuQf JvIeOAK/oBqmOZdOWbymFtvCOy4Qap3NbloxY3cPHm7FNUdEvGoMG3gZyL2KgyENLMcq 3kLOIjzRNWFIDz2xt4FqbHnJjgc9PLIctMlWJE7KiTpDtAeJXIA2aCot0qymWU+mkLaE m4Aw== X-Gm-Message-State: APt69E0UdzMTRsObVDvHd/Rj7wXb05nRJqNZ+8baSw+5SRb71hliM33O aBTUL3MvyCufOoEkLC9eCdkoBSgwsISbcq0EQG3YZ3dp X-Google-Smtp-Source: ADUXVKIsUUfMDJ/JX3uU/pzDYu5iS2dElikzm6KWdRlJeezS6InyLUogXjTg2pbc7+wbm6DJKDMu+Ipn/25vw2sC14E= X-Received: by 2002:adf:a319:: with SMTP id c25-v6mr2752317wrb.189.1529098571068; Fri, 15 Jun 2018 14:36:11 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:adf:af73:0:0:0:0:0 with HTTP; Fri, 15 Jun 2018 14:36:10 -0700 (PDT) In-Reply-To: References: <9387579A-D717-497B-BE22-F14929820B8D@amazon.com> From: Thomas DELTEIL Date: Fri, 15 Jun 2018 14:36:10 -0700 Message-ID: Subject: Re: Reverting pull request To: dev@mxnet.incubator.apache.org Content-Type: multipart/alternative; boundary="00000000000027bcda056eb501cf" --00000000000027bcda056eb501cf Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Copying a comment I made on a flaky test introduced by this PR: https://github.com/apache/incubator-mxnet/issues/11171 " @piiswrong you introduced this test in this commit [WIP] Do Not Merge. Static memory allocation for cached_op (#10817 ) 2dbd143 and it seems to be flaky. I have seen it failing a few times in recent builds. Can you take a look? e.g http://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-= mxnet/detail/master/1002/pipeline/ Given all the talk about quality contributions going on the dev mailing list, I am a little unsettled by the fact this PR went undocumented (no design docs or explanation in the PR), unreviewed (1 question ignored), the optimization wasn't tested or benchmarked (it was actually making things slower), and the code was self-merged. Should we enforce that each PR , especially the ones that introduce a significant number of changes be properly documented and reviewed before merging? " My main gripe is that there is literally no description of what the PR is achieving, no design docs to refer to. I spent time benchmarking the different optimization because I thought I did something wrong when I found that it was slower with the optimization. I would have hoped that a significant change like that would have at least been benchmarked. In French we say "Il ne faut pas confondre vitesse et precipitation" which loosely translates to "Haste makes waste". I would advocate to take the time to document PRs properly, benchmark and thoroughly test significant changes. Because down the line, other users end up losing so much more time trying to figure out what is happening when things go wrong (like here). Thanks, Thomas 2018-06-15 14:27 GMT-07:00 Marco de Abreu < marco.g.abreu@googlemail.com.invalid>: > If it causes issues, I'd like to invite everybody to direct their request= s > to Eric since he merged the PR prematurely. The committer who merges a PR > is responsible and can be held liable for any negative impact being the > result of their action [1]. > > [1]: https://www.apache.org/dev/committers.html#committer-responsibilitie= s > > On Fri, Jun 15, 2018 at 2:23 PM Zheng, Da > wrote: > > > +1 The PR has been merged a while ago, so it has been tested by many > > people. > > Other people's work now depends on this PR. Reverting it at this point > can > > cause a lot of problems for many other people. > > > > Best, > > Da > > > > =EF=BB=BFOn 6/15/18, 2:18 PM, "workcrow@gmail.com on behalf of Tianqi C= hen" < > > workcrow@gmail.com on behalf of tqchen@cs.washington.edu> wrote: > > > > +1 We would be stuck at local minimums if we just keep reverting > the > > PR > > that brings improvements in the long term > > > > Tianqi > > > > On Fri, Jun 15, 2018 at 2:15 PM, Mu Li wrote: > > > > > Why reverting instead of fixing the bugs? Static memory aims to > > reduce > > > memory allocation, it's a key feature to bridge the perf gap > between > > gluon > > > and symbol. > > > > > > On Fri, Jun 15, 2018 at 2:06 PM, Marco de Abreu < > > > marco.g.abreu@googlemail.com.invalid> wrote: > > > > > > > Hello, > > > > > > > > I'm reverting https://github.com/apache/ > incubator-mxnet/pull/10817 > > as of > > > > https://github.com/apache/incubator-mxnet/pull/11311 due to > > regressions > > > > described in > > https://github.com/apache/incubator-mxnet/issues/11171 and > > > > https://github.com/apache/incubator-mxnet/pull/10817. > > > > > > > > The pull request has been self-merged without proper review and > > > introduced > > > > regressions. Committers should act as role models in this proje= ct > > and > > > > adhere to software engineer best practices. > > > > > > > > Best regards, > > > > Marco > > > > > > > > > > > > > > --00000000000027bcda056eb501cf--