8000 NEP: backwards compatibility and deprecation policy by rgommers · Pull Request #11596 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 4 commits into from
Sep 25, 2018

Conversation

rgommers
Copy link
Member

No description provided.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commas around 'e.g.'.

Copy link
Member

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.
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'impact'

Copy link
Contributor
@hameerabbasi hameerabbasi left a 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.**
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@charris
Copy link
Member
charris commented Jul 22, 2018

Very nice, I particularly like the historic examples. Punctuation could use some work, but can wait until content settles out.


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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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 different thing than a temporary new kwarg.

Copy link
Member Author

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.

@shoyer
Copy link
Member
shoyer commented Jul 22, 2018 via email

@hameerabbasi
Copy link
Contributor
hameerabbasi commented Jul 22, 2018

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 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 correspondingl 6D40 y 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.

@charris
Copy link
Member
charris commented Jul 22, 2018

One thing I propose we do is ping major downstream users on a single question

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?
Copy link
Member

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.

Copy link
Member
@pv pv Jul 22, 2018

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.

@rgommers
Copy link
Member Author

One thing I propose we do is ping major downstream users on a single question

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.

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.
Copy link
Member

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"?

Copy link
Member

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.

Copy link
Member Author

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.

@mattip
Copy link
Member
mattip commented Sep 12, 2018

It seems we never merged this and put it for discussion on the mailing list. Is there more to do before merging?

@seberg
Copy link
Member
seberg commented Sep 12, 2018

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!

@rgommers
Copy link
Member Author

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.

@shoyer
Copy link
Member
shoyer commented Sep 13, 2018

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.

@rgommers
Copy link
Member Author

@charris: thanks for the ping.

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.

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)

@charris charris merged commit 5f2a5ae into numpy:master Sep 25, 2018
@charris
Copy link
Member
charris commented Sep 25, 2018

Draft looks good and merging now helps clean up the PR stack. Thank Ralf.

@rgommers rgommers deleted the nep-backcompat branch September 25, 2018 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0