+1 On Tue, Jun 26, 2018 at 5:50 PM Anirudh <[email protected]> wrote:
> Hi all, > > The current warning message for save_params: "save_params is deprecated, > use save_parameters instead" is misleading > for users who may use the API to load into SymbolBlock. To make it clearer > for all users, a better warning message is to include alternative to > save_params and reference to detailed documentation. > The message improvement is important, but not blocking to move forward to > complete the voting process for MXNet v1.2.1 patch release. > We plan to follow up with a 1.2.2 patch release to improve the message and > potentially include other bug fixes. > Please let me know if you have any thoughts, questions or suggestions. > > > Anirudh > > On Mon, Jun 25, 2018 at 10:44 PM, Anirudh <[email protected]> wrote: > > > > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > >> 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 <[email protected]> > >> > 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 <[email protected]> > 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 <[email protected]> > >> > >> 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 > >> > >>>>> > >> > >>>> > >> > >>> > >> > >> > >> > > >> > > > > >
