Add expected BC-breaking change warning to soundfile #906
Conversation
torchaudio/backend/utils.py
Outdated
| warnings.warn( | ||
| '"soundfile" backend is planned to change the function signatures in 0.9.0. ' | ||
| 'Please refer to https://github.com/pytorch/audio/issues/903') |
There was a problem hiding this comment.
When I see a warning like this, I think "Ok, how can I change my code to migrate and suppress the warning?". This warning may give the impression soundfile should not be used, which is not the case.
If we want to warn, then I'd say we should have a mechanism for the user to use the new interface right away.
There was a problem hiding this comment.
(offline comment on internal document)
mthrok
force-pushed
the
warning-soundfile
branch
from
27ca7d2 to
9896f86
Compare
There was a problem hiding this comment.
As mentioned offline, I'd suggest instead:
- For
out=,signalinfo=,encodinginfo=,filetype, inloadandprecisioninsave, I would simply raise a warning if different from default and then remove. - For
normalization=andoffset=, I would simply create the new parameter, and tell user to switch, since it will be removed.
That being said, I'm also ok with the current warning, so I'll approve the PR so you can decide what to do and move forward :)
mthrok
force-pushed
the
warning-soundfile
branch
from
9896f86 to
d0d221f
Compare
| if compression is not None: | ||
| warnings.warn( | ||
| '"soundfile" backend\'s `save` function does not support changing compression level. ' | ||
| '`compression` argument is silently ignore.' |
There was a problem hiding this comment.
will this parameter be removed?
| elif frame_offset != 0: | ||
| offset = frame_offset |
|
|
||
| if compression is not None: | ||
| warnings.warn( | ||
| '"soundfile" backend\'s `save` function does not support changing compression level. ' |
There was a problem hiding this comment.
Do you have an issue that gives more detail for this?
| @@ -34,8 +35,26 @@ def load(filepath: str, | |||
| offset: int = 0, | |||
| signalinfo: SignalInfo = None, | |||
| encodinginfo: EncodingInfo = None, | |||
| filetype: Optional[str] = None) -> Tuple[Tensor, int]: | |||
| filetype: Optional[str] = None, | |||
| *, | |||
| warnings.warn( | ||
| '"soundfile" backend is planned to change the function signatures in 0.8.0. ' | ||
| 'Please refer to https://github.com/pytorch/audio/issues/903 for the migration step.') |
There was a problem hiding this comment.
the real issue behind this particular comment is the return type? then it could be specialized to return type?
There was a problem hiding this comment.
how about a local parameter that controls the return type in the load/save function?
| r"""See torchaudio.save""" | ||
|
|
||
| if precision: | ||
| warnings.warn( | ||
| '"precision" argument is depricated and removed in 0.8.0. In 0.8.0, ' |
| warnings.warn( | ||
| 'The return type of `load` function in "soundfile" backend is going to be changed in 0.8.0. ' | ||
| 'Please refer to https://github.com/pytorch/audio/issues/903 for the detail.' | ||
| ) |
There was a problem hiding this comment.
that's fine to me. next best thing to creating a new backend?
See #903
Superseded by #922