search

pytorch / hub

Submission to https://pytorch.org/hub/
1.4k stars 244 forks source link

silero usage of `torch.stft` #283

Closed vmoens closed 2 years ago

vmoens commented 2 years ago

It seems that silero uses the torch.stft function in a way that will be deprecated in the next pytorch release.

Previously, the following usage was permitted

torch.stft(tensor, num, num, num, num, bool, bool, *other)

but now a string representing the padding mode must be included

torch.stft(tensor, num, num, num, num, bool, str, bool, *other)

Link to the old function signature

Link to the new function signature

Link to the broken CI log

PR presumably responsible of the BC breaking change

Perhaps @peterbell10 can comment on this?

@snakers4 do you think it's something that can be fixed? Because the source code is not shared it's hard for us to debug this properly.

cc @NicolasHug

snakers4 commented 2 years ago

Hi,

Link to the broken CI log

Looks like this is based off a nightly PyTorch build.

Here is how we use torch.stft before converting to TorchScript

class STFT(nn.Module):
    def __init__(self):

....

    def forward(self, x):
        return torch.stft(x,
                          n_fft=self.n_fft,
                          hop_length=self.hop_length,
                          win_length=self.n_fft,
                          return_complex=True,
                          window=torch.hann_window(window_length=self.n_fft,
                                                   dtype=x.dtype, device=x.device)).abs()

but now a string representing the padding mode must be included pad_mode (string, optional) – controls the padding method used when center is True. Default: "reflect"

Is it already set in stone that this breaking change will be merged as is? Usually PyTorch does not make such changes.

The following files fail:

In any case: snakers4_silero-vad_language.py and python_code/snakers4_silero-vad_number.py can be deprecated, because no one uses them and we decided to deprecate them.

As for python_code/snakers4_silero-models_stt.py I can just rebuild and reupload the latest models if the change gets added to a release.

vmoens commented 2 years ago

@snakers4

Is it already set in stone that this breaking change will be merged as is? Usually PyTorch does not make such changes.

it seems this PR will be reverted so let's wait for a couple of days before acting on anything.

Thanks for your responsiveness!

snakers4 commented 2 years ago

ok

NicolasHug commented 2 years ago

Our CI seems to be green now: https://github.com/pytorch/hub/pull/225 (https://app.circleci.com/pipelines/github/pytorch/hub/1725/workflows/ec0914a3-1510-4dcd-b3b3-42f1eca174cd/jobs/2166)

Thanks a lot @vmoens for investigating, and @snakers4 for your timely replies!

snakers4 commented 2 years ago

Hope that this minor contribution made PyTorch a bit better)