Skip to content
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

NEP: update backwards compatibility and deprecation policy NEP #18097

Merged
merged 8 commits into from Jan 25, 2021

Conversation

rgommers
Copy link
Member

It would be nice to get this NEP to Accepted status. Main changes in this PR are:

  • Removed all examples that people objected to
  • Removed all content regarding versioning
  • Restructured sections, and added "Strategies related to deprecations" (using suggestions by @njsmith and @shoyer).
  • Added concrete examples of deprecations, and a more thorough description of how to go about adding warnings incl. Sphinx directives, using stacklevel, etc.

@njsmith I used a bunch of phrasing and restructuring ideas from your review of the first version of this NEP on the mailing list. Can I add you as a co-author?

The removed examples were ones that one or more people objected to,
because they were either not completely settled, or controversial.
There was agreement in the previous discussion that this doesn't
belong in this NEP.
Also add examples of how deprecations and future warnings should
be done, taken from the current code base.
DeprecationWarning, stacklevel=2)

# A change in NumPy 1.14.0 for Python 3 loadtxt/genfromtext, slightly
# tweaked in this NEP (original didn't have version number).
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't quite reflect current practice.

Python
Note comment and stacklevel.

    # NumPy 1.15.0, 2017-12-10
    warnings.warn(
        "np.loads is deprecated, use pickle.loads instead",
        DeprecationWarning, stacklevel=2)
    return pickle.loads(*args, **kwargs)

C
Note comment.

        /* DEPRECATED 2020-05-13, NumPy 1.20 */
        if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1,
                matrix_deprecation_msg, ufunc->name, "first") < 0) {
            return NULL;
        }

This is more howto than policy, maybe it should go elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think I agree with these changed examples. What is the user supposed to do, read the source code or grep release notes to find the version it was introduced?

Copy link
Member

Choose a reason for hiding this comment

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

Grep the source code is what I do, it helps if the information has a standard form recognized by a regex. I don't disagree that a NumPy version should be included in the message as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're just commenting on the comment. Okay, I'll take that over, the comment here is not part of the example, it's indicating that the example warning was changed from how it looks in the code base.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is more howto than policy, maybe it should go elsewhere.

Probably useful to keep, it doesn't take up much space and examples are usually helpful.

- shall use ``VisibleDeprecationWarning`` rather than ``DeprecationWarning``
for cases of relevance to end users. For cases only relevant to
downstream libraries, a regular ``DeprecationWarning`` is fine.
*Rationale: regular deprecation warnings are invisible by default; library
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually true any more? I think the default visibility changed in 3.8, although possibly not enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still true. I think you're referring to Python Development Mode, which makes a number of warnings visible but is not enabled by default.

Copy link
Member

Choose a reason for hiding this comment

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

My thinking was that if "end users" have a workflow of running python myscript.py, then DeprecationWarnings are visible to them automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's https://www.python.org/dev/peps/pep-0565/ - never even noticed that.

The Python Development Mode docs are then incorrect/incomplete about their discussion of DeprecationWarning.

Copy link
Member

Choose a reason for hiding this comment

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

To repeat it here. I don't think we have ever used VisibleDeprecationWarning as default warning (which this reads like)? So I need a bit convincing that we want to change that (almost all changes are relevant to end users). I would currently used it when:

  • We already had a DeprecationWarning (so libraries are mostly updated, but end-users may not be, and it was a noisy warning that we wanted to take slow)
  • We were tempted to even skip deprecation because the behaviour may have caused bugs or is deeply confusing or very strongly undesirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated this with @seberg's suggestion

@charris
Copy link
Member

charris commented Jan 2, 2021

I think the default visibility changed in 3.8

Looks like it is still filtered out by default, but FutureWarning has been repurposed as as a visible deprecation warning for users.

Changed in version 3.7: Previously DeprecationWarning and FutureWarning were distinguished based on whether a feature was being removed entirely or changing its behaviour. They are now distinguished based on their intended audience and the way they’re handled by the default warnings filters.

See https://docs.python.org/3/library/warnings.html

So the question becomes, should NumPy follow Python in that regard. IIRC, DeprecationWarning was filtered because PyCapsule was backported to Python 2.7 and PyCObject deprecated. That was a mistake that resulted in a flood of DeprecationWarnings and the easiest way to recover was to filter them out by default :)

EDIT: Changing the meaning of FutureWarning was an unfortunate idea, I think Python should have added a new warning as we did.

@rgommers
Copy link
Member Author

rgommers commented Jan 2, 2021

EDIT: Changing the meaning of FutureWarning was an unfortunate idea, I think Python should have added a new warning as we did.

I agree. Keeping our VisibleDeprecationWarning and not changing what FutureWarning means seems better.

@seberg
Copy link
Member

seberg commented Jan 2, 2021

I have some smaller comments. The visible deprecation warning was the only thing that made me wonder a bit more. I don't think it is current practice really, and I am not sure I agree with it being a good first step, unless:

  • you would be sure that this is end-users only (which seems unlikely)
  • It warns the user about a potential bug/trap.

I do think end-users now do see them often? I don't disagree with the desire to make sure end-users see it, but it is also annoying if end-users see warnings they can do nothing about. So the question is which we rather have as a default. If something is very noisy, I would say we should do what we did in the past (first use DeprecationWarning, then switch to VisibleDeprecationWarning).

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

I don't have any squirms with the draft with respect to accepting it. But a few comments and maybe others want to kneed the text a bit more. I do not remember discussion on the old draft (I doubt I really looked into it at the time), so I am sorry if I am missing something there.

These are largely comments/thoughts and likely most can be disregarded. Although, I admit I think the first part that is loosely about the "assessment/decision making" could probably be structured a bit clearer.

@@ -257,34 +292,26 @@ ecosystem - being fairly conservative is required in order to not increase the
extra maintenance for downstream libraries and end users to an unacceptable
level.
Copy link
Member

Choose a reason for hiding this comment

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

Could remove the sub-heading, as it is only one paragraph remaining (for one, we could also be less aggressive)? Honestly, I would even be fine with just deleting the whole "Alternatives" section, in the end there is not much of an "alternative" to a policy (it is not a feature that could be implemented in two completely different ways, like the array protocols). The whole section is not really more than: The deprecation policy could be modified to be more or less conservative, but we believe the proposed time frames and steps are a good trade-off.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main alternative that some maintainers and downstream library authors have argued for though. No one has seriously said "let's never change anything" I think. So can't hurt to keep this.

doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
only, or moving the documentation for the functionality to a less prominent
place or even removing it completely. Commenting on open issues related to it
that they are low-prio or labeling them as "wontfix" will also be a signal to
users, and reduce the maintenance effort needing to be spent.
Copy link
Member

Choose a reason for hiding this comment

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

This section starts with details about impact assessment, and then moves on to alternatives to deprecations. I think it may be better to make that an explicit section. Maybe the "strategies related to deprecation" itself should be one heading level lower? And the "General Principles" section should actually be clearly about decisions (the "Implementation ...." are also "General Principles").

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, adding "Impact assessment" and "Alternatives to deprecations" sub-sections.

Maybe the "strategies related to deprecation" itself should be one heading level lower?

also done

And the "General Principles" section should actually be clearly about decisions (the "Implementation ...." are also "General Principles").

I think this bit is fine as is. The principles 1-4 are far more general than concrete items about how to go about a deprecation.

Copy link
Member

Choose a reason for hiding this comment

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

"Commenting that they are low priority or labeling them as "wontfix" on open issues..."

@@ -19,46 +19,206 @@ processes for individual cases where breaking backwards compatibility
is considered.


Detailed description
Motivation and Scope
--------------------

NumPy has a very large user base. Those users rely on NumPy being stable
and the code they write that uses NumPy functionality to keep working.
Copy link
Member

Choose a reason for hiding this comment

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

Should we add 1-2 sentences here (and then start a new paragraph)? Saying that this also about:

  • ensure that users are well informed about any change
  • set expectations on the speed and process with which (most) changes occur so that users can plan accordingly.

I.e. that is to say, the document is not just a set of rules for us, but also about informing users about them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Added this a bit lower - it is important, but secondary to decision making about code changes I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added:
In addition, this NEP can serve as documentation for users about how the NumPy
project treats backwards compatibility, and the speed at which they can expect
changes to be made."

doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
doc/neps/nep-0023-backwards-compatibility.rst Show resolved Hide resolved
Out of scope are:

- Making concrete decisions about deprecations of particular functionality.
- NumPy's versioning scheme.
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 there is a need for the out-of-scope list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making too many concrete decisions was a main complaint about the first draft, so I'd like to keep it. Having an out of scope is almost always a good idea.

doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Thanks, Ralf! I think most of the pieces are here. It may be worth mentioning the deprecation timeline earlier; it only appears once in a paragraph much further down.

doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
doc/neps/nep-0023-backwards-compatibility.rst Outdated Show resolved Hide resolved
Deprecations:

- shall include the version number of the release in which the functionality
was deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

@rossbar Is this something numpydoc can check for?

Deprecations:

- shall include the version number of the release in which the functionality
was deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should give an explicit example of how that looks like in a docstring.

doc/neps/nep-0023-backwards-compatibility.rst Show resolved Hide resolved
doc/neps/nep-0023-backwards-compatibility.rst Show resolved Hide resolved
Deprecations:

- shall include the version number of the release in which the functionality
was deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

(To be clear, I was thinking of .. deprecated::).

rgommers and others added 3 commits January 23, 2021 17:27
@rgommers
Copy link
Member Author

Thanks for the reviews. I updated the text for most of them, and replied to the ones I didn't quite agree with.

@charris
Copy link
Member

charris commented Jan 23, 2021

Sorry for the clutter :) Github isn't made for line editing documents.

@rgommers
Copy link
Member Author

Sorry for the clutter :) Github isn't made for line editing documents.

No worries, thanks for the copy edits. All adopted.

@mattip mattip merged commit bfc7e43 into numpy:master Jan 25, 2021
@mattip
Copy link
Member

mattip commented Jan 25, 2021

Thanks @rgommers

@rgommers rgommers deleted the nep-backcompat-update branch January 26, 2021 08:17
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.

None yet

6 participants