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

Add sox_effects submodule and delegate sox_effects initialization #708

Merged
merged 1 commit into from Jun 10, 2020

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Jun 9, 2020

No description provided.

@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #708 into master will decrease coverage by 0.05%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
- Coverage   89.29%   89.23%   -0.06%     
==========================================
  Files          25       26       +1     
  Lines        2344     2350       +6     
==========================================
+ Hits         2093     2097       +4     
- Misses        251      253       +2     
Impacted Files Coverage Δ
torchaudio/backend/utils.py 86.66% <84.61%> (-5.34%) ⬇️
torchaudio/__init__.py 89.47% <100.00%> (-1.84%) ⬇️
torchaudio/backend/__init__.py 100.00% <100.00%> (ø)
torchaudio/sox_effects/__init__.py 100.00% <100.00%> (ø)
torchaudio/sox_effects/sox_effects.py 87.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b788db3...c7eb0e9. Read the comment docs.

@mthrok mthrok force-pushed the sox-effect branch 3 times, most recently from 34f3380 to c7eb0e9 Compare June 9, 2020 15:59
@mthrok mthrok marked this pull request as ready for review June 9, 2020 18:57
@mthrok mthrok requested a review from vincentqb June 9, 2020 18:57
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

Can you detail what you are trying to do here?

return code
return _SOX_SUCCESS_CODE


@_mod_utils.requires_module("torchaudio._torchaudio")
def shutdown_sox() -> int:
def shutdown_sox_effects() -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

that's BC breaking, no? torchaudio.sox_effects.initialize_sox()

Copy link
Contributor

@vincentqb vincentqb Jun 10, 2020

Choose a reason for hiding this comment

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

I saw that torchaudio.shutdown_sox() still works as you re-import correctly above. However, users could have been doing torchaudio.sox_effects.shutdown_sox() which would now be torchaudio.sox_effects.shutdown_sox_effects(), right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah if the user adopts torchaudio.sox_effects.shutdown_sox() which introduced only a couple of days ago.
I do not see that a problem but if you insist, I can simply add [BC braking] tag to commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, since it wasn't part of a release, we don't need to worry about this then.

@@ -11,7 +10,7 @@
)

if _mod_utils.is_module_available('torchaudio._torchaudio'):
from . import _torchaudio
from torchaudio import _torchaudio
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: that's a separate issue :)

@mthrok
Copy link
Collaborator Author

mthrok commented Jun 9, 2020

Can you detail what you are trying to do here?

There are couple of aspects of this PR that overall improves the maintainability of the code base, based on "decoupling" and "separation of concerns".

As you know, sox_effects functionalities are either available or unavailable. From the viewpoint of torchaudio main module, the looser the connection between the torchaudio module and torchaudio.sox_effects, the more manageable the code base become because you can change the two modules independently. This is mostly accomplished when the definitions of initialize_sox and shutdown_sox were moved from torchaudio.__init__ to torchaudio.sox_effects, but the initialization of sox effects are still happening in torchaudio.__init__. If we move the initialization to sox_effects module, the responsibility of sox initialization is moved to sox_effects module, along with the required module availability check etc. The main torchaudio module can be carefree about how the sox_effects module should work.

In addition to that, I found that initialize_sox and shutdown_sox are confusing because it sound like they are required for libsox based I/O. To make it clear, I renamed them to include sox_effect in function name.

Also moving functions from the original places are BC breaking itself, therefore, these functions are re-imported in torchaudio.__init__ and renamed to match the original names. Therefore the PR is not BC breaking.

In the next PR 9519310, I am gonna mark the initialize_sox and shutdown_sox deprecated, so that the decoupling is completed.

@vincentqb
Copy link
Contributor

Thanks for clarifying. I just wanted to make sure we are on the same page :)

@mthrok
Copy link
Collaborator Author

mthrok commented Jun 10, 2020

Thanks for clarifying. I just wanted to make sure we are on the same page :)

Great, let me know if you need something for approval.

Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

LGTM

@mthrok mthrok merged commit c82a7f9 into pytorch:master Jun 10, 2020
@mthrok mthrok deleted the sox-effect branch June 10, 2020 15:39
@mthrok
Copy link
Collaborator Author

mthrok commented Jun 10, 2020

Thanks!

mthrok pushed a commit to mthrok/audio that referenced this pull request Feb 26, 2021
Make jit tutorial consistently using 4 spaces as indent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants