Hi Mu, Thanks for bringing this up and hopefully this should answer Sheng's question. Thomas pointed out something similar in the PR here for the warning message which I didn't notice back then: https://github.com/apache/incubator-mxnet/pull/11127
Not sure about the reasoning to not add it and if there was an offline discussion about this between Thomas and Erik. It would be nice if you guys could pitch in if there were any strong reasons. I understand that a more informed warning when using save_params would really avoid some customer frustration. Having said that, I am a little worried about the timeline though since some customers are eagerly waiting for the release of 1.2.1. Another RC would delay it by at-least one and a half weeks. Anirudh On Mon, Jun 25, 2018 at 9:54 PM, Mu Li <muli....@gmail.com> wrote: > Detailed documents should help, but the current warning message that > "save_params is deprecated, use save_parameters instead" is not sufficient > enough. > > Some details about the API changes: > > 1. v1.2 changed the implementation of "save_params", which is explained in > the release note. The main benefit for this change is that we don't need to > create layers within a name scope. [1] > 2. we found this change breaks a gluon-to-symbol usage, even though we > recommended users to use "export" for this usage. [2] > 3. for some good reasons we made a decision to revert save_params in > v1.2.1, and introduced a new API called save_parameters for this new > behavior. [3] > > Since calling save_params each time will generate a warning message, it's a > major API change. The recommended for users to update their codes are: > > 1. If you save parameters to load back into a SymbolBlock, you can use > export instead, though keeping it will not break your codes except for a > warning message. (But it will break in v1.2) > 2. If you create gluon layers without a name scope, you must replace > save_params with save_parameters. Otherwise, your model cannot be loaded > back in v1.2.1 (though it works in v1.2) > 3. For the rest case, such as models are created within a name scope, and > the models are loaded into gluon (not symbolblock) later, recommend > replacing save_params with save_parameteres. If you don't do it, nothing > will break in v1.2 and v1.2.1, but v1.2.1 will give you a warning message. > > This API changes in v1.2 and v1.2.1 are pretty tricky. Anirudh did a great > job in capturing them in release notes. But I feel it's hard for users to > understand the impacts. I suggest to improve the warning message to "use > export if you want to load into SymbolBlock, otherwise use save_parameters. > For more details, refer to this URL". > > [1] https://github.com/apache/incubator-mxnet/releases/tag/1.2.0 > [2] https://github.com/apache/incubator-mxnet/issues/11091 > [3] https://github.com/apache/incubator-mxnet/pull/11127 > > On Mon, Jun 25, 2018 at 9:23 PM, Sheng Zha <szha....@gmail.com> wrote: > > > Wouldn’t this break users who are on 1.2.0 and used our API correctly? > Why > > do we have to revert load_params, given that it’s backward compatible? > > > > -sz > > > > > On Jun 25, 2018, at 6:30 PM, Anirudh <anirudh2...@gmail.com> wrote: > > > > > > Hi, > > > > > > 1.2.1 (load_params) is backward compatible with 1.1.0 not with 1.2.0. > > > It does not adhere exactly with semver but it had to be made, to > quickly > > > help our customers who were using the APIs incorrectly. > > > > > > Anirudh > > > > > >> On Mon, Jun 25, 2018 at 5:42 PM, Sheng Zha <szha....@gmail.com> > wrote: > > >> > > >> save_parameters didn't exist in 1.2.0 so its addition usually isn't > > >> supposed to happen in a patch release if we stick to semantic > > versioning. I > > >> couldn't find a discussion on this exception. Did it happen? > > >> > > >> Would people who used 1.2.0 to save models be able to load parameters > in > > >> 1.2.1 using the reverted load_params? (i.e. is it backward compatible) > > >> > > >> -sz > > >> > > >> > > >>> On Mon, Jun 25, 2018 at 4:07 PM, Anirudh <anirudh2...@gmail.com> > > wrote: > > >>> > > >>> Hi Mu, > > >>> > > >>> The warining currently printed is "save_params is deprecated. Please > > use > > >>> save_parameters." > > >>> Isn't this similar to what you are suggesting ? > > >>> > > >>> Anirudh > > >>> > > >>>> On Mon, Jun 25, 2018 at 3:47 PM, Mu Li <muli....@gmail.com> wrote: > > >>>> > > >>>> v1.2.1 will print a deprecating warning message when calling > > >>>> save_params. We should tell users clearly to replace "save_params" > > with > > >>>> "save_parameters" or something else. > > >>>> > > >>>> On Mon, Jun 18, 2018 at 6:52 PM, Anirudh <anirudh2...@gmail.com> > > >> wrote: > > >>>> > > >>>>> Hi, > > >>>>> > > >>>>> This is the vote to release Apache MXNet (incubating) version > 1.2.1. > > >>>> Voting > > >>>>> will start now and close Thursday June 21st 7:00 PM PDT. > > >>>>> > > >>>>> Link to release candidate 1.2.1.rc0: > > >>>>> > > >>>>> https://github.com/apache/incubator-mxnet/releases/tag/1.2.1.rc0 > > >>>>> > > >>>>> View this page for installation instructions: > > >>>>> > > >>>>> https://mxnet.incubator.apache.org/install/index.html > > >>>>> > > >>>>> (Note: The README.md points to the 1.2.1 tag and does not work at > the > > >>>>> moment). > > >>>>> > > >>>>> Please remember to test first before voting accordingly. > > >>>>> > > >>>>> +1 = approve > > >>>>> +0 = no opinion > > >>>>> -1 = disapprove (provide reason) > > >>>>> > > >>>>> Anirudh > > >>>>> > > >>>> > > >>> > > >> > > >
