Inconsistent default and TS-incompatible lazy behavior in MelScale

[mthrok]

1. All the spectrogram related transforms have default frequency bin n_fft: int = 400, while MelScale and InverseMelScale is n_stft: Optional[int] = None.

2. In MelScale, when n_stft=None, it tries to resize the buffer in forward, but this causes TorchScripted (and loaded from file) version fail.

audio/torchaudio/transforms.py

Lines 287 to 308 in 931555c

	specgram (Tensor): A spectrogram STFT of dimension (..., freq, time).
	Returns:
	Tensor: Mel frequency spectrogram of size (..., ``n_mels``, time).
	"""
	# pack batch
	shape = specgram.size()
	specgram = specgram.reshape(-1, shape[-2], shape[-1])
	if self.fb.numel() == 0:
	tmp_fb = F.create_fb_matrix(specgram.size(1), self.f_min, self.f_max,
	self.n_mels, self.sample_rate, self.norm,
	self.mel_scale)
	# Attributes cannot be reassigned outside __init__ so workaround
	self.fb.resize_(tmp_fb.size())
	self.fb.copy_(tmp_fb)
	# (channel, frequency, time).transpose(...) dot (frequency, n_mels)
	# -> (channel, time, n_mels).transpose(...)
	mel_specgram = torch.matmul(specgram.transpose(1, 2), self.fb).transpose(1, 2)

https://app.circleci.com/pipelines/github/pytorch/audio/5716/workflows/fe399658-a33f-47f2-8227-7750b2f0af2f/jobs/197223/tests#failed-test-0

>       return callable(*args, **kwargs)
E       RuntimeError: The following operation failed in the TorchScript interpreter.
E       Traceback of TorchScript, serialized code (most recent call last):
E         File "code/__torch__/torchaudio/transforms.py", line 20, in forward
E           if torch.eq(torch.numel(self.fb), 0):
E             tmp_fb = _0(torch.size(specgram0, 1), 0., 8000., 128, 16000, self.norm, self.mel_scale, )
E             _1 = torch.resize_(self.fb, torch.size(tmp_fb), memory_format=None)
E                  ~~~~~~~~~~~~~ <--- HERE
E             _2 = torch.copy_(self.fb, tmp_fb, False)
E           else:
E       
E       Traceback of TorchScript, original code (most recent call last):
E         File "/root/project/env/lib/python3.9/site-packages/torchaudio-0.9.0a0+bb886e7-py3.9-linux-x86_64.egg/torchaudio/transforms.py", line 302, in forward
E                                               self.mel_scale)
E                   # Attributes cannot be reassigned outside __init__ so workaround
E                   self.fb.resize_(tmp_fb.size())
E                   ~~~~~~~~~~~~~~~ <--- HERE
E                   self.fb.copy_(tmp_fb)
E           
E       RuntimeError: Trying to resize storage that is not resizable at /opt/conda/conda-bld/pytorch_1617951974812/work/aten/src/TH/THStorageFunctions.cpp:87

To reproduce,

1. construct MelScale with n_stft=None.
2. Script the transform and save on file
3. Load the transform from file and feed a spectrogram Tensor.

Once the transform is scripted and dumped, there is no way to fix this issue.
The library code should not be hacking around, which can generate such a stack state.

For fix, since all the n_fft defaults to 400, n_stft should default to 201 as well.
This will remove the need of the above resize_ hack.

[cpuhrsch]

Here are some thoughts on this from our offline discussion.

At a minimum we should use __prepare_scriptable__ to throw an error when scripting when n_stft=None to make it easier to detect this issue.

From a user convenience vs correctness / consistency perspective users could mistakenly pass a waveform instead of a spectrogram with this shape inference, if n_stft isn't fixed. Then lazy evaluation causes a delayed error when tensors with wrong shape are passed. (shape mismatch will happen further down the pipeline, which obfuscates the real error).

I suggest we add a warning when a user doesn't specify an explicit value for this release and then deprecate this behavior in the next release.

cc @vincentqb @astaff

[vincentqb]

Is anyone using shape inference? Is it a design principle that we would even like to follow? e.g. internal. I remember @ezyang was looking into shape inference. Maybe he wants to comment here :)

From a user convenience vs correctness / consistency perspective users could mistakenly pass a waveform instead of a spectrogram with this shape inference, if n_stft isn't fixed. Then lazy evaluation causes a delayed error when tensors with wrong shape are passed. (shape mismatch will happen further down the pipeline, which obfuscates the real error).

Did someone complain about identifiability? If the goal is correctness and identifiability: are there better solutions, say like simply changing the default?

I suggest we add a warning when a user doesn't specify an explicit value for this release and then deprecate this behavior in the next release.

The options I see are

1. deprecating laziness in MelScale
2. extending laziness to other functional
3. disabling laziness when torchscripting
4. leaving things as is (and MelScale is not torchscriptable)

Unless we are explicitly interested in extending lazy inference (option 2), I agree with re-aligning MelScale with the others, and deprecating that functionality as mentioned (option 1). Let's of course make sure the documentation and the error message make it very clear what the value of the inferred parameter should be.

[cpuhrsch]

My main argument against proliferation this kind of laziness is that it's not very principled. For this I'd rather see efforts such as LazyTensors pan out. It's not our place to invent those in peripheral libraries, but we should be involved in motivating those (similar to complex numbers). This is then also an argument against keeping this around. Why make a one-off exception?

[vincentqb]

My main argument against proliferation this kind of laziness is that it's not very principled. For this I'd rather see efforts such as LazyTensors pan out. It's not our place to invent those in peripheral libraries, but we should be involved in motivating those (similar to complex numbers). This is then also an argument against keeping this around. Why make a one-off exception?

Yup, I agree. The place for such initiative is not torchaudio.

[ezyang]

This doesn't have anything to do with lazy tensors, it's lazy modules. You want to resize fb based on the forward argument. There's precedent for this in core PyTorch via lazy modules: pytorch/pytorch#44538 You should apply the pattern employed there to Mel and you won't have a problem anymore.

cc @emcastillo @albanD

[cpuhrsch]

Until we have a more principles approach to lazy behavior, let's go with Option 1 of Vincent's comment:
deprecating laziness in MelScale