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.

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

Make sox selective #1338

Merged
merged 6 commits into from Mar 2, 2021
Merged

Make sox selective #1338

merged 6 commits into from Mar 2, 2021

Conversation

carolineechen
Copy link
Contributor

support functionality to exclude libsox from C++ extension, so that we can eventually support C++ extension on Windows

  • change behavior of BUILD_SOX=0 to exclude libsox from being built + bound, different from the current implementation (dynamic build if =0)
  • construct functions to detect whether or not libsox is built, to appropriately handle functions and unittests requiring sox. previously, this was tested using detection of C++ extension since libsox and extension were always built together, but this will no longer be the case

testing: pytest torchaudio_unittest for both BUILDSOX=0 and BUILDSOX=1 cases

@mthrok mthrok self-requested a review March 2, 2021 18:44
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!

torchaudio/csrc/pybind.cpp Show resolved Hide resolved
torchaudio/csrc/sox/utils.h Outdated Show resolved Hide resolved
torchaudio/utils/sox_utils.py Outdated Show resolved Hide resolved
@@ -10,6 +10,7 @@
from .common import AudioMetaData


@_mod_utils.requires_sox()
@_mod_utils.requires_module('torchaudio._torchaudio')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the logic of requires_sox contains that of requires_module('torchaudio._torchaudio'), so we should be able to replace 'torchaudio._torchaudio'.

@@ -0,0 +1,17 @@
#include <torch/script.h>

namespace {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add torchaudio namespace too?

@@ -1,4 +1,4 @@
from torchaudio._internal import module_utils as _mod_utils
from torchaudio._internal.module_utils import is_sox_available
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason why we adopted the import pattern of from torchaudio._internal import module_utils as _mod_utils is that the whole module (and functions inside of it) is intended for internal use, so we do not want to expose it in other module.

from torchaudio._internal.module_utils import is_sox_available makes the helper function accessible like torchaudio.XXX.is_sox_available. Of course this is not a documented function and it should not be used by users, but we want to be careful about it. This is because at the beginning of torchaudio project, many helper functions were defined in __init__.py without under score prefix and it was messy/confusing. If we import individual functions from internal module, users cannot tell from the outside of the package that the function resides in the internal module, thus not for users to use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course, you can do from torchaudio._internal.module_utils import is_sox_available as _is_sox_available too.

@mthrok mthrok merged commit ecfed4d into pytorch:master Mar 2, 2021
mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
* Update spaCy model download to full name
Short 'en' and 'de' names has been deprecated since spaCy 3.0, so use fullname instead as suggested by the warning:
```
[!] As of spaCy v3.0, shortcuts like 'en' are deprecated. Pleaseuse the full
pipeline package name 'en_core_web_sm' instead.
[!] As of spaCy v3.0, shortcuts like 'de' are deprecated. Pleaseuse the full
pipeline package name 'de_core_news_sm' instead.
```

* Add retries to window build downloads

* Also, pin spacy version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants