1.2.1 release notes

[anirudh2290]

Description

I have added more info to 1.2.1 release notes from what @mli sent on the mailing list.

Please help review: Once this is approved , I will make the new release with the same content.

@mli @piiswrong @ThomasDelteil @szha @marcoabreu

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

• The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage:
• Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
• Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
• Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
• Code is well-documented:
• For user-facing API changes, API doc string has been updated.
• For new C++ functions in header files, their functionalities and arguments are documented.
• For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
• Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
• To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

• Feature1, tests, (and when applicable, API doc)
• Feature2, tests, (and when applicable, API doc)

Comments

• If this change is a backward incompatible change, why must this change be made.
• Interesting edge cases to note here

[ThomasDelteil]

"Although, this is not an use-case that the API was meant to support, it won't break your user code" => I think this confuses more than inform (and would just remove it), people that are using it already know it works for them, and shouldn't expect it to break, but ideally we would like to encourage to move to export/imports

[mli]

It will be great if Aaron can have a look, I feel the current release note may be too complicate for beginner users to read

[mli]

not within a name scope

[anirudh2290]

good catch .. will fix.

[mli]

Suggest to move this section on the top of this release.

[anirudh2290]

will do ..

[anirudh2290]

actually the deprecation section gives some background before introducing the user code changes section. I am not sure if deprecation under user code changes would make sense. I can remove the lines starting at "If your model is hybridizable" to make it more clear. What do you think ?

[anirudh2290]

@aaronmarkham can you please take a look.

[aaronmarkham]

Some clarifications and suggestions....

[aaronmarkham]

Has the "gluon book" been updated? Was it TSD? Maybe be specific about which it was.

[aaronmarkham]

led to MXNet users depending --> led MXNet users to depend

[aaronmarkham]

Don't make this a bullet since it's the only thing.

[aaronmarkham]

Create a new paragraph after the intro (after 'hack to break.')

[aaronmarkham]

Sigh new APIs using the same names - just slight variations of the same word - that have different functionalities!? Too late for my feedback on that I guess.

[anirudh2290]

I don't think we say use save_params and load_params in the tutorial. We are removing that. You are right, better to remove the PR and issue.

[anirudh2290]

I still see the old version in the gluon book (https://gluon.mxnet.io/chapter07_distributed-learning/hybridize.html) Thomas opened a PR for correcting it.zackchase/mxnet-the-straight-dope#498 @ThomasDelteil do you know when this change will be deployed ?

[anirudh2290]

Its risky to link to the gluon book because when users look at it they wont know if it is the correct version or the incorrect one.

[ThomasDelteil]

@anirudh2290 I don't know. @zackchase @mli how do we update the straight dope website?

[anirudh2290]

I have included the github gluon link - https://github.com/zackchase/mxnet-the-straight-dope/blob/master/chapter07_distributed-learning/hybridize.ipynb which is updated to the correct apis. Hopefully, users will look at git history to figure that it was wrong and corrected.

[aaronmarkham]

Instead of linking [here], say please refer to the Saving and Loading Gluon Models tutorial.

[anirudh2290]

will do.

[nswamy]

Please remove the semver exception from Release notes

[nswamy]

I don't think we need to explain we are making exception to SemVer. Why is it important for the user to know as a part especially as a part of Release notes

[anirudh2290]

This was suggested by @ThomasDelteil and the reasoning was that some users may depend on version numbers to decide whether it is a breaking change or an addition or a bug fix based on versioning.

[szha]

@nswamy violation of our own rule without even explanation seems like a good way of losing trust.

[anirudh2290]

I understand @nswamy 's point though. There may be users who don't understand what semantic versioning and for them this piece of text is just confusing also given that our earlier releases never had it. Which group of users is bigger is debatable and I don't have data to prove either way.

[szha]

Then why don't we make the text more self-contained by including a link to the definition of semantic versioning? This is about our own stance, and not at all about group sizes.

[nswamy]

@szha this is not about hiding anything..I am suggesting we keep the release notes just enough.

[ThomasDelteil]

@nswamy let's try to work towards a compromise here. I think there are valid concerns that originated from @szha. I think factually it is true that we are doing an exception to semantic versioning, and I think it is important for our users to see that we know it and take this seriously and are transparent about it, to increase the trust in the future releases.

@nswamy Do you feel strongly that this should be removed? Because I think some others in the discussion really feel strongly that it should stay. If it's just that you think it is adding unnecessary details, I would suggest to leave it as it is, and would suggest removing your veto. For transparency and generally documentation cases, I think it is better to have too much than too little.

[nswamy]

@ThomasDelteil Release notes is not a place to add these unnecessary information. If people feel strongly raise it else where, why are we burdening the user to read abut SemVer, why is it important for the user to know why you made an exception.
I would like this information to be dropped from the Release notes for our Users sake, there will no end to what goes in release notes.

[nswamy]

@ThomasDelteil @szha adding a Breaking Change section is more appropriate in the Release Notes(which I think is very well explained already)

[marcoabreu]

It might be rephrased, but I agree that we need something with justification to express that this was a one-time-off (other frameworks make a lot of breaking changes with minor releases). Having a stable API gives us a lot trust and is often a major point for users. We had customer using 0.11 although 1.1 was released just because they were afraid of upgrading. Same for 1.0 users although 1.2 is released - we should make it our highest priority that we gain trust in our versioning and explain it in quite detail if we diverge from it. Ideally, we'd be like Apple and basically have everybody upgrade as soon as an update gets released. Otherwise, we'll always have to keep supporting old versions although we officially say that we don't.

If people are not interest in the background, they're free to skip it. I'd rather err on the side of caution here and expect our users to have proper knowledge about versioning.

[anirudh2290]

ping @mli @aaronmarkham @ThomasDelteil @nswamy for another round of reviews.

[aaronmarkham]

<new line>
To allow for backward compatibility and to fix this breaking change, save_params and load_params APIs have been reverted to their previous behavior as specified in v1.1.0. With v1.2.1, usage of these two APIs will resume their former functionality and a deprecation warning will appear. The functionality introduced to these APIs in v1.2.0 is now renamed to save_parameters and load_parameters respectively.
<new line>
All scripts to save and load parameters for a Gluon model should use the new API for save_parameters and load_parameters. If your model is hybridizable and you want to export a serialized structure of the model as well as parameters you should migrate your code to use export API and the newly added imports API instead of save_params and load_params APIs.

Note: At this point I'm left wondering what the difference is between export and save_parameters. Same for import and load_parameters... Both of these are new and "recommended", but I don't understand what the difference is.

[anirudh2290]

should I Link this doc: https://mxnet.incubator.apache.org/tutorials/gluon/save_load_params.html to better explain the difference ?

[nswamy]

why are we blaming the user that they developed a hack/incorrect usage ? If its a API that is available to them they can find creative ways to use it. It was also advertised in the gluon tutorials. The onus is on us to either hide or indicate private methods.

[anirudh2290]

@srochel your changes have been incorporated.

Requesting to change first paragraph

[szha]

There's factual problem in this PR.

[szha]

I disagree with the assessment above.

[ThomasDelteil]

can you please pinpoint which part is not factual in your opinion?

[safrooze]

The tutorial that NEWS.md points to for "more information" only mentions save_params() and load_params().

[szha]

For example, load_params was not reverted to the behavior of 1.1.0 in 1.2.1. I think Anirudh should already be aware of it.

[anirudh2290]

The statement talks about save_params, load_params in conjunction. I think the revert statement makes sense for our users.
From the user point of view, if they were using save_params/load_params to save their model architecture and params, to load it back later into Symbolblock, they know that the their codes which broke in 1.2 will not break in 1.2.1. They are also informed that they should use export and imports API. For users who used it to create layers without namescope in v1.2.0, they know that this feature didn't work with save_params and load_params in v1.1.0 so they will switch to save_parameters and load_parameters.

[szha]

I already did above. Please update.

[anirudh2290]

I have already explained that (save_params \ load_params) is in conjunction and why the revert statement makes sense to our users.

[szha]

@anirudh2290 you asked for my specific concerns, so I explained that my concern is false statement, and I offered alternative that doesn't require false statement.

Seems weird to me that the justification for making false statement is because it "makes sense to our users".

[anirudh2290]

How is it false that the behavior of save_params/load_params API has been reverted to the behavior of 1.1.0 ? Does the line say that load_params API has been reverted to 1.1.0 ?

[szha]

The code for load_params (and its behavior) wasn't reverted to what's in 1.1.0, for the best compatibility.

Maybe I misunderstood what you were attempting to say with the distinction between "API has been reverted to the behavior of 1.1.0" v.s. "API has been reverted to 1.1.0". I was treating them as equivalent in this case. So please bear with me :)

My suggestion would be to provide explanation as faithful to the code as possible, so that the explanation would always be true regardless of the users' level of understanding. Besides that, you may add whatever you think would benefit the users as you see fit.

[szha]

Since we're making an inconvenience to the users, it's probably a good idea to state that "We sincerely apologize for the inconvenience, and we as a community will continue to uphold semantic versioning."

[srochel]

Suggest:
We did break the promise made with semantic versioning due to the API behavior change in 1.2.0 and the backward incompatible change between 1.2.0 and 1.2.1 patch release. The breaking use case is documented in point 2 above. The reason for doing this is because the 1.2.0 release broke a documented use case from the gluon book and this release reverts the breakage. As a community we are apologizing for the inconvenience caused and will continue to strive to uphold semantic versioning.

[anirudh2290]

i have made the change.

concerns addressed.