mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sheng Zha <szha....@gmail.com>
Subject Re: [VOTE] Release MXNet version 1.3.0.RC0
Date Tue, 04 Sep 2018 23:51:43 GMT
Hi Hagay,

You asked, "It can be fixed and included in the release alongside the rest
of the release content, right?"

Yes, it can, after it has appropriate approval and merged to master, and at
the cost of restarting the vote.

However, personally, I do not think there's enough justification for this
patch to stop the release, given that:
1. this is not a regression, so 1.3 is not in a worse shape than any prior
releases, in the area that this patch addresses.
2. the attempt of putting in this patch does not respect the code freeze
time that the community agrees.
3. we are not stopping this issue for any of the other 139 open bug reports
[1] and you did not provide an argument that fixing this bug is more
important than fixing any of those 139 bugs.

Finally, your first claiming that the fix "is ready to be cherry picked
into the release branch" when it's not, and then moving on to "I think it
is waiting for your review", this flow makes me uncomfortable. If you'd
like to imply that I'm blocking the merge of that patch, I'm not. As you
may not realize, I have other work to do as many committers do. Given your
status as an engineering lead at Amazon, you can probably get immediate
help if you ask the committers on your team.

[1]
https://github.com/apache/incubator-mxnet/issues?page=2&q=is%3Aissue+is%3Aopen+label%3ABug

On Tue, Sep 4, 2018 at 4:14 PM Hagay Lupesko <lupesko@gmail.com> wrote:

> Hi Sheng,
>
> Addressing your questions:
>
> - "why this specific bug is more important than all the other known bugs,
> that this becomes a release blocker"
> I do not consider it to be more or less important than other fixes. It can
> be fixed and included in the release alongside the rest of the release
> content, right?
> From the description of the issue it seems important since it is blocking
> users from loading models that were previously trained and saved. There is
> nothing stopping the community from including this fix into 1.3.0,
> alongside the rest of the features and fixes.
>
> - "The bug exists since SymbolBlock was introduced a year ago and has
> survived at least three releases, so this is not a regression."
> I do not think I said it is a regression. However, the fact a bug existed
> before, does not mean it is OK to release it rather than fix it.
>
> - "Timeline-wise, this bug was reported on 7/21, but was not reported as
> release-blocker in the release discussion thread until 8/31 [1]. Neither
> its reporting as release-blocker nor its fix made it for the 8/3 code
> freeze."
> You are right, would have been better to have this identified and fixed
> earlier and included before code freeze.
>
> - "The PR is still not ready yet as it doesn't have approval."
> I think it is waiting for your review.
>
> - "it would be great if you could provide some additional reasoning besides
> "X mentions the issue" or "fix was done by X""
> I have. Repeating what I wrote in my previous email for clarity: Basic
> functionality broken: loading a model (albeit one that that was saved as
> non FP32)
>
> So, yes - this issue seems to have been out there for a while, somehow went
> under the radar... but I think the key question is whether this blocks a
> basic functionality in MXNet. I believe so, hence my -1 vote.
>
> Hagay
>
> On Tue, Sep 4, 2018 at 1:19 PM Sheng Zha <szha.pvg@gmail.com> wrote:
>
> > Hi Hagay and Sandeep,
> >
> > Could you help us understand why this specific bug is more important than
> > all the other known bugs, that this becomes a release blocker?
> >
> > Some facts to consider:
> > - The bug exists since SymbolBlock was introduced a year ago and has
> > survived at least three releases, so this is not a regression.
> > - Timeline-wise, this bug was reported on 7/21, but was not reported as
> > release-blocker in the release discussion thread until 8/31 [1]. Neither
> > its reporting as release-blocker nor its fix made it for the 8/3 code
> > freeze.
> > - The PR is still not ready yet as it doesn't have approval.
> >
> > Hagay, it would be great if you could provide some additional reasoning
> > besides "X mentions the issue" or "fix was done by X". Thanks.
> >
> > -sz
> >
> > [1]
> >
> >
> https://lists.apache.org/thread.html/d1ed611f98c20d5d85c294b0c07c8bdebca13a209cf66a3872c9123e@%3Cdev.mxnet.apache.org%3E
> >
> > On Tue, Sep 4, 2018 at 12:39 PM Hagay Lupesko <lupesko@gmail.com> wrote:
> >
> > > Sandeep mentions the issue of an error when user tries to load model
> > params
> > > trained/saved as FP16.
> > > https://github.com/apache/incubator-mxnet/issues/11849
> > > The fix was done by Sandeep:
> > > https://github.com/apache/incubator-mxnet/pull/12412 and is ready to
> be
> > > cherry picked into the release branch.
> > >
> > > This seems like a release blocker to me:
> > > - Basic functionality broken: loading a model (albeit one that that was
> > > saved as non FP32)
> > > - Reported by 3 users (wgchang@, nicklhy@ and ThomasDelteil@)
> > >
> > > -1 (non binding)
> > >
> > > Hagay
> > >
> > >
> > >
> > > On Tue, Sep 4, 2018 at 12:01 PM sandeep krishnamurthy <
> > > sandeep.krishna98@gmail.com> wrote:
> > >
> > > > "- 0"
> > > >
> > > > I believe the bug #11849
> > > > <https://github.com/apache/incubator-mxnet/issues/11849>, unable
to
> > > import
> > > > non-fp32 models into Gluon, fixed in this PR #12412
> > > > <https://github.com/apache/incubator-mxnet/pull/12412> is important
> > for
> > > > the
> > > > users. I would rather pick this fix in this release than plan a minor
> > > > release later.
> > > >
> > > > Best,
> > > > Sandeep
> > > >
> > > >
> > > >
> > > > On Mon, Sep 3, 2018 at 2:34 PM Philip Cho <
> chohyu01@cs.washington.edu>
> > > > wrote:
> > > >
> > > > > Actually, the command "git clone --recursive
> > > > > https://github.com/apache/incubator-mxnet -b 1.3.0.rc0" works fine
> > > now,
> > > > > never mind.
> > > > >
> > > > > On Mon, Sep 3, 2018 at 1:45 PM Philip Cho <
> > chohyu01@cs.washington.edu>
> > > > > wrote:
> > > > >
> > > > > > Unfortunately, MXNet was depending on a branch of TVM that is
now
> > > > > deleted.
> > > > > > We will have to merge #12448
> > > > > > <https://github.com/apache/incubator-mxnet/pull/12448>
before
> the
> > > > > release.
> > > > > >
> > > > > > Background: See dmlc/tvm#1394 <
> > > https://github.com/dmlc/tvm/issues/1394
> > > > >.
> > > > > >
> > > > > > Philip.
> > > > > >
> > > > > > On Mon, Sep 3, 2018 at 7:26 AM Carin Meier <carinmeier@gmail.com
> >
> > > > wrote:
> > > > > >
> > > > > >> Checked out the tag, built and tested the Clojure package.
+1
> > > > > >>
> > > > > >> On Fri, Aug 31, 2018 at 10:59 PM Roshani Nagmote <
> > > > > >> roshaninagmote2@gmail.com>
> > > > > >> wrote:
> > > > > >>
> > > > > >> > Hi all,
> > > > > >> >
> > > > > >> > I would like to propose a vote to release Apache MXNet
> > > (incubating)
> > > > > >> version
> > > > > >> > 1.3.0.RC0. Voting will start now (Friday, Aug 31st)
and end at
> > > 7:00
> > > > PM
> > > > > >> > PDT, Wednesday, Sept 5th.
> > > > > >> >
> > > > > >> > Link to release notes:
> > > > > >> > https://github.com/apache/incubator-mxnet/releases
> > > > > >> >
> > > > > >> > Link to release candidate 1.3.0.rc0:
> > > > > >> > *
> > https://github.com/apache/incubator-mxnet/releases/tag/1.3.0.rc
> > > > > >> > <
> > https://github.com/apache/incubator-mxnet/releases/tag/1.3.0.rc0
> > > > >0*
> > > > > >> >
> > > > > >> > View this page, click on "Build from Source", and use
the
> source
> > > > code
> > > > > >> > obtained from 1.3.0.rc0 tag:
> > > > > >> > https://mxnet.incubator.apache.org/install/index.html
> > > > > >> >
> > > > > >> > Please remember to TEST first before voting accordingly:
> > > > > >> >
> > > > > >> > +1 = approve
> > > > > >> > +0 = no opinion
> > > > > >> > -1 = disapprove (provide reason)
> > > > > >> >
> > > > > >> > Thanks,
> > > > > >> > Roshani
> > > > > >> >
> > > > > >>
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Sandeep Krishnamurthy
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message