Skip to content

ENH: New proposed deprecation policy #58169

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

Closed
wants to merge 21 commits into from

Conversation

Aloqeely
Copy link
Member

@Aloqeely Aloqeely commented Apr 5, 2024

xref #54970

Use DeprecationWarning until the last minor version before a major release then we switch to FutureWarning

@Aloqeely Aloqeely requested a review from MarcoGorelli as a code owner April 5, 2024 23:37
@Aloqeely Aloqeely changed the title New proposed deprecation policy ENH: New proposed deprecation policy Apr 5, 2024
@Aloqeely
Copy link
Member Author

Aloqeely commented Apr 5, 2024

One thing to note is that when using pytest.mark.filterwarnings I have kept the ignore string as "ignore::DeprecationWarning" which we will need to change to FutureWarning once we change Pandas40DeprecationWarning, my reasoning for this is that you have to pass the module path (so ignore::pandas.util._exceptions.Pandas40DeprecationWarning) for aliased warnings, which didn't look pretty to me

Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label May 17, 2024
@Aloqeely Aloqeely removed the Stale label May 24, 2024
@Aloqeely
Copy link
Member Author

Aloqeely commented Jun 6, 2024

Gentle ping @lithomas1. I would like this to move forward so I don't have to update it every time a new deprecation is added

@Aloqeely Aloqeely requested a review from rhshadrach as a code owner June 6, 2024 04:30
@lithomas1
Copy link
Member

Gentle ping @lithomas1. I would like this to move forward so I don't have to update it every time a new deprecation is added

Thanks for the reminder (and sorry that this slipped off my radar).
Will take a look this evening.

Copy link
Member

@lithomas1 lithomas1 left a comment

Choose a reason for hiding this comment

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

I left some more comments.

I wouldn't be comfortable merging this in by myself. Let's see what some other core devs think.

cc @pandas-dev/pandas-core for other thoughts on this.

@@ -24,6 +24,10 @@ enhancement1
enhancement2
^^^^^^^^^^^^

New Deprecation Policy
^^^^^^^^^^^^^^^^^^^^^^
pandas 3.0.0 introduces a new 3-stage deprecation policy: using ``DeprecationWarning`` initially, then switching to ``FutureWarning`` for broader visibility in the last minor version before the next major release, and then removal of the deprecated functionality in the major release.
Copy link
Member

Choose a reason for hiding this comment

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

It might be good to add some reasoning to this
(e.g. This was done to give downstream packages more time to adjust to pandas deprecations, which should reducing the amount of warnings that an user gets from code that isn't theirs.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd have to also say that there would always be a minimal amount of time (6 months, maybe more??) between the last minor version before the next major release.

Copy link
Member Author

Choose a reason for hiding this comment

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

a minimal amount of time (6 months, maybe more??) between the last minor version before the next major release.

Is this consistent? 3.0 was planned to be released within 6 months of releasing 2.2.0 (#57064)

Copy link
Contributor

Choose a reason for hiding this comment

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

a minimal amount of time (6 months, maybe more??) between the last minor version before the next major release.

Is this consistent? 3.0 was planned to be released within 6 months of releasing 2.2.0 (#57064)

In your last commit, you wrote "This was done to give downstream packages more time to adjust to pandas deprecations". So if we are going to have a policy about this, then we have to decide how much time is sufficient for "more time".

@lithomas1
Copy link
Member

One thing we also could consider doing is decoupling the test changes from this PR, and just introducing Pandas40DeprecationWarning, and basic tests that it works as expected.

That way there's less chances of this creating conflicts and getting out of sync.

@bashtage
Copy link
Contributor

bashtage commented Jun 7, 2024

I think switching to Future warning only one release before the release that will break is too short. My motion on this is that it would be only 6 months or so, and sometimes less. Often end users report issues to projects through future warnings. This then requires downstream to make the changes and make a release before the next pandas, or face a break (unless strictly capping, which is not common).

Producing a FutureWarning 2 releases prior to breaking things seems like a more balanced approach.

@Aloqeely
Copy link
Member Author

Aloqeely commented Jun 7, 2024

Producing a FutureWarning 2 releases prior to breaking things seems like a more balanced approach.

That would mean any deprecations released in the last 2 minor releases will immediately be FutureWarnings which means downstream libraries won't have any time to handle/suppress these warnings. Had we implemented your idea earlier, v2.1 deprecations would immediately be FutureWarning (there are a lot of them, see #50578), one of them being the cause of #54970 being opened in the first place.

I guess if we think some deprecation needs attention by end users, we can leave it as a FutureWarning and postpone enforcing the deprecation to a later major release.

Comment on lines +14 to +16
Pandas4DeprecationWarning = DeprecationWarning
Pandas5DeprecationWarning = DeprecationWarning
CurrentDeprecationWarning = Pandas4DeprecationWarning
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be implemented as a Generic class with a parameter corresponding to the version number. So you'd have PandasDeprecationWarning[4] and PandasDeprecationWarning[5], etc. This would allow people to write code that would catch a PandasDeprecationWarning without having to worry about the particular version number.

Copy link
Member Author

Choose a reason for hiding this comment

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

How will the class decide whether to inherit from FutureWarning or DeprecationWarning?

Copy link
Contributor

Choose a reason for hiding this comment

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

How will the class decide whether to inherit from FutureWarning or DeprecationWarning?

You could make the base class a parameter of the Generic declaration.

Copy link
Member

Choose a reason for hiding this comment

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

@Dr-Irv - I don't follow what you're requesting here. Could you post a small PoC? I would like to get this implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this:

_WarningTypeT = TypeVar("_WarningTypeT", DeprecationWarning, FutureWarning)
_WarningVersionT = TypeVar("_WarningVersionT", bound=int)

class PandasWarning(_WarningTypeT, Generic[_WarningTypeT, _WarningVersionT]):
    pass

CurrentDeprecationWarning: TypeAlias = PandasWarning[DeprecationWarning, 3]
CurrentFutureWarning: TypeAlias = PandasWarning[FutureWarning, 4]

Then if you want to warn about a specific version in code, you just use PandasWarning[DeprecationWarning, 4] instead of the Pandas4DeprecationWarning proposed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it appears I misunderstood how the Generic stuff works and got ahead of myself.

You can't parameterize a class via a TypeVar, nor can you parameterize it via a specified integer.

So I withdraw my suggestion to use Generic. Having said that, I do think we should create classes PandasDeprecationWarning and PandasFutureWarning that are just covers over DeprecationWarning and FutureWarning respectively, so that it is easy to catch those warnings without worrying about the 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.

I think the following fits your original intent. Comments below are the warning raised; lack of a comment means no warning seen.

class PandasWarning(Warning):
    pass

class Pandas4Warning(PandasWarning, FutureWarning):
    pass

class Pandas5Warning(PandasWarning, DeprecationWarning):
    pass

warnings.warn("Future", category=Pandas4Warning)
# Pandas4Warning: Future
warnings.warn("Deprecation", category=Pandas5Warning)
# Pandas5Warning: Deprecation

with warnings.catch_warnings():
    warnings.filterwarnings('ignore', '', PandasWarning)
    warnings.warn("Future", category=Pandas4Warning)
    warnings.warn("Deprecation", category=Pandas5Warning)

with warnings.catch_warnings():
    warnings.filterwarnings('ignore', '', FutureWarning)
    warnings.warn("Future", category=Pandas4Warning)
    warnings.warn("Deprecation", category=Pandas5Warning)
    # Pandas5Warning: Deprecation

with warnings.catch_warnings():
    warnings.filterwarnings('ignore', '', DeprecationWarning)
    warnings.warn("Future", category=Pandas4Warning)
    # Pandas4Warning: Future
    warnings.warn("Deprecation", category=Pandas5Warning)

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of

class Pandas4Warning(PandasWarning, FutureWarning):
    pass

class Pandas5Warning(PandasWarning, DeprecationWarning):
    pass

why not do either
OPTION A (leave out the versions):

class PandasFutureWarning(PandasWarning, FutureWarning):
    pass

class PandasDeprecationWarning(PandasWarning, DeprecationWarning):
    pass

OR
OPTION B (include the versions and a more "generic" way)

class Pandas4Warning(PandasWarning, FutureWarning):
    pass

class Pandas5Warning(PandasWarning, DeprecationWarning):
    pass

class PandasFutureWarning(Pandas4Warning):
    pass

class PandasDeprecationWarning(Pandas5Warning):
    pass

Then people can just trap PandasFutureWarning or PandasDeprecationWarning and not worry about version numbers. Or, with option B, they can do it either way.

Copy link
Member

Choose a reason for hiding this comment

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

OPTION A (leave out the versions):

Part of the proposal here is to reduce the maintenance of tracking the promotion of a warning from DeprecationWarning to FutureWarning to removal. Leaving out the version is not an option in my opinion.

I'm fine with adding PandasFutureWarning / PandasDeprecationWarning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Part of the proposal here is to reduce the maintenance of tracking the promotion of a warning from DeprecationWarning to FutureWarning to removal. Leaving out the version is not an option in my opinion.

OK, that makes sense.

I'm fine with adding PandasFutureWarning / PandasDeprecationWarning.

I think we have converged.

Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

@Dr-Irv
Copy link
Contributor

Dr-Irv commented Jun 12, 2024

@Aloqeely we had a discussion about our deprecation policy at the bi-weekly developers meeting, and we think that a PDEP should be created that would formalize the policy and allow for greater discussion. Are you willing to write up a PDEP with a proposed policy?

@Aloqeely
Copy link
Member Author

Sure, I can give it a go.
Going to close this for now

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

Successfully merging this pull request may close these issues.

5 participants