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

[MRG] Replace DeprecationWarning with FutureWarning #15080

Merged
merged 56 commits into from Oct 29, 2019

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 24, 2019

closes #9857

This PR replaces DeprecationWarning with a newly created SklearnDeprecationWarning (similar to numpy's VisibleDeprecationWarning).

This fixes the custom filter issue raised in #9857 (custom filter are bad basically)

The downside is that users who are already ignoring the DeprecationWarning will need to change their code to now ignore the SklearnDeprecationWarning.

There's no backward compatibility guarantee regarding warnings, but if this PR gets merged, we should make sure to let users know as much as possible

Also we should give an update on this SO question

EDIT: changed name from VisibleDeprecationWarning to SklearnDeprecationWarning

EDIT: changed again to raise FutureWarning

@amueller
Copy link
Member

@amueller amueller commented Sep 24, 2019

Is it good to name it the same as Numpy? Shouldn't we have a SKLVisibleDeprecationWarning or something like that?

@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Sep 24, 2019

I was hoping that users could ignore both ours and numpy's with a simple 'VisibleDeprecationWarning' regex but that doesn't seem to be possible anyway (maybe for the better).

I'll change to SklearnDeprecationWarning

@NicolasHug NicolasHug changed the title [NOMRG] Replace DepWarning with VisibleDepWarning [MRG] Replace DeprecationWarning with SklearnDeprecationWarning Sep 25, 2019
@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Sep 25, 2019

@adrinjalali you were thinking about warnigns ;)

@adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Sep 26, 2019

I'm wondering why we do DeprecationWarning in the first place. Python's docs say:

DeprecationWarning:
Base category for warnings about deprecated features when those warnings are intended for other Python developers (ignored by default, unless triggered by code in __main__).

The unless triggered by code in __main__ is since Python 3.7 (PEP-565)

On the other hand, we have:

FutureWarning:
Base category for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python.

And:

PendingFutureWarning:
Base category for warnings about features that will be deprecated in the future (ignored by default).

IMO most DeprecationWarnings we have, are indeed FutureWarnings, which is shown by default to the user anyway.

I'm in favor of using FutureWarning wherever we actually want the user to see them, and the other two, based on whatever suits each case, when we want to warn the package maintainers.

Packages who depend on sklearn, or any other package, should have a CI failing on DeprecationWarning, those warnings shouldn't be displayed to all users.

Also, we wan move a bunch of our UserWarnings to logger.warn.

@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Sep 26, 2019

I agree our DeprecationWarnings should probably have been FutureWarnings in the first place. I'm OK for switching to FutureWarnings.

However I don't think we want to worry about PendingFutureWarnings (at least for now, and definitely not for this PR, let's keep things simple).

Same for DeprecationWarning: these are for Python developers, not for library users/maintainers. I don't think we should ever mess with those.

@rth
Copy link
Member

@rth rth commented Sep 26, 2019

+1 for using FutureWarnings instead of a custom warning class.

Related discussions in numpy/numpy#8829 (comment) and scikit-image/scikit-image#3244

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Sep 26, 2019

+1 for FutureWarning. Do we still use the verb “deprecated” in the message?

@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Sep 26, 2019

Why shouldn't we?

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Sep 26, 2019

The options are:

  1. This feature was deprecated in 0.22 and will be removed in 0.24.
  2. This feature will be removed in 0.24.

Is the "deprecated in 0.22" actionable information? I am going under the assumption that "no one reads warnings", so making the warning only include actionable information is a little nicer. "Remove in 0.24" -> better do something about this before 0.24.

@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Oct 28, 2019

I have no idea why that joblib warning is showing up :/

https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=9548

@adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Oct 28, 2019

I have no idea why that joblib warning is showing up :/

@jeremiedbb or @ogrisel maybe?

@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Oct 28, 2019

It's green \o/

ping @thomasjpfan @adrinjalali @ogrisel @glemaitre

@@ -25,10 +24,6 @@
logger.setLevel(logging.INFO)


# Make sure that DeprecationWarning within this package always gets printed
warnings.filterwarnings('always', category=DeprecationWarning,
module=r'^{0}\.'.format(re.escape(__name__)))
Copy link
Member Author

@NicolasHug NicolasHug Oct 28, 2019

Choose a reason for hiding this comment

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

Note: the main purpose of this PR is to remove this filter

Copy link
Member

@adrinjalali adrinjalali left a comment

I'll be happy to merge soon if there are no objections. Before there are more conflicts and issues.

@adrinjalali adrinjalali added this to In progress in Meeting Issues via automation Oct 29, 2019
@adrinjalali adrinjalali moved this from In progress to Reviewer approved in Meeting Issues Oct 29, 2019
@adrinjalali adrinjalali added this to In progress in Interpretability / Plotting / Interactive dev via automation Oct 29, 2019
@adrinjalali adrinjalali moved this from In progress to Reviewer approved in Interpretability / Plotting / Interactive dev Oct 29, 2019
@adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Oct 29, 2019

Could you please add a nice whats_new for this one? I think it's missing it.

@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Oct 29, 2019

Just pushed something but I think we can have a better message

@adrinjalali adrinjalali merged commit 19ad136 into scikit-learn:master Oct 29, 2019
19 checks passed
Interpretability / Plotting / Interactive dev automation moved this from Reviewer approved to Done Oct 29, 2019
Meeting Issues automation moved this from Reviewer approved to Done Oct 29, 2019
@NicolasHug
Copy link
Member Author

@NicolasHug NicolasHug commented Oct 29, 2019

Thanks for the reviews!! \o/

@stefanv
Copy link
Contributor

@stefanv stefanv commented Jan 21, 2021

I noticed PEP 565 says:

For library and framework authors that want to ensure their API compatibility warnings are more reliably seen by their users, the recommendation is to use a custom warning class that derives from DeprecationWarning in Python 3.7+, and from FutureWarning in earlier versions.

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Jan 22, 2021

In the same section Additional use case for FutureWarning in the PEP 565 it states:

The standard library documentation will be updated to explicitly recommend the use of FutureWarning (rather than DeprecationWarning) for backwards compatibility warnings that are intended to be seen by users of an application.

In the end I think we want the warning to be shown by default to users in the following situation:

# my_module.py
import warnings
def hello():
	warnings.warn("Deprecated", DeprecationWarning)
# main.py
from my_module import hello

hello()

and running python main.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants