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

[MNT]: Delay (or make pending) the deprecation of set_constrained_layout/set_tight_layout #22343

Closed
anntzer opened this issue Jan 28, 2022 · 9 comments · Fixed by #22345
Closed
Milestone

Comments

@anntzer
Copy link
Contributor

anntzer commented Jan 28, 2022

Summary

set_constrained_layout/set_tight_layout is being deprecated in 3.6, but this makes it difficult (other than by version-gating) to work e.g. with matplotlib master (which I do on my personal setup) while keeping the code working with the last released version (3.5).

This also means that once mpl3.6 releases, essentially all projects that use this set_constrained_layout/set_tight_layout will need to "immediately" release a new version to hide that warning from end users.

Instead, perhaps that deprecation can be made pending for now, and turned into an actual deprecation in mpl 3.7 for release in 3.9, which leaves some buffer time for the change.

More generally, I think that when a new feature comes in in version X and is intended to replace an older approach, that old approach should not be deprecated in version X, but only in version X+1 at the latest (it can be pending-deprecation in version X), so that dependents can update during the life-cycle of version X, and so that people using master (before the release of X) do not run into the issue above.

Proposed fix

Make the deprecations pending.

@mwaskom
Copy link

mwaskom commented Jan 28, 2022

This also means that once mpl3.6 releases, essentially all projects that use this set_constrained_layout/set_tight_layout will need to "immediately" release a new version to hide that warning from end users.

Isn't that what DeprecationWarning is for? Firing a warning that will appear in tests but not (by default) in a normal session.

@jklymak
Copy link
Member

jklymak commented Jan 28, 2022

I made these particular deprecations, and am happy to do as I'm told. However, https://docs.python.org/3/library/exceptions.html#DeprecationWarning indeed seems to support the idea that end users need not be protected from deprecation warnings.

Is the problem then that developers will have their tests fail unless they add a gate or ignore the warning and never adjust?

Its pretty hard on us as MPL developers to decide to change feature X, but not be allowed to mark the change until the next release. I think the odds of someone remembering in 15 months to deprecate set_constrained_layout is pretty small. I suppose we could add a pending_deprecation decorator and either do nothing, or have it emit PendingDeprecationWarning (which python thinks should only be used rarely).

Finally in this particular case its probably not necessary to remove these methods at all, and simply discourage them in the docstring, and maybe remove in a few years if we want.

FWIW I am no sure that developers should be setting either tight or constrained layout for their users, as that leads to mysterious behaviour. But I've not ever used matplotlib in a downstream library and maybe there are valid use cases.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 28, 2022

Isn't that what DeprecationWarning is for? Firing a warning that will appear in tests but not (by default) in a normal session.

I usually write modules that can be run as standalone utilities from the command line as python -mmodule ... via a if __name__ == "__main__" block (but can also be imported by other modules) and this usage will cause DeprecationWarnings to be emitted if module uses set_constrained_layout.

Is the problem then that developers will have their tests fail unless they add a gate or ignore the warning and never adjust?

Or just that they get a warning on stderr which needs some contortions to be suppressed.

I suppose we could add a pending_deprecation decorator

There's already pending=True that's supported, and we can easily grep for these after each release to promote them to normal deprecations (in the same vein as the big "expire deprecation" PRs that show up after each release).

I am no sure that developers should be setting either tight or constrained layout for their users, as that leads to mysterious behaviour.

In my case I'm writing something than can serve as a module, but also as a standalone command-line utility, and in that case there is no place where the caller can call set_constrained_layout (the caller doesn't even write any python code, just shell).

@jklymak
Copy link
Member

jklymak commented Jan 28, 2022

Hmm, OK, so you are really writing a app ;-)

There's already pending=True that's supported,

Sure we can do that. A quick grep indicates it is only used for some pending tick deprecations in 3.1 that are, ahem, still pending.

@jklymak
Copy link
Member

jklymak commented Jan 28, 2022

... though my ignorance is such that I am not sure how a PendingDeprecationWarning is better.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 28, 2022

The PendingDeprecationWarning is ignored (not displayed) by default (https://www.python.org/dev/peps/pep-0565/#new-default-warnings-filter-entry). But it should still be enabled e.g. by pytest.

@mwaskom
Copy link

mwaskom commented Jan 28, 2022

More generally, I think that when a new feature comes in in version X and is intended to replace an older approach, that old approach should not be deprecated in version X, but only in version X+1 at the latest (it can be pending-deprecation in version X), so that dependents can update during the life-cycle of version X, and so that people using master (before the release of X) do not run into the issue above.

FWIW, I think most maintainers of downstream projects who are not also the #1 committer to matplotlib would probably not be aware of these deprecations until they start issuing warnings (at least in tests.) So providing a smoother transition is 👍, but at some point you do need to make some noise.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 29, 2022

To be clear, I agree with making some noise; I'm just deferring this for one release. Also I don't think (but I may be mistaken) that in this specific case keeping the old backcompat API makes further improvements to layout engines harder? If that was the case I would also be more open to a faster deprecation cycle.

@oscargus
Copy link
Contributor

oscargus commented Feb 1, 2022

it is only used for some pending tick deprecations in 3.1 that are, ahem, still pending.

I did independently detect that and was thinking of making them non-pending as a part of #22366

Any opinions? Is it time?

@QuLogic QuLogic added this to the v3.6.0 milestone Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants