fix parameter names in the estimator api

[liuzh47]

Description

This fix is to make parameter names of estimator api consistent. Previously, we have val_metrics, eval_net and evaluation_loss refer to the metrics, model and loss used during validation. To make the names consistent, we change these names to val_metrics, val_net and val_loss. So every parameter shares a common prefix of val_ for validation.

Checklist

Essentials

• 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)
• Code is well-documented:
• For user-facing API changes, API doc string has been updated.
• To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

[liuzh47]

@ptrendx please tag this fix for r1.6.0

[ptrendx]

Sorry, but no. There has to be a time where no more stuff gets into the release. I want to do RC tonight and this PR is not even reviewed. If we need to do RC1 then we can potentially include it (although it changes the API so I'm not sure about that).

[leezu]

@ptrendx It unifies the naming of the API changes introduced in 1.6. It may be useful to include for 1.6. If not, then we shouldn't merge it to master either (as it will break the API between 1.6 and 1.7)

[szha]

@leezu @liuzh91 thanks for making the change. Note that beyond the announced code freeze time, it's first at the discretion of the release manager whether a change will be included. This is because after code freeze, the release manager could have already started the steps for validating the release, packing and signing, so let's be mindful.

Contrib packages, given the experimental nature, are generally not considered stable and thus are not subject to the semantic versioning, so technically it should be OK to go into the next release. Also, we will create branch for the 2.0 API-breaking efforts soon too. On the other hand, it would also be nice to not break this API between 1.6 and the next 1.7 version that some community members want. @ptrendx can decide which way it goes.

[liuzh47]

@ptrendx @leezu @szha Thanks for the reply. I understand the risk of changing parameter names may break existing api. But the parameters evaluation_loss eval_net were just introduced last week, there have not yet much dependencies on these parameters.

Anyway it is ok to merge this fix into a later release.

[leezu]

@szha @liuzh91 while the API is considered experimental, it's not good to break it every release. Thus if it's not merged for 1.6, it's better to keep the inconsistent naming in my opinion.

[sxjscience]

My personal suggestion is to include these into an official release once the whole Estimator stuffs are mature enough. I don't think it's a good practice given that we have already passed the code freeze deadline. Also, is it really urgent to be included into 1.6.0?

[szha]

@sxjscience I think the above discussion already covers your concerns. Let us know if you have more to add.

[sxjscience]

@szha I don't think it covers the concern. When working on this release, it really confuses me what "Code Freeze" means in our package. Is it to say that everything inside contrib will go to the official release?

[szha]

@sxjscience

what "Code Freeze" means

code freeze stands for the date after which the release branch will no longer synchronize with the master branch. After code freeze, inevitably, we could discover issues that need fixing and that's when release managers choose which changes to include.

contrib will go to the official release

contrib modules, just like other modules, are part of the package. The contrib modules are explicitly advertised as experimental and non-stable. It's a contract between the developers and the users so there shouldn't be a need to impose additional limitation on ourselves.

[leezu]

@sxjscience we should avoid changing the API in every release. The reason for backporting these changes to 1.6, is to avoid having the API change in MXNet 1.5, 1.6 and 1.7.. That would be extremely inconvenient for our users.
Ultimately the reason is that an API change was included in 1.6, which was later discovered by @liuzh91 to be problematic. An alternative would have been to revert that change.

[sxjscience]

@leezu. As @szha has explained, "contrib is experimental and is not subject to semantic versioning". Thus, it is valid to change the API inside contrib for 1.x versions. Also, I don't think we should announce Estimator as a stable feature in 1.6.0 because we are still actively developing this functionality.

I expressed my concern here because I think we need to respect @ptrendx who has been doing tons of works for the 1.6.0 release process. We should avoid adding new functionalities after the code freeze time because it's not a good engineering practice even if it's in contrib.

For this PR, it's safe to be merged to master as it just changes the name and the order of the arguments. However, adding it to 1.6.0 starts a bad practice of continuously updating contrib in the code freeze status.

[sxjscience]

@leezu @liuzh91 Which bug does this PR solve? Is there an issue for that?

[leezu]

@sxjscience it is bad practice to release MXNet 1.6 with a breaking change to API compared to 1.5, with the change already being deprecated at the time of release. There are only two options IMO: 1) revert the change and stay with the old API 2) Fix the API before the release.

Even if the feature is experimental, it's bad practice to change the API unnecessarily many times.

In the future, let's go via the revert route. This time, we tried to fix the API before the release.

[leezu]

Changing the name and order of arguments means it's impossible to write library code that supports both 1.6 and development version of mxnet.

[sxjscience]

If it’s in the official API, the argument name/order should be changed before the code freeze period. Thus, in the code freeze period, we have enough time to test the features and fix potential bugs.

From my point of view, having a deadline encourages us to think hard about which functionalities we are going to add in the major release. Are we 100% sure that the Estimator is mature enough and we should encourage the users to use it? What if we later find some other naming issues and submit a new PR? Will the new PR go into major release? We need to respect the code freeze deadline and avoid including unnecessary changes.

For this PR, @ptrendx may decide to include it if he finds it’s appropriate. However, in the future, we should definitely avoid adding functionalities after the code freeze deadline.

[liuzh47]

Changing the name and order of arguments means it's impossible to write library code that supports both 1.6 and development version of mxnet.

I believe changing keyword argument orders will not break existing library codes, will they?

[liuzh47]

@leezu @liuzh91 Which bug does this PR solve? Is there an issue for that?

Validation network and loss function were introduced into estimator api just one week before. I think the naming is not consistent enough so I submit a hotfix for that. I have not opened any issue for this PR.

[liuzh47]

If it’s in the official API, the argument name/order should be changed before the code freeze period. Thus, in the code freeze period, we have enough time to test the features and fix potential bugs.

From my point of view, having a deadline encourages us to think hard about which functionalities we are going to add in the major release. Are we 100% sure that the Estimator is mature enough and we should encourage the users to use it? What if we later find some other naming issues and submit a new PR? Will the new PR go into major release? We need to respect the code freeze deadline and avoid including unnecessary changes.

For this PR, @ptrendx may decide to include it if he finds it’s appropriate. However, in the future, we should definitely avoid adding functionalities after the code freeze deadline.

Since contrib package is highly experimental, why do we need to be 100% sure the api is mature enough? The previous design of estimator api on model validation is unsophisticated, so we introduce many bug fixes as new features into the current estimator api. In these bug fixes, we introduce new variables evaluation_loss, val_metrics and eval_net. My concern is that these new introduced variables have inconsistent prefixes and users may complain about the naming. So we open this PR to unify naming of these arguments.

[sxjscience]

@liuzh91 There’s no problem in this PR, which will be merged into master. The problem is that we should not keep updating the 1.6.0 branch after the code freeze deadline. In fact, given it’s experimental nature, we can just compile some nightly versions and ask the user to depend on that if we are going to use Estimator in downstream packages, e.g. GluonNLP.

[leezu]

@sxjscience the problem is that the API was changed on November 1, even though the code-freeze was in effect since 2019-10-25.
The change of November 1 was later discovered to be incomplete and @liuzh91 worked hard to fix it.

I agree with you that in retrospect it would be better to revert the change from November 1 and include all changes to the master only, instead of trying to fix 1.6 during the code freeze.

While the code is experimental, we shouldn't break it unnecessarily between releases. Thus I suggest we 1) either remove the incomplete Estimator API changes from 1.6, or 2) we decide the current naming is fine, or 3) we fix the naming in 1.6 release. Changing only the naming in the 1.7 release seems bad to me.

[leezu]

@ptrendx will you include this in RC1? If so, let's merge it

[ptrendx]

@leezu Did you check that the nightly tests pass with this change? If so, then yes, let's take it.

[szha]

This PR looks good so I'm merging it to master. @liuzh91 @leezu please verify and submit separate PR to the release branch.