API Wrap __new__ instead of __init__ with the deprecated decorator #25733
Conversation
Co-authored-by: Brigitta Sipőcz <brigitta.sipocz@gmail.com>
There was a problem hiding this comment.
Thanks for the PR! Do we need to change anything here?
scikit-learn/sklearn/tests/test_docstring_parameters.py
Lines 112 to 115 in 2c867b8
|
I'm going to test this against |
|
@adrinjalali please check with the last commit because I just pushed a fix |
Good catch, we do. And it actually found a bug. When overriding new, only the class can be passed. |
|
|
sklearn/utils/deprecation.py
Outdated
| warnings.warn(msg, category=FutureWarning) | ||
| return new(*args, **kwargs) | ||
| return new(cls) |
There was a problem hiding this comment.
If *args are not passed up, then the following fails:
@deprecated("a message")
class MockClass6:
def __new__(cls, *args, **kwargs):
assert args[0] == 42
# if super() is object, then only `cls` can be passed up
return super().__new__(cls)
def __init__(self, a):
self.a = a
def test_deprecated():
with pytest.warns(FutureWarning, match="a message"):
MockClass6(42)I do not see a great way around it besides catching the error and trying again:
def wrapped(cls, *args, **kwargs):
warnings.warn(msg, category=FutureWarning)
try:
return new(cls, *args, **kwargs)
except TypeError:
# object accepts a single argument
return new(cls)There was a problem hiding this comment.
This makes me wonder if we really want to move this logic to __new__. It's adding complexity somewhere else, only hoping weird things don't happen to __new__ in the child class I guess.
Since this is about deprecations and changes in parameters, should we just mark the original issue as "won't fix" and expect third party developers to handle it themselves?
There was a problem hiding this comment.
Last attempt: I pushed a new commit which should be safe.
It's adding complexity somewhere else, only hoping weird things don't happen to new in the child class I guess.
I wouldn't say "adding" but "moving". The current version in main already suffers from the same kind of issue:
@deprecated("a message")
class MockClass6:
def __new__(cls, a):
print("a", a)
return super().__new__(cls)
MockClass6(42)There was a problem hiding this comment.
However, if you still find this too brittle, I'm okay to close this PR and the original issue as won't fix.
There was a problem hiding this comment.
I played around with this a bit, I think I'm okay with the current implementation, but I'll let @thomasjpfan have another look.
Fixes #15994
Closes #15995
Although the original issue an PR are old, I think it's still relevant for third party developers.
When a class inherits from a deprecated class but completely overrides the
__init__method, the FutureWarning is not raised.This PR proposes to raise the warning in
__new__instead as an old comment suggested. Raising in both methods is also a possibility but then we would emit the warning twice which is not very user friendly.