-
Notifications
You must be signed in to change notification settings - Fork 721
Add deprecation warnings #3924
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
Add deprecation warnings #3924
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/audio/3924
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Hi @samanklesaria! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this @samanklesaria ! I made a few comments below, let's chat more during our sync
src/torchaudio/_backend/utils.py
Outdated
@@ -115,6 +116,7 @@ def dispatcher( | |||
return backend | |||
raise RuntimeError(f"Couldn't find appropriate backend to handle uri {uri} and format {format}.") | |||
|
|||
@dropping_support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be easier for us to keep track of what is deprecated at a glance if we add the decorator in the __init__.py
files, on the public functions. This way, we can just look at the __init__.py
and be sure we didn't miss anything.
Do you mind moving the @dropping_support
there ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure I understand here: in the __init__.py
files, we usually export a function foo
with from submodule import foo
(and then including "foo"
in __all__
). You're proposing we change each of these imports to something like the following:
from submodule import foo as foo_deprecated
foo = drop_support(foo_deprecated)
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I had in mind yes. I haven't checked but I hope it should work? To avoid foo_deprecated
to be public, I think we'd just need to import it as:
from submodule import foo as _foo_deprecated
# or shorter
from submodule import foo as _foo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the code to use these pattern wherever __init__.py
files exist. Not all submodules are structured this way though (for example kaldi_io
), so when there isn't an init file, I keep the deprecations inline. Also: as every single function in the prototype
directory is being deprecated, I've kept those deprecations inline as well.
@@ -94,6 +94,9 @@ def wrapped(*args, **kwargs): | |||
|
|||
return decorator | |||
|
|||
dropping_support = deprecated( | |||
"As TorchAudio is no longer being actively developed, this function can no longer be supported." | |||
"See https://github.com/pytorch/audio/issues/3902 for more details.", version="2.9", remove=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see there are already some depreaction utils, and good job on re-using those!
We can work towards consolidating the exact message in future PRs. For now, I think we should at least have 2 decorators:
- one for the io functionality like
load
,save
etc. above, inviting users to migrate to TorchCodec - one for general functions that are getting removed
|
||
__all__ = [] | ||
|
||
|
||
@dropping_support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the other comment above, we should try to add the deprecations in the __init__.py
files when possible.
@@ -1760,6 +1760,7 @@ def _fix_waveform_shape( | |||
return waveform_shift | |||
|
|||
|
|||
@dropping_support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of the functional, we should make sure their corresponding transforms classes in transforms/
are deprecated too. Here there is the RNNTLoss
class.
I was going to comment that we should move the deprecation to __init__
, but unfortunately this file is technically public, because it has no leading underscore anywhere: src/torchaudio/functional/functional.py
. So it's possible that some users have been importing things as from torchaudio.functional.functional improt XYZ
instead of the intended from torchaudio.functional improt XYZ
. As a result, I think we'll have to keep the deprecations in this file, instead of in torchaudio/functional/__init__.py
, which would have been more convenient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the version of RRNTLoss
in transforms
itself calls the version in functional
, putting a deprecation warning in both places would show duplicate warnings. I guess the way to get around this would be to have the version in transforms
call an un-deprecated version of the on in functional
instead?
|
||
CodecConfig.__init__ = dropping_support(CodecConfig.__init__) | ||
StreamReader.__init__ = dropping_support(StreamReader.__init__) | ||
StreamWriter.__init__ = dropping_support(StreamWriter.__init__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm interested to see if it works! Let's discuss tests during our chat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to work when testing interactively.
The codebase already uses a |
I think it'll be easier for us if we don't touch anything that has already been deprecated, at least for the time being. It helps to keep the new deprecations we're introducing separate from the rest, which is why I like that you created a new |
8691e58
to
0d030bf
Compare
try: | ||
func() | ||
except: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's a lot simpler than what I anticipated.
For my own ref and for @scotts, I was surprised this would even work because I thought the call to func()
would raise a loud TypeError
error before we even get a chance to raise the deprecation warning. But the @deprecated
decorator that we use actually raises the warning before entering the underlying function, so this is correct.
I would just suggest to make it slightly more robust with the following changes:
- Use
pytest
andpytest.mark.parametrize()
. Turns out torchaudio already invokes pytest, they just don't use it in the test, probably for historical reasons. But there's no reason we can't do that anymore. - assert the type of the raised exception
import pytest
from torchaudio._internal.module_utils import UNSUPPORTED
@pytest.mark.parametrize("func", UNSUPPORTED)
def test_deprecations(func):
with pytest.warns(UserWarning, match="deprecated"):
try:
func()
except Exception as e:
# Type or Runtime error because we call func() without proper parameters.
# The deprecation warning is still properly raised, since it is emitted before
# the underlying (deprecated) function is called.
assert isinstance(e, (TypeError, RuntimeError))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR @samanklesaria . I made a suggestion above for the test, but otherwise I think we can merge the PR as-is and open follow-up PRs. I'll sync with you offline to discuss what's next.
CI is still red, but the newly added tests are passing locally.
This PR adds deprecation warnings to all IO functions, prototypes, and functionality requiring C++ extensions as discussed in #3902.