Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove skipIfNoSoxBackend #1390

Merged
merged 12 commits into from Mar 29, 2021
Merged

Remove skipIfNoSoxBackend #1390

merged 12 commits into from Mar 29, 2021

Conversation

krishnakalyan3
Copy link
Contributor

Fixes #1388

@krishnakalyan3
Copy link
Contributor Author

Hello @mthrok,

I am not sure why unit tests fail with this error.E NameError: name 'torchaudio' is not defined

mthrok
mthrok reviewed Mar 14, 2021
View reviewed changes
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @krishnakalyan3

Thanks for working on this. The direction of the change looks good.

test/torchaudio_unittest/common_utils/case_utils.py Outdated
skipIfNoSox = unittest.skipIf(not is_sox_available(), reason='Sox not available')
skipIfNoSox = unittest.skipIf(
not is_sox_available() or
'sox' not in torchaudio.list_audio_backends(), reason='Sox not available')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to update the logic. 'sox' has been removed from the list of backends so 'sox' not in torchaudio.list_audio_backends() is always False.

test/torchaudio_unittest/common_utils/case_utils.py
@@ -7,7 +7,6 @@

import torch
from torch.testing._internal.common_utils import TestCase as PytorchTestCase
import torchaudio
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is failing simply because this import statement is being removed. This should stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my bad. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Somehow the style check fails for this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see. Since now we remove the 'sox' not in torchaudio.list_audio_backends(), torchaudio is no longer used. Yes, this can be finally removed.

mthrok
mthrok reviewed Mar 14, 2021
View reviewed changes
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are some tests which depended on the old sox backend, which should not be.

Can you

  1. Remove the following backend='sox'

    audio/test/torchaudio_unittest/compliance_kaldi_test.py

    Line 50 in f2b7542

    backend = 'sox'
  2. Replace the following torchaudio.load with common_utils.load_wav?

    audio/test/torchaudio_unittest/compliance_kaldi_test.py

    Line 116 in f2b7542

    sound, sample_rate = torchaudio.load(self.test_filepath, normalization=False)

    like in

    audio/test/torchaudio_unittest/compliance_kaldi_test.py

    Line 147 in f2b7542

    sound, sr = common_utils.load_wav(sound_filepath, normalize=False)

test/torchaudio_unittest/common_utils/case_utils.py
@@ -7,7 +7,6 @@

import torch
from torch.testing._internal.common_utils import TestCase as PytorchTestCase
import torchaudio
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see. Since now we remove the 'sox' not in torchaudio.list_audio_backends(), torchaudio is no longer used. Yes, this can be finally removed.

mthrok
mthrok reviewed Mar 15, 2021
View reviewed changes
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @krishnakalyan3

Thanks for working on this. I merged #1387, part of which also touches the same file as this PR, so can you resolve the conflict? That will resolve the test failures with resample_waveform.

For other test failures, can you copy-paste this method from funtional to transform class?

test/torchaudio_unittest/compliance_kaldi_test.py Outdated Show resolved Hide resolved
mthrok
mthrok approved these changes Mar 29, 2021
View reviewed changes
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@mthrok mthrok changed the title remove skipIfNoSoxBackend Remove skipIfNoSoxBackend Mar 29, 2021
@mthrok mthrok merged commit 512c2fa into pytorch:master Mar 29, 2021
@krishnakalyan3 krishnakalyan3 deleted the legacy_test branch April 1, 2021 21:46
@krishnakalyan3
Copy link
Contributor Author

Thanks for the review

mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
@brianjo
Update prototype_index.rst (pytorch#1390)
b56094c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mthrok mthrok mthrok approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Removing skipIfNoSoxBackend in test
3 participants
@krishnakalyan3 @mthrok @facebook-github-bot