-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update API Compatibility Policy #2778
Conversation
docs/source/compatibility.rst
Outdated
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
The backward compatibility is kept for **revision updates**, which are applied to the latest stable version. | ||
A **major update** from the latest release candidate basically keeps the backward compatibility, although it is not guaranteed. | ||
Any **pre-releases** may break the bakward compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: backward compatibility.
docs/source/compatibility.rst
Outdated
At this point, users should not use the marked API in their new application codes. | ||
- At the major update announced in the above update, change the API. | ||
Since Chainer v2, we have stopped adopting any solid processes to break backward compatibilities (e.g. a solid schedule for deprecating and removing a feature) in order to keep the development fast enough to support the cutting-edge research. | ||
**It does not mean we stop taking care of mantenability of user codes.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: maintenability
docs/source/compatibility.rst
Outdated
|
||
Deprecation, Dropping, and Its Preparation | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
The backward compatibility is kept for **revision updates**, which are applied to the latest stable version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove "latest" because that could mistakenly make users think that there are multiple stable versions, which is not true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there ARE multiple stable versions (e.g. both 1.24.0 and 2.0.0 will be stable after releasing 2.0.0). Isn't it true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the explanation of "stable version" that appeared for the first time in the update document (here), stable version is the revision updates for the latest major versions. So I thought the stable version automatically points at the latest major version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Hmm, ok, I'll fix the presentation.
|
||
Model Format Compatibility | ||
-------------------------- | ||
|
||
Objects serialized by official serializers that Chainer provides are correctly loaded with the higher (future) versions. | ||
Links and chains serialized by official serializers that Chainer provides are correctly loaded with the future versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we ensure model compatibilities are kept even an major version up is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we should do that, otherwise, users that have their own serialized models cannot upgrade to a newer version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. Thank you.
For example, suppose an API X is added at the minor update. | ||
In the previous version, attempts to use X cause AttributeError. | ||
This behavior is not stated in the documentation, so this is undefined. | ||
Thus, adding the API X in minor version is permissible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this example is still valid if we replace "minor update" with "revision update". So I don't think we need to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API won't be added to the revision update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thank you.
|
||
Model Format Compatibility | ||
-------------------------- | ||
|
||
Objects serialized by official serializers that Chainer provides are correctly loaded with the higher (future) versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the reason why you have changed the term is that you think it is difficult to keep the model compatibilities other than Link and Chains (e.g. Trainer)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want confirm that we have a consensus about narrowing the support among the developer team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already broke the serialization compatibility of non-model objects.
This change updates API Compatibility Policy for v2. Since it includes a link to some sections of the updated contribution guide, this PR depends on #2773. Please merge #2773 before this one.