-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
NEP: backwards compatibility and deprecation policy #11596
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
Conversation
to give those libraries time to fix their code first. It was finally | ||
introduced for v1.11.0 and turned into a hard error for v1.12.0. | ||
|
||
This change was disruptive, however it did catch real bugs in e.g. SciPy and |
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.
Commas around 'e.g.'.
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.
Lots of comma stuff, I won't make anymore comments about it at this time.
**Removing the financial functions** | ||
|
||
The financial functions (e.g. ``np.pmt``) are badly named, are present in the | ||
main NumPy namespace, and don't really fit well with NumPy's scope. |
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.
'within'.
The motivation was that all these cost maintenance effort, and that they slow | ||
down work on the core of Numpy (ndarrays, dtypes and ufuncs). | ||
|
||
The import on downstream libraries and users would be very large, and |
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.
'impact'
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.
A few typos. I saw one more but can't for the life of me find it.
Alternatives | ||
------------ | ||
|
||
**Being more agressive with deprecations.** |
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.
agressive -> aggressive
|
||
**Being more agressive with deprecations.** | ||
|
||
The goal of being more agressive is to allow NumPy to move forward faster. |
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.
agressive -> aggressive
Deprecations: | ||
|
||
- shall include the version numbers of both when the functionality was deprecated | ||
and when it will be removed (either two releases after the warning is |
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 two releases may be on the short side for some things. Is it really that important to put a time or removal on it? Note that NumPy releases at about 3x the rate of Python.
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.
Could be longer, maybe 3 makes more sense. Can also add an option to deviate from those 2 or 3. I think it's import to have a default though, both so we document things inside the DeprecationWarning
and so that we don't spend time on deciding on that for each deprecation.
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.
For me, unless it was almost a bug, I think we always had more an "at least" two versions thing going? Also one thing that is not too untypical is that we delay deprecations (possibly with maybe more aggressive warnings) when people start noticing that they missed them during rc testing.
The major release discussion is interesting, but I guess we should keep it a bit vague here. Or do you think anything that we feel should take longer then a year, should really lock in with a major release (lets say we do not do a major release more then every 2 years or so)?
Anyway, from first quick read over it, looks very nice Ralf.
Very nice, I particularly like the historic examples. Punctuation could use some work, but can wait until content settles out. |
Also fix some typos.
|
||
The ``new`` keyword was planned from the start to be temporary; such a plan | ||
forces users to change their code more than once. Such keywords (there have | ||
been other instances proposed, e.g. ``legacy_index`` in |
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 don't think legacy_index
falls in the same category as new
here. It is not intended as temporary -- there is no plan 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.
there is no plan to remove it.
there isn't such a plan right now:) I'm pretty sure the next generation of developers will look at it the same way as we look at things that are ugly and we'd rather not have - we start proposing to deprecate and remove.
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.
True, although if a typical downstream project supports say 4 years of numpy versions, I think once those have passed and people start discussing such a plan, then maybe it is a bit of a diff
8000
erent thing than a temporary new
kwarg.
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.
@shoyer removed. clearly your proposal wasn't in the same league as new
which disappeared in 2 releases.
I'm serious though that everything named new
, old
, legacy
, change_behavior
etc. is a mistake almost by definition. It was similar in the random
NEP; thinking about a separate use case (unit testing) and creating StableRandom
was a much better outcome there than the original LegacyGenerator
.
This a major misrepresentation of our proposal in the indexing NEP. Please
remove the reference.
…On Sat, Jul 21, 2018 at 6:07 PM Ralf Gommers ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/neps/nep-0023-backwards-compatibility.rst
<#11596 (comment)>:
> +
+ def histogram(a, bins=10, range=None, normed=False, weights=None, new=False): #v1.1.0
+
+ def histogram(a, bins=10, range=None, normed=False, weights=None, new=None): #v1.2.0
+
+ def histogram(a, bins=10, range=None, normed=False, weights=None): #v1.5.0
+
+ def histogram(a, bins=10, range=None, normed=False, weights=None, density=None): #v1.6.0
+
+ def histogram(a, bins=10, range=None, normed=None, weights=None, density=None): #v1.15.0
+ # v1.15.0 was the first release where `normed` started emitting
+ # DeprecationWarnings
+
+The ``new`` keyword was planned from the start to be temporary; such a plan
+forces users to change their code more than once. Such keywords (there have
+been other instances proposed, e.g. ``legacy_index`` in
there is no plan to remove it.
there isn't such a plan right now:) I'm pretty sure the next generation of
developers will look at it the same way as we look at things that are ugly
and we'd rather not have - we start proposing to deprecate and remove.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11596 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1suhmyoYz9mk9hnehysxVTDjUqxJks5uI9BFgaJpZM4VZzP7>
.
|
One thing I propose we do is ping major downstream users on a single question - Faster evolution vs avoiding breakage, to get a feel for how other core devs see things. We should get more opinions, instead of assuming what they'd want 6DAF and allow them to speak for themselves. If I may, we could set up a simple poll. I'd assume that it takes a lot less effort to vote in a poll than to write your opinions up in an email or comments - Which would mean we get a correspondingly large number of opinions. Edit: My point in doing this is that everyone has their experience to draw from, and different people have different experiences. We can reason about it all day, but in the end, only statistics (properly collected) will tell the truth. |
I think the problem with that is that our contacts will only be a minority of downstream users. Heck, downstream users miss release candidates. And to be honest, I don't think we should make it a choice between the two options. |
|
||
**Examples of features not added because of backwards compatibility** | ||
|
||
TODO: do we have good examples here? Possibly subclassing related? |
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.
PR #11381 which tried to remove the NPY_ITER_OVERLAP_ASSUME_ELEMENTWISE
flag from gufunc calls is a possible candidate - it was rejected since there is concern and a concrete example that depends on this precise flag which seems to be erroneously copy-pasted from other code sections, gufunc calls can obviously do non-element-wise operand handling.
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 would characterize this instead as design disagreement on what guarantees a gufunc inner loop should satisfy. The flag is not copypasted erroneously, but under a different interpretation of how gufunc inner loops should handle overlap --- I remember what I had in mind when writing this code. Whether that guarantee is in reality practical and reasonable design for gufuncs to follow is then a more arguable question, and deciding that is necessary before proceeding with code changes.
…y_index``. [ci skip]
I agree. We'd likely only ping the kinds of projects that anyway follow the numpy mailing list. And actually many numpy devs and contributors also work on a downstream package, so they assess proposals with two hats on. What we need to worry about are less actively developed packages, further downstream ones, ones with long time horizons (e.g. shipped in commercial products), new end users, etc. We won't reach any of those with a poll. |
- ``numpy.random`` has its own backwards compatibility policy, | ||
see `NEP 19 <http://www.numpy.org/neps/nep-0019-rng-policy.html>`_. | ||
- The file format for ``.npy`` and ``.npz`` files must not be changed in a backwards | ||
incompatible way. |
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.
The npy file format has it's own major/minor version numbers. We actually already did increment the major version number for version 2.0 when we made a backwards incompatible change.
So I don't think this is quite right. Maybe add "unless the file format versions numbers are appropriately incremented"?
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.
OK, maybe I was mixing up backwards and forwards compatibility here :). It might be better to explicitly say:
- Backwards compat: New versions of numpy should always read old files.
- Forwards compat: New version of numpy should write files readable by old versions of numpy whenever possible.
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.
That sounds like a good addition, thanks. Will update like that.
It seems we never merged this and put it for discussion on the mailing list. Is there more to do before merging? |
Except for the tiny fixups here maybe. My guess is, just a heads up to the mailing list and maybe wait a week or so to make sure we stick to formality (since IIRC there was consensus already, but doesn't hurt). Its nice to have a document to point to! |
Well, there were a number of comments on the mailing list, so I have to rework the document before it will be accepted (will take some time, limited bandwidth in Sep/Oct). Given what we do with other NEPs, I think this can be merged as Draft though. But it cannot be proposed for Accept in its current form. |
One other minor point for the next iteration: Consider mentioning that some "experimental" components of NumPy (e.g., datetime64, though perhaps it's time to change that) may have reduced backwards compatibility guarantees. |
@charris: thanks for the ping.
People seem good with merging this as Draft now as far as I can tell. If not, please comment and then we leave it open till I come back to it. Otherwise, please someone hit the green button (or I will next time I'm here) |
Draft looks good and merging now helps clean up the PR stack. Thank Ralf. |
No description provided.