Add deprecation warning to initialize/shutdown_sox #709

mthrok · 2020-06-09T03:01:42Z

torchaudio.initialize_sox (and corresponding torchaudio.shutdown_sox) is a confusing function because it sounds like this function has to be called to use any sox functionality though it is required only for SoX Effects. This PR proposes to deprecate these functions and direct users to use torchaudio.sox_effects.init_sox_effects (and torchaudio.sox_effects.shutdown_sox_effects), if necessary.

codecov · 2020-06-09T03:11:04Z

Codecov Report

Merging #709 into master will decrease coverage by 0.12%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master     #709      +/-   ##
==========================================
- Coverage   89.16%   89.03%   -0.13%     
==========================================
  Files          27       27              
  Lines        2399     2417      +18     
==========================================
+ Hits         2139     2152      +13     
- Misses        260      265       +5

Impacted Files	Coverage Δ
torchaudio/sox_effects/sox_effects.py	`87.50% <ø> (ø)`
torchaudio/__init__.py	`84.61% <71.42%> (-4.86%)`	⬇️
torchaudio/_internal/module_utils.py	`85.18% <72.72%> (-8.57%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50939b7...09275e0. Read the comment docs.

cpuhrsch · 2020-06-10T02:13:28Z

torchaudio/_internal/module_utils.py

@@ -33,3 +34,14 @@ def wrapped(*args, **kwargs):
                raise RuntimeError(f'{func.__module__}.{func.__name__} requires {req}')
            return wrapped
    return decorator
+
+
+def depricate(direction: str):


Does pytorch already have something like this? I imagine we'd want to match their format.

Also, nit, it's deprecate.

Does pytorch already have something like this? I imagine we'd want to match their format.

Quick search reveals they do not have one. It's all manually annotated.
https://github.com/pytorch/pytorch/search?l=Python&q=deprecated

PyTorch's convention in deprecation I know is that they use warnings.warn, which emits UserWarning.

Also, nit, it's deprecate.

that's not nit, very important one. I'm gonna fix it.

vincentqb

Let's replace initialize_sox and shutdown_sox in torchaudio/__init__.py by functions that do not do anything but raise a warning. We then make init_sox_effects and shutdown_sox_effects in torchaudio.sox_effects private. This way we send a clear signal that users no longer need them.

vincentqb · 2020-06-10T18:17:25Z

torchaudio/__init__.py

+    "Use `torchaudio.sox_effects.init_sox_effects`, or simply remove the call. "
+    "It is automatically handled."


nit:

sox_effects is now automatically initialized and shutdown, and this function doesn't do anything. Please remove this function to suppress the warning, and see torchaudio.sox_effects.init_sox_effects for more information.

I agree that recommending to remove the function call should be the first thing, instead of second option. but I argue this function doesn't do anything. is not quite accurate and shutdown_sox_effects allows user code to release the resources early.

torchaudio/__init__.py

vincentqb · 2020-06-10T18:17:50Z

torchaudio/__init__.py

+    "Use `torchaudio.sox_effects.shutdown_sox`, or remove the call. "
+    "It is automatically handled."


same as above

torchaudio/__init__.py

vincentqb · 2020-06-10T18:19:07Z

torchaudio/_internal/module_utils.py

@@ -33,3 +34,19 @@ def wrapped(*args, **kwargs):
                raise RuntimeError(f'{func.__module__}.{func.__name__} requires {req}')
            return wrapped
    return decorator
+
+
+def deprecated(direction: str):


What does this decorator provide more than just adding the warning in the wrapping class?

It give the consistent message format. We are going to deprecate a bunch of functions related to the current sox backend such as sox_signalinfo_t so it will make it easier to do the job.

torchaudio/__init__.py

torchaudio/sox_effects/sox_effects.py

mthrok · 2020-06-10T22:13:47Z

Let's replace initialize_sox and shutdown_sox in torchaudio/__init__.py by functions that do not do anything but raise a warning.

I am not sure about this. There could be some funky situations where user code needs to invoke init/shutdown manually. (such as multi-processing environment) in that case we want to have torchaudio.initialize_sox and torchaudio.shutdown_sox to do the actual work, otherwise such existing code stops working. In that case, user code has to change the function invocation to torchaudio.sox_effects.init/shutdown, that is BC breaking.

vincentqb · 2020-06-10T22:30:36Z

I am not sure about this. There could be some funky situations where user code needs to invoke init/shutdown manually. (such as multi-processing environment)

I see. Do you have an example that requires init/shutdown, and that the automatic init/shutdown fails?

mthrok · 2020-06-11T00:16:07Z

I am not sure about this. There could be some funky situations where user code needs to invoke init/shutdown manually. (such as multi-processing environment)

I see. Do you have an example that requires init/shutdown, and that the automatic init/shutdown fails?

Nope.

mthrok · 2020-06-11T15:12:02Z

Let's replace initialize_sox and shutdown_sox in torchaudio/__init__.py by functions that do not do anything but raise a warning.

After I gave some more thoughts on this suggestion, I think this suggestion is going the opposite of what PR is proposing here.
We want to let users know that we are deprecating these functions and give directions for migrations.
We want to do that without changing the current functionalities. So that it will not disturb the user code, while we can better organize our code base.
Changing it to pass is changing the functionalities without changing the interface. This could give a very bad experience to users, because, on the surface, their code seems to work as the way it was, but it could change the internal state of their applications. Internal state here is the memory state of C++ extension, and so these change does not surface to Python code easily, and errors might happen at somewhere totally different, which is very difficult to debug.

This way we send a clear signal that users no longer need them.

I do not think it's a reasonable expectation that users read the source code and understand the intention in the code. We are issuing the message through warning and clearly stating that it's been deprieated. Changing the code to pass would not add more value to the message and would not reach to endusers.

vincentqb · 2020-06-11T16:28:08Z

Changing it to pass is changing the functionalities without changing the interface.

"pass" or "same as before with warning" are fine by me. We can go with the latter to avoid changing the behavior of current codes.

That being said, init should already be a non-action since sox is initialized at import, no? shutdown isn't.

I am not sure about this. There could be some funky situations where user code needs to invoke init/shutdown manually. (such as multi-processing environment)

I see. Do you have an example that requires init/shutdown, and that the automatic init/shutdown fails?

Nope.

Since sox is automatically initialized at import, your concern might be that in some cases we have to shutdown sox manually for the code to continue without errors?

mthrok · 2020-06-11T16:57:42Z

Since sox is automatically initialized at import, your concern might be that in some cases we have to shutdown sox manually for the code to continue without errors?

yes

mthrok · 2020-06-11T18:13:02Z

Rebased on #714

vincentqb

Since sox is automatically initialized at import, your concern might be that in some cases we have to shutdown sox manually for the code to continue without errors?

yes

This means we foresee rare events where a user may in fact need to shutdown sox. In that case, I don't see a reason to deprecate the function(s) at all. We could move it to torchaudio.sox_effects.shutdown_sox_effects., but then this adds friction.

[If we don't deprecate shutdown, then we could also not deprecate init.]

vincentqb · 2020-06-11T20:22:44Z

torchaudio/__init__.py

@@ -34,6 +35,26 @@
    pass


+@_mod_utils.deprecated(
+    "Please remove the function call. Resource initialization is automatically handled")


nit: "Please remove the function call to initialize_sox. Resource initialization is now automatically handled."

vincentqb · 2020-06-11T20:26:10Z

torchaudio/__init__.py

+
+
+@_mod_utils.deprecated(
+    "Please remove the function call. Resource clean up is automatically handled")


nit: In case we would indeed move to deprecate this function: "Please remove the function call to torchaudio.shutdown_sox. Resource clean up is now automatically handled. In the unlikely event that you need to manually shutdown sox, please use torchaudio.sox_effects.shutdown_sox_effects."

mthrok · 2020-06-11T21:31:59Z

Since sox is automatically initialized at import, your concern might be that in some cases we have to shutdown sox manually for the code to continue without errors?

yes

This means we foresee rare events where a user may in fact need to shutdown sox. In that case, I don't see a reason to deprecate the function(s) at all. We could move it to torchaudio.sox_effects.shutdown_sox_effects., but then this adds friction.

[If we don't deprecate shutdown, then we could also not deprecate init.]

That is why I originally kept it as public function.
Like mentioned in the PR description, initialize_sox is a misnomer. It is confusing and giving a very bad user experience. It should never have been defined in torchaudio.__init__. Now that everything has been properly modularized in sox_effects. We encourage users to call the function from there, not on torchaudio. That's the whole point of this PR.

For example of confusion; https://github.com/pytorch/audio/blob/master/test/test_sox_effects.py#L24-L25
This is the decorator you added, and I was not aware of how backend works back then, but sox_effects and sox backend works completely independent and it has no coupling between them, but you thought there was. If you are confused, then general users are more confused, so it is time to make the interface around sox clean and straight forward.

This reverts commit 10b35d2.

This reverts commit 2187348.

vincentqb · 2020-06-11T23:19:29Z

Following of our offline discussion :)

The reason skipIf and AudioBackendScope were used was that the goal at the time was to disable sox and sox_effects completely if the backend selected wasn't sox. This goal has evolved :)
torchaudio.initialize_sox and torchaudio.shutdown_sox are moved to torchaudio.sox_effects.init_sox_effects and torchaudio.sox_effects.shutdown_sox_effects respectively to clarify their meaning.
torchaudio.sox_effects.shutdown_sox_effects will remain since there may be cases where the user needs to shutdown sox_effects.
torchaudio.sox_effects.init_sox_effects may be removed in a later release as the initialization is done automatically when torchaudio is imported.

vincentqb

LGTM

mthrok · 2020-06-11T23:46:41Z

Thanks!

mthrok force-pushed the depre-sox-effect branch 2 times, most recently from 2a73754 to 9519310 Compare June 9, 2020 18:40

cpuhrsch reviewed Jun 10, 2020

View reviewed changes

mthrok force-pushed the depre-sox-effect branch from 2b54ec2 to b37e34c Compare June 10, 2020 15:40

mthrok marked this pull request as ready for review June 10, 2020 17:33

mthrok requested review from vincentqb and cpuhrsch June 10, 2020 17:33

vincentqb suggested changes Jun 10, 2020

View reviewed changes

mthrok force-pushed the depre-sox-effect branch from 0336b12 to e31fc54 Compare June 11, 2020 16:59

mthrok added 5 commits June 11, 2020 11:11

Add deprecation warning to initialize/shutdown_sox

0d55b4f

Fix typo and stacklevel

8245573

Update docstring

7e33362

Fix import

500bd07

Update message

c311db6

mthrok force-pushed the depre-sox-effect branch from e31fc54 to c311db6 Compare June 11, 2020 18:12

mthrok added 2 commits June 11, 2020 18:57

Move init/shutdown to private

2187348

fixup! Move init/shutdown to private

10b35d2

mthrok requested a review from vincentqb June 11, 2020 19:21

vincentqb reviewed Jun 11, 2020

View reviewed changes

mthrok added 2 commits June 11, 2020 21:37

Revert "fixup! Move init/shutdown to private"

bb1a0ff

This reverts commit 10b35d2.

Revert "Move init/shutdown to private"

8aeb482

This reverts commit 2187348.

Update message

09275e0

vincentqb approved these changes Jun 11, 2020

View reviewed changes

mthrok merged commit b822580 into pytorch:master Jun 11, 2020

mthrok deleted the depre-sox-effect branch June 11, 2020 23:46

mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023

Update .gitignore (pytorch#709)

2b2c40b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecation warning to initialize/shutdown_sox #709

Add deprecation warning to initialize/shutdown_sox #709

mthrok commented Jun 9, 2020 •

edited

codecov bot commented Jun 9, 2020 •

edited

cpuhrsch Jun 10, 2020

mthrok Jun 10, 2020 •

edited

vincentqb left a comment •

edited

vincentqb Jun 10, 2020

mthrok Jun 11, 2020 •

edited

vincentqb Jun 10, 2020

vincentqb Jun 10, 2020

mthrok Jun 11, 2020

mthrok commented Jun 10, 2020

vincentqb commented Jun 10, 2020

mthrok commented Jun 11, 2020

mthrok commented Jun 11, 2020

vincentqb commented Jun 11, 2020

mthrok commented Jun 11, 2020

mthrok commented Jun 11, 2020

vincentqb left a comment

vincentqb Jun 11, 2020

vincentqb Jun 11, 2020

mthrok commented Jun 11, 2020 •

edited

vincentqb commented Jun 11, 2020

vincentqb left a comment

mthrok commented Jun 11, 2020

		"Use `torchaudio.sox_effects.init_sox_effects`, or simply remove the call. "
		"It is automatically handled."

		"Use `torchaudio.sox_effects.shutdown_sox`, or remove the call. "
		"It is automatically handled."



		@_mod_utils.deprecated(
		"Please remove the function call. Resource clean up is automatically handled")

Add deprecation warning to initialize/shutdown_sox #709

Add deprecation warning to initialize/shutdown_sox #709

Conversation

mthrok commented Jun 9, 2020 • edited

codecov bot commented Jun 9, 2020 • edited

Codecov Report

cpuhrsch Jun 10, 2020

Choose a reason for hiding this comment

mthrok Jun 10, 2020 • edited

Choose a reason for hiding this comment

vincentqb left a comment • edited

Choose a reason for hiding this comment

vincentqb Jun 10, 2020

Choose a reason for hiding this comment

mthrok Jun 11, 2020 • edited

Choose a reason for hiding this comment

vincentqb Jun 10, 2020

Choose a reason for hiding this comment

vincentqb Jun 10, 2020

Choose a reason for hiding this comment

mthrok Jun 11, 2020

Choose a reason for hiding this comment

mthrok commented Jun 10, 2020

vincentqb commented Jun 10, 2020

mthrok commented Jun 11, 2020

mthrok commented Jun 11, 2020

vincentqb commented Jun 11, 2020

mthrok commented Jun 11, 2020

mthrok commented Jun 11, 2020

vincentqb left a comment

Choose a reason for hiding this comment

vincentqb Jun 11, 2020

Choose a reason for hiding this comment

vincentqb Jun 11, 2020

Choose a reason for hiding this comment

mthrok commented Jun 11, 2020 • edited

vincentqb commented Jun 11, 2020

vincentqb left a comment

Choose a reason for hiding this comment

mthrok commented Jun 11, 2020

mthrok commented Jun 9, 2020 •

edited

codecov bot commented Jun 9, 2020 •

edited

mthrok Jun 10, 2020 •

edited

vincentqb left a comment •

edited

mthrok Jun 11, 2020 •

edited

mthrok commented Jun 11, 2020 •

edited