Backend switch #355

vincentqb · 2019-11-26T00:08:44Z

Introduce a backend switch in a similar way to torchvision from pytorch/vision#153.

Offer an option to change backend to load files, as in torchvision (https://pytorch.org/docs/stable/torchvision/index.html).
Import sox only when sox is used at runtime, e.g. sox_effects or loading files.
Offer wrapper function to switch between backends for load/save. (Maintained current interface to avoid BC-breaking change.)
Move sox functions from __init__ to torchaudio.sox_backend. (torchaudio.sox_effects does not change.)
Add deprecation warning if calling sox functions using for instance torchaudio.initialize_sox.
Offer pysoundfile to experiment with new interface.
Add tests for soundfile backend.
Add test to load/save with different backends
~~Fix librosa test appearing here if not flaky? deactivate failing test #372~~

For later:

Add libsndfile directly (pysoundfile uses numpy arrays when wrapping libsndfile).
Add decorator to restrict which backend is supported by given function, see comment.
Make sure mechanism works in parallel context (e.g. could add a backend parameter to functions needing it)
Make sure mechanism is torchscriptable (e.g. torchscript currently does not support global variable)

Add to release notes:

SoxEffectsChain.EFFECTS_AVAILABLE replaced by SoxEffectsChain().EFFECTS_AVAILABLE

Comparison of backends
Internal doc

Fixes #329 by offering other backend as options. As such, this also is a first step in addressing #357.

test/test.py

vincentqb · 2019-12-03T20:14:32Z

torchaudio/__init__.py

-                                             encodinginfo,
-                                             filetype)
+    if get_audio_backend() == "sox":
+        waveform, sample_rate = sox_backend.load(


Another way to branch would be by doing a conditional import but with the same name

if get_audio_backend() == "sox": from sox_backend import load elif get_audio_backend() == "soundfile": from _soundfile_backend import load else: raise NotImplementedError waveform, sample_rate = load( filepath, out=out, normalization=normalization, channels_first=channels_first, num_frames=num_frames, offset=offset, filetype=filetype, )

Thoughts?

torchvision uses the backend switch like so but most of the time simply uses PIL.

You might be able to save on these if statements if you overwrite the load functions etc. when switching backends, but i'd almost chalk that up under a performance optimization, so it's not necessary yet

vincentqb · 2019-12-03T20:29:33Z

@cpuhrsch -- For this PR, I'd focus on the interface for the user, and leave a new backend for later. This still addresses the main issue of completely blocking the import of torchaudio when there is an issue with sox. Thoughts?

test/test.py

cpuhrsch · 2019-12-03T21:03:45Z

torchaudio/__init__.py

+    """
+    Specifies the package used to load.
+    Args:
+        backend (string): Name of the backend. one of {'sox'}.


nit: add soundfile

Having references to the sources of those backends in the doctstring could be useful too

cpuhrsch · 2019-12-03T21:06:19Z

torchaudio/__init__.py

-                                             encodinginfo,
-                                             filetype)
+    if get_audio_backend() == "sox":
+        waveform, sample_rate = sox_backend.load(


You could still assign to a local load function and then move these branches higher (which will save on indentations) and make the code more readable

cpuhrsch · 2019-12-03T21:08:27Z

Looks good so far!

From what I gather from the PR description there are no BC-breaking changes introduced here?

cpuhrsch · 2019-12-03T21:11:20Z

test/test.py

+                    x_sine_part, _ = torchaudio.load(
+                        input_sine_path, num_frames=num_frames, offset=offset
+                    )
+                    l1_error = (


I don't want to be "that guy", but these code format changes make this harder to review. You could make them the last commit and continue to maintain that, or you could send a separate PR later on. They also introduce a lot of meaningless git blame changes.

Yeah, (1) this was definitely not meant to be in, and (2) this was not quite ready for review :)

cpuhrsch · 2019-12-03T21:13:12Z

torchaudio/__init__.py

-                 filetype=None):
-    r"""Saves a tensor of an audio signal to disk as a standard format like mp3, wav, etc.
+    if get_audio_backend() == "sox":
+        from torchaudio import sox_backend


I'd carefully make sure that these repeated imports aren't expensive due to some kind of initialization code in sox.

or soundfile for that matter

One way of avoiding repeated import is to import at the beginning and catch import errors, as done in vision. However, local test on my mac don't seem to see a cost to repeated import and seem to properly fetch the cached version:

In [1]: %timeit import torchaudio The slowest run took 17.73 times longer than the fastest. This could mean that an intermediate result is being cached. 637 ns ± 1.07 µs per loop (mean ± std. dev. of 7 runs, 1 loop each) In [2]: %timeit import _torch_sox 76.3 ns ± 1.84 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

cpuhrsch · 2019-12-03T21:15:20Z

torchaudio/__init__.py

-    src = src.contiguous()
-    _torch_sox.write_audio_file(filepath, src, signalinfo, encodinginfo, filetype)
+
+def save_encinfo(*args, **kwargs):


The current docs for encinfo don't even reference the function in the example. This is a strange one.

cpuhrsch · 2019-12-03T21:16:23Z

torchaudio/__init__.py


+def sox_encodinginfo_t(*args, **kwargs):


encoding information, signal information etc. are very useful functions in general. we could also think about adding some backend independent interfaces, but maybe after the release.

cpuhrsch · 2019-12-03T21:18:04Z

torchaudio/_soundfile_backend.py

+    num_frames=0,
+    offset=0,
+    filetype=None,
+):


I notice you're repeating the docstring here. But you call into the load backends separately. I don't think the user will be able to see this unless she looks at this function which is part of a private backend. Here assigning a backend function to the module unction could help use actually choose the correct doc string at runtime.

However, the static documentation won't be able to do that.

So instead I'd say it makes sense to have a single docstring for our load function and then reference it here.

When you say "reference", I assume you mean

"""See torchaudio.save"""

Unless you meant copying docstring from another with something like functools.wraps() or @functools.docs-decorator as you mentioned in this comment?

Yes that's correct.

torchaudio/_soundfile_backend.py

torchaudio/sox_effects.py

torchaudio/_soundfile_backend.py

vincentqb · 2019-12-05T23:03:08Z

I can't reproduce locally the librosa error.

❯ conda create -n librosa-conda python=3.7
❯ conda activate librosa-conda
❯ conda install -c pytorch pytorch
❯ conda install -c conda-forge sox pysoundfile librosa
❯ conda install backports.tempfile
❯ python setup.py clean --all
❯ MACOSX_DEPLOYMENT_TARGET=10.9 CC=clang CXX=clang++ NO_CUDA=1 python setup.py install

❯ python test/test_transforms.py
----------------------------------------------------------------------
Ran 23 tests in 0.845s

OK
❯ python test/test.py                   
----------------------------------------------------------------------
Ran 7 tests in 0.164s

OK

test/test.py

cpuhrsch · 2019-12-06T17:16:13Z

test/test.py

@@ -171,5 +300,21 @@ def test_5_get_info(self):
        self.assertEqual(si.rate, rate)
        self.assertEqual(ei.bits_per_sample, precision)

+        torchaudio.set_audio_backend(self.default_audio_backend)
+
+    def _test_5_get_info_soundfile(self):


I think there's repetition again?

There isn't because info returns a different struct depending on the backend.

cpuhrsch · 2019-12-06T17:16:43Z

test/test.py

+        self.assertEqual(si.channels, channels)
+        self.assertEqual(si.frames, samples)
+        self.assertEqual(si.samplerate, rate)
+        si_precision = _extract_digits(si.subtype)


If this is necessary we should at least write out as a todo to make this consistent. That can be done via a wrapper class that uses getattribute etc to align these attributes consistently.

It should make sense to standardize on sox since otherwise we'll introduce BC-breaking changes to support this new backend.

cpuhrsch · 2019-12-06T17:33:57Z

torchaudio/__init__.py

@@ -242,6 +280,11 @@ def sox_signalinfo_t():
        >>> si.precision = 16
        >>> si.length = 0
    """
+
+    if get_audio_backend() != "sox":


You could create a generic decorator to write this less often

def _backend_guard(backends): def decorator(fn): @functools.docs-decorator(fn) # not sure about the name def _fn(*args, **kwargs): if get_audio_backend() not in backends: raise Runtime("fn {} requires backend to be one of".format(fn.__name__, backends) fn(*args, **kwargs) return _fn

@backend_support(['sox']) def sox_signalinfo_t(): ....

Nice, but let's make that a separate PR.

Alright, added to standardize import error. :)

cpuhrsch · 2019-12-06T17:40:13Z

torchaudio/__init__.py

-    return save_encinfo(filepath, src, channels_first, si)
+
+    if get_audio_backend() == "sox":
+        func = _sox_backend.save


You could do

getattr(get_audio_backend_module() , 'save')

Do we want get_audio_backend() to return a module or a string that is the name?

A module can yield the name as well via introspection

cpuhrsch · 2019-12-19T04:12:40Z

test/test.py

+        with AudioBackendScope(backend2):
+            tensor2, sample_rate2 = torchaudio.load(output_path)
+
+        # tensor1 = tensor1.type(torch.FloatTensor)


nit: maybe you wanted to remove these?

Thanks for catching this!

cpuhrsch

Great! Added a small nit

tadas-subonis · 2020-04-05T15:35:36Z

Is there any reason why

    # normalize if needed
    # _audio_normalization(out, normalization)

was commented out?

vincentqb changed the title ~~Sox~~ Backend switch Nov 27, 2019

vincentqb mentioned this pull request Nov 27, 2019

torchaudio.load() with mp3 file floods the console #357

Closed

vincentqb commented Nov 27, 2019

View reviewed changes

test/test.py Show resolved Hide resolved

vincentqb commented Dec 3, 2019

View reviewed changes

cpuhrsch reviewed Dec 3, 2019

View reviewed changes

test/test.py Show resolved Hide resolved

cpuhrsch reviewed Dec 3, 2019

View reviewed changes

torchaudio/_soundfile_backend.py Outdated Show resolved Hide resolved

vincentqb mentioned this pull request Dec 4, 2019

Move import _torch_sox inside function calls #361

Closed

vincentqb force-pushed the sox branch 2 times, most recently from 6763600 to 505618c Compare December 5, 2019 15:45

vincentqb commented Dec 5, 2019

View reviewed changes

torchaudio/sox_effects.py Show resolved Hide resolved

vincentqb commented Dec 5, 2019

View reviewed changes

torchaudio/_soundfile_backend.py Outdated Show resolved Hide resolved

vincentqb marked this pull request as ready for review December 5, 2019 23:14

vincentqb requested a review from cpuhrsch December 6, 2019 00:02

cpuhrsch reviewed Dec 6, 2019

View reviewed changes

test/test.py Outdated Show resolved Hide resolved

cpuhrsch reviewed Dec 6, 2019

View reviewed changes

test/test.py Show resolved Hide resolved

cpuhrsch reviewed Dec 6, 2019

View reviewed changes

test/test.py Outdated Show resolved Hide resolved

cpuhrsch reviewed Dec 6, 2019

View reviewed changes

vincentqb added 21 commits December 18, 2019 15:11

add support for precision.

55bac5d

remove inplace out support.

41d0f8f

flake8.

823138e

explicitly convert from numpy.

ea7a827

adding test for wav file.

0a54a38

standardizing tests.

300096d

getattr to load module.

15de2fc

soundfile info follows sox.

0202cf5

error with incorrect parameter instead of silent ignore.

16179e8

correct name of test.

45d214d

normalization required for soundfile.

3285c8b

flake8.

2f5caf2

no need to change.

0d9b3d5

no need to change.

6905086

no need to change.

fa50b00

no need to change.

45b65ec

add equivalent wav file.

4a9064b

oneliner.

7aac175

adding test across backend. using float32 as done in sox.

d0690ae

backend guard decorator.

5f6494f

move to backend file, for easier import.

00066ef

vincentqb force-pushed the sox branch from d6ea29d to 00066ef Compare December 18, 2019 20:11

vincentqb requested a review from cpuhrsch December 18, 2019 20:59

cpuhrsch reviewed Dec 19, 2019

View reviewed changes

cpuhrsch approved these changes Dec 19, 2019

View reviewed changes

remove commented out line

bac898b

vincentqb merged commit 774ebc7 into pytorch:master Dec 19, 2019

This was referenced Dec 20, 2019

Update audio preprocessing tutorial pytorch/tutorials#797

Merged

Windows support #50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend switch #355

Backend switch #355

vincentqb commented Nov 26, 2019 •

edited

vincentqb Dec 3, 2019 •

edited

cpuhrsch Dec 3, 2019

vincentqb commented Dec 3, 2019

cpuhrsch Dec 3, 2019

cpuhrsch Dec 3, 2019

vincentqb Dec 5, 2019

cpuhrsch commented Dec 3, 2019

cpuhrsch Dec 3, 2019

vincentqb Dec 3, 2019

cpuhrsch Dec 3, 2019

cpuhrsch Dec 3, 2019

vincentqb Dec 4, 2019

cpuhrsch Dec 3, 2019

cpuhrsch Dec 3, 2019

cpuhrsch Dec 3, 2019

vincentqb Dec 13, 2019 •

edited

cpuhrsch Dec 17, 2019

vincentqb commented Dec 5, 2019

cpuhrsch Dec 6, 2019

cpuhrsch Dec 6, 2019

cpuhrsch Dec 6, 2019

cpuhrsch Dec 6, 2019

cpuhrsch Dec 6, 2019

vincentqb Dec 6, 2019

vincentqb Dec 17, 2019

cpuhrsch Dec 6, 2019

cpuhrsch Dec 19, 2019

vincentqb Dec 19, 2019

cpuhrsch left a comment

tadas-subonis commented Apr 5, 2020

Backend switch #355

Backend switch #355

Conversation

vincentqb commented Nov 26, 2019 • edited

vincentqb Dec 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentqb commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuhrsch commented Dec 3, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentqb Dec 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentqb commented Dec 5, 2019

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

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

cpuhrsch left a comment

Choose a reason for hiding this comment

tadas-subonis commented Apr 5, 2020

vincentqb commented Nov 26, 2019 •

edited

vincentqb Dec 3, 2019 •

edited

vincentqb Dec 13, 2019 •

edited