[FLINK-40529][python] Warn on use of deprecated APIs, not at import time - #29061
Draft
deepyaman wants to merge 3 commits into
Draft
[FLINK-40529][python] Warn on use of deprecated APIs, not at import time#29061deepyaman wants to merge 3 commits into
deepyaman wants to merge 3 commits into
Conversation
Deprecated emitted its DeprecationWarning from __call__, which the decorator syntax invokes in order to apply the decorator. The warning therefore fired at decoration time -- that is, at import -- for every deprecated API a module defines, whether or not the user touches it, while actually calling one emitted nothing, and stacklevel=2 pointed at the decoration site inside PyFlink's own source rather than at user code. Functions now get a functools.wraps wrapper that warns when called. Classes are returned unchanged, with __init__ wrapped in place so that isinstance checks and subclassing keep working; as in PEP 702, only instantiating the deprecated class itself warns, which also avoids warning twice when a deprecated class inherits the __init__ of a deprecated base class. staticmethod and classmethod objects are unwrapped and re-packaged, and properties, ABCs and Enum subclasses degrade to the docstring directive rather than raising -- decorating any of these used to fail. A missing detail argument no longer raises either. The message format, the DeprecationWarning category, the docstring directives and the __stability_decorators attribute read by PythonAPICompletenessTestCase are unchanged, and Experimental, Internal, Public and PublicEvolving are unaffected. pyflink/util was not in the list of modules that dev/integration_test.sh runs, so it is added there for the new tests to run in CI. Generated-by: Claude Code 2.1.252 (Claude Opus 5)
deepyaman
commented
Sep 3, 2026
Comment on lines
+36
to
+44
| def _catch_warnings(): | ||
| """ | ||
| Returns a context manager recording every warning raised within it. | ||
| """ | ||
| context = warnings.catch_warnings(record=True) | ||
| warnings.simplefilter("always") | ||
| return context | ||
|
|
||
|
|
Contributor
Author
There was a problem hiding this comment.
Why not use something like assertWarns (or pytest.raises, although PyFlink doesn't seem to generally use pytest for whatever reason).
Comment on lines
+67
to
+68
| # pyflink.table used to warn as soon as the package was imported. This needs a fresh | ||
| # interpreter, as pyflink.table is already imported in the one running the tests. |
Contributor
Author
There was a problem hiding this comment.
Is this really the best approach? How is this usually done?
Comment on lines
+70
to
+75
| "import warnings\n" | ||
| "with warnings.catch_warnings(record=True) as caught:\n" | ||
| " warnings.simplefilter('always')\n" | ||
| " import pyflink.table\n" | ||
| "print([str(warning.message) for warning in caught\n" | ||
| " if 'has been deprecated since version' in str(warning.message)])\n" |
Contributor
Author
There was a problem hiding this comment.
Any reason not to use a triple-quoted string instead?
Comment on lines
+80
to
+83
| try: | ||
| func_or_cls.__doc__ = f"{docstring}\n{directive}" | ||
| except (AttributeError, TypeError): | ||
| pass |
Contributor
Author
There was a problem hiding this comment.
What's the purpose of the try/except here?
Comment on lines
+91
to
+97
| # Not every decorated object accepts attribute assignment (a property, for | ||
| # example). Those simply cannot be introspected; that is not a reason to fail | ||
| # at import time. | ||
| try: | ||
| setattr(func_or_cls, '__stability_decorators', {self.__class__}) | ||
| except (AttributeError, TypeError): | ||
| pass |
Contributor
Author
There was a problem hiding this comment.
Again, is this actually being exercised? Or is this unnecessarily-defensive coding?
| return msg | ||
|
|
||
| @override | ||
| def __call__(self, func_or_cls: T) -> T: |
Contributor
Author
There was a problem hiding this comment.
All the changes (and also the existing code) is very complicated! Any reason this can't use @deprecated from typing-extensions>=4.5.0, at least as a base?
If not, could a lot not at least be learned from the PEP-702 implementation?
Review feedback on the previous commit. A deprecated function is now wrapped by typing_extensions.deprecated, the backport of PEP 702, rather than by a hand-written wrapper: it warns with the caller's stacklevel and sets the __deprecated__ attribute that type checkers read. Classes keep their own wrapper around __init__. PEP 702 warns when a deprecated class is subclassed as well as when it is instantiated, and Rowtime and Schema extend the deprecated Descriptor at module level, so adopting it for classes would warn when pyflink.table.descriptors is imported -- the behaviour this ticket is fixing. For the same reason a staticmethod and a classmethod are still unwrapped and re-packaged: typing_extensions.deprecated rejects a classmethod, and on Python 3.10 and later it turns a staticmethod into a plain function that then fails when called on an instance. Both cases now have regression tests. typing_extensions has been imported by this module since FLINK-37365 without being declared as a dependency; it resolved only because apache-beam happens to require it. It is now declared, with the 4.5.0 floor that typing_extensions.deprecated needs. The guards around the docstring assignment and around replacing __init__ are gone: no decorated object reaches them. The guard around recording __stability_decorators stays, because a property does reach it and rejects attribute assignment. The test helper installed its warning filter before entering catch_warnings, which leaked "always" into the global filter state for the rest of the session; it is a context manager now. Assertions that something warns use assertWarns, and the helper is only used where it cannot: that nothing warned, or that something warned exactly once. Generated-by: Claude Code 2.1.252 (Claude Opus 5)
github-actions
Bot
added
the
community-reviewed
deepyaman
commented
Sep 3, 2026
Comment on lines
+38
to
+48
| @contextlib.contextmanager | ||
| def _catch_warnings(): | ||
| """ | ||
| Returns a context manager recording every warning raised within it. | ||
| Records every warning raised within the block. | ||
|
|
||
| Used where :func:`unittest.TestCase.assertWarns` cannot express the assertion: that | ||
| nothing warned, or that something warned exactly once. | ||
| """ | ||
| context = warnings.catch_warnings(record=True) | ||
| warnings.simplefilter("always") | ||
| return context | ||
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter("always") | ||
| yield caught |
Contributor
Author
There was a problem hiding this comment.
Still, instead of doing this, why can't we just us pytest.warns? Would there be some incompatibility with unittest? I assume not...
| @@ -131,33 +150,41 @@ class Cls(object): | |||
Contributor
Author
There was a problem hiding this comment.
What's with all these Cls inheriting from object? Isn't that more of a Python 2 construct?
Comment on lines
166
to
+176
| @@ -174,57 +170,50 @@ def __call__(self, func_or_cls: T) -> T: | |||
| if isclass(func_or_cls): | |||
| self._deprecate_class(func_or_cls) | |||
| elif isfunction(func_or_cls): | |||
| return cast(T, self._deprecate_function(func_or_cls)) | |||
| # Anything else (a property, for instance) cannot be wrapped without changing what the | |||
| # decorated name refers to, so the docstring directive is all we apply. | |||
| # PEP 702's implementation, by way of its typing_extensions backport: a | |||
| # functools.wraps wrapper that warns with the caller's stacklevel, plus the | |||
| # __deprecated__ attribute that type checkers read. | |||
| return cast(T, deprecated(self._get_message(func_or_cls))(func_or_cls)) | |||
Contributor
Author
There was a problem hiding this comment.
Are these casts wholly necessary? If it's necessary for type checking, I get it, but just want to be sure.
Comment on lines
+189
to
+190
| and PyFlink subclasses its own deprecated classes -- Rowtime and Schema in | ||
| pyflink.table.descriptors both extend the deprecated Descriptor -- so that warning |
Contributor
Author
There was a problem hiding this comment.
Is this level of detail re specific examples necessary for a docstring?
Review feedback on the previous commits. No behaviour change. Comments and docstrings that restated the code, or carried detail that belongs in the commit message, are cut back to the reasoning a reader needs: why the warning cannot be emitted at decoration time, why typing_extensions.deprecated covers functions but not classes, and why a property is only documented. The class names that illustrated the subclassing problem are gone from the docstring, since they would rot as pyflink.table.descriptors changes. The test fixtures no longer inherit from object explicitly. Generated-by: Claude Code 2.1.252 (Claude Opus 5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Deprecatedinflink-python/pyflink/util/api_stability_decorators.pyemitted itsDeprecationWarningfrom__call__, which the decorator syntax invokes in order to apply the decorator. The warning therefore fired at decoration time — that is, at import — and the decorated function/class was returned unwrapped, so:pyflink.tableemitted aDeprecationWarningfor every deprecated API it defines, whether or not the user touches them;stacklevel=2pointed at the decoration site inside PyFlink's own source, not at user code.FLINK-37365, which introduced these decorators, describes the intended behaviour as warning "at runtime on their invocation", so this was an oversight. PyFlink supports Python >= 3.9, so
warnings.deprecated(PEP 702) is not available; the fix is by hand, mirroring PEP 702's semantics where reasonable.Brief change log
Deprecatedapplied to a function returns afunctools.wrapswrapper that warns when the function is called, withstacklevel=2so the warning is attributed to the caller.Deprecatedapplied to a class returns the class itself and wraps__init__on it, soisinstancechecks and subclassing are unaffected. As in PEP 702 only instantiating the deprecated class itself warns, which also avoids warning twice when a deprecated class inherits the__init__of a deprecated base class.staticmethod/classmethodobjects are unwrapped, decorated and re-packaged; properties, ABCs andEnumsubclasses fall back to applying the docstring directive alone rather than raising. Decorating any of these previously failed, as did omitting thedetailargument.DeprecationWarningcategory, the docstring/Sphinx directives and the__stability_decoratorsattribute read byPythonAPICompletenessTestCaseare unchanged;Experimental,Internal,PublicandPublicEvolvingare untouched.pyflink/utilwas not among the modulesdev/integration_test.shruns, so it is added there.Verifying this change
This change added tests and can be verified as follows:
pyflink/util/tests/test_api_stability_decorators.py: no warning at decoration; a regression test that importspyflink.tablein a fresh interpreter and asserts no deprecation warning is emitted; warning on function call and on class instantiation; the warning is attributed to the caller's file and line; docstring directives still applied;__stability_decoratorsstill populated;staticmethod/classmethod/property/ABC/Enumcases; subclassing and double-warning guards; the other four decorators still silent and still returning their argument unchanged.python -W error::DeprecationWarning -c "import pyflink.table"no longer raises, and that callingTable.get_schemawarns exactly once, pointing at the calling script.flake8andmypyas configured inflink-python/tox.iniare clean, and the Sphinx docs build (SPHINXOPTS="-a -W" make html) still succeeds, withDeprecated since version 2.1.0still rendered on the affected APIs.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code 2.1.252 (Claude Opus 5)