Refactor backend and not rely on global variables on switching #698

mthrok · 2020-06-05T03:14:35Z

Do not rely on global variables for backend switch
So that load/save/info/load_wav functions will be torchscript-able
Add no_backend module to for the case there is no backend module available
[bonus] This allows the whole codebase importable on systems that do not have torchaudio C++ extension nor soundfile.

codecov · 2020-06-09T00:40:22Z

Codecov Report

Merging #698 into master will increase coverage by 0.01%.
The diff coverage is 92.13%.

@@            Coverage Diff             @@
##           master     #698      +/-   ##
==========================================
+ Coverage   89.03%   89.05%   +0.01%     
==========================================
  Files          27       28       +1     
  Lines        2417     2467      +50     
==========================================
+ Hits         2152     2197      +45     
- Misses        265      270       +5

Impacted Files	Coverage Δ
torchaudio/__init__.py	`71.42% <ø> (-13.19%)`	⬇️
torchaudio/backend/__init__.py	`100.00% <ø> (ø)`
torchaudio/backend/no_backend.py	`76.47% <76.47%> (ø)`
torchaudio/backend/utils.py	`87.80% <91.89%> (+1.13%)`	⬆️
torchaudio/backend/common.py	`100.00% <100.00%> (ø)`
torchaudio/backend/soundfile_backend.py	`96.77% <100.00%> (+0.54%)`	⬆️
torchaudio/backend/sox_backend.py	`86.86% <100.00%> (+1.31%)`	⬆️

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 b822580...9ebd0ed. Read the comment docs.

1. Do not rely on global variables for backend switch So that load/save/info/load_wav functions will be torchscript-able 2. Add no_backend module to for the case there is no backend module available [bonus] This allows the whole codebase importable on systems that do not have torchaudio C++ extension nor soundfile.

mthrok · 2020-06-12T13:21:56Z

test/test_datasets.py

@@ -29,11 +29,13 @@ def test_librispeech(self):
        data[0]

    @unittest.skipIf("sox" not in common_utils.BACKENDS, "sox not available")
+    @common_utils.AudioBackendScope('sox')


Does this fixes #715 completely?

Nope. This is a temporary fix. #719 gives the principled fix which fully utilizes no_backend from this PR.

vincentqb · 2020-06-12T17:27:55Z

torchaudio/backend/common.py

+def _impl_load(func):
+    setattr(func, '__doc__', _LOAD_DOCSTRING)
+    return func
+
+
+def _impl_load_wav(func):
+    setattr(func, '__doc__', _LOAD_WAV_DOCSTRING)
+    return func
+
+
+def _impl_save(func):
+    setattr(func, '__doc__', _SAVE_DOCSTRING)
+    return func
+
+
+def _impl_info(func):
+    setattr(func, '__doc__', _INFO_DOCSTRING)
+    return func


nit: how about calling them _add_docstring_* instead?

~~Instead of having these functions, we could also use "no backend" as the template to grab the docstring from.~~

torchaudio/backend/utils.py

vincentqb · 2020-06-12T19:56:51Z

torchaudio/backend/utils.py

-        raise RuntimeError('Backend is not initialized.')
-    return _BACKENDS[_BACKEND]
+    for func in ['save', 'load', 'load_wav', 'info']:
+        setattr(torchaudio, func, getattr(module, func))


Let's just make sure we make that conscious choice: torchaudio.backend.__init__ invokes utils._init_audio_backend() which then modifies torchaudio.

Would it be advantageous to attach load, save, etc directly in torchaudio.__init__ instead?

I am not sure what you mean by advantageous here, but the current approach gives cleaner separation of torchaudio.__init__ and torchaudio.backend because backend module provides 1. backend implementations, 2. way to manipulate backend, and 3. way to fetch information about current backend and available backends. This way, all the responsibilities on backend is closed within torchaudio.backend module and the only contract between torchaudio.__init__ and torchaudio.backend is that torchaudio.load/save/load_wav/info attributes are reserved for torchaudio.backend module (implying that as long as torchaudio.__init__ does not touch these attributes, torchaudio.backend understands everything about it), and these attributes are the only attributes outside of torchaudio.backend that torchaudio.backend module is allowed to touch.

If the initialization is moved to torchaudio.__init__, that means there are more contract between torchaudio.__init__ and torchaudio.backend, like how the backend switch function works, and it requires torchaudio.__init__ to know the internal mechanics of torchaudio.backend. This introduces more coupling between torchaudio.__init__ and torchaudio.backend and adds maintenance cost.

My main concern was the type of coupling that led to #712, when one module modifies another.

If I understand what you are saying: in order to avoid having the two depending on each other, we would need to bring torchaudio.backend completely within torchaudio/__init__.py. Is that what you are saying?

btw I am not advocating this :)

My main concern was the type of coupling that led to #712, when one module modifies another.

That's different. #712 was about new classes being defined inside of a module when a module function called from outside call.
Refactoring of backend switch is about achieving the proper Separation on Concerns, by letting the backend module handle everything about backend, including initialization, so that main torchaudio module does not need to.

If I understand what you are saying: in order to avoid having the two depending on each other, we would need to bring torchaudio.backend completely within torchaudio/__init__.py. Is that what you are saying?

No, that's totally opposite of what I am suggesting. It's better maitainable if torchaudio/__init__.py knows nothing about backend internal mechanism.

Let me rephrase my concern. If I read __init__, I don't have any way of knowing that load/save/etc are being added to it since it's added by another module. Am I missing something?

How about setting the attribute to torchaudio.backend and simply importing in torchaudio/__init__.py? Thoughts?

Let me rephrase my concern. If I read __init__, I don't have any way of knowing that load/save/etc are being added to it since it's added by another module. Am I missing something?

Yes, that's the correct way to achieve the Separate of Concerns.

How about setting the attribute to torchaudio.backend and simply importing in torchaudio/__init__.py? Thoughts?

No that way, the responsibility of backend switching is spread to torchaudio/__init__.py and torchaudio.backend, which introcudes more coupling. That's anti-pattern.

Just adding some comment about save/load/info assigned in torchaudio.backend would suffice.

vincentqb · 2020-06-12T19:58:34Z

test/test_datasets.py

@@ -29,11 +29,13 @@ def test_librispeech(self):
        data[0]

    @unittest.skipIf("sox" not in common_utils.BACKENDS, "sox not available")
+    @common_utils.AudioBackendScope('sox')


Does this fixes #715 completely?

vincentqb

LGTM!

mthrok · 2020-06-16T02:31:37Z

thanks!

mthrok force-pushed the backend-switch branch 17 times, most recently from 8b2acce to 49ca6a2 Compare June 9, 2020 00:29

mthrok force-pushed the backend-switch branch 11 times, most recently from 60b0fee to e2643cd Compare June 11, 2020 23:52

mthrok changed the title ~~Do not rely on global variables for backend switch~~ Refactor backend switching Jun 11, 2020

mthrok force-pushed the backend-switch branch 3 times, most recently from 6f6ff0f to d84a53e Compare June 12, 2020 00:14

mthrok force-pushed the backend-switch branch from d84a53e to 3e710a8 Compare June 12, 2020 00:36

add sox backend

9ebd0ed

mthrok mentioned this pull request Jun 12, 2020

Backend is not deterministic in test #715

Closed

mthrok changed the title ~~Refactor backend switching~~ Refactor backend and not rely on global variables on switching Jun 12, 2020

mthrok marked this pull request as ready for review June 12, 2020 13:13

mthrok requested a review from vincentqb June 12, 2020 13:13

mthrok commented Jun 12, 2020

View reviewed changes

vincentqb reviewed Jun 12, 2020

View reviewed changes

vincentqb approved these changes Jun 15, 2020

View reviewed changes

vincentqb mentioned this pull request Jun 15, 2020

Refactor test backend #719

Merged

mthrok merged commit e9f19c3 into pytorch:master Jun 16, 2020

mthrok deleted the backend-switch branch June 16, 2020 02:31

mthrok pushed a commit to mthrok/audio that referenced this pull request Feb 26, 2021

Fix tutorial comment typing error (pytorch#698)

feca7bc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor backend and not rely on global variables on switching #698

Refactor backend and not rely on global variables on switching #698

mthrok commented Jun 5, 2020 •

edited

codecov bot commented Jun 9, 2020 •

edited

mthrok Jun 12, 2020

vincentqb Jun 12, 2020

mthrok Jun 12, 2020 •

edited

vincentqb Jun 12, 2020

vincentqb Jun 12, 2020

mthrok Jun 12, 2020

vincentqb Jun 15, 2020

mthrok Jun 15, 2020

vincentqb Jun 15, 2020 •

edited by mthrok

mthrok Jun 15, 2020

vincentqb Jun 12, 2020

vincentqb left a comment

mthrok commented Jun 16, 2020

Refactor backend and not rely on global variables on switching #698

Refactor backend and not rely on global variables on switching #698

Conversation

mthrok commented Jun 5, 2020 • edited

codecov bot commented Jun 9, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthrok Jun 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentqb Jun 15, 2020 • edited by mthrok

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentqb left a comment

Choose a reason for hiding this comment

mthrok commented Jun 16, 2020

mthrok commented Jun 5, 2020 •

edited

codecov bot commented Jun 9, 2020 •

edited

mthrok Jun 12, 2020 •

edited

vincentqb Jun 15, 2020 •

edited by mthrok