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

[BC Breaking] Reorganize C++ resources #630

Merged
merged 1 commit into from May 15, 2020
Merged

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented May 12, 2020

Breakdown of #625

This PR

  • Changes the location of C++ codes from
    • from torchaudio/torch_sox.cpp to torchaudio/csrc/sox.cpp
    • from torchaudio/torch_sox.h to torchaudio/csrc/sox.h
  • Changes the location where the resulting library is placed
    • from _torch_sox.so (outside of torchaudio module)
      to torchaudio/_torchaudio.so.

BTW: I noticed that torch_sox.h is not used by extension. We can remove it if we do not intend to make the extension library usable from external.

@mthrok mthrok force-pushed the libtorchaudio branch 3 times, most recently from c9f754f to 6219062 Compare May 12, 2020 15:49
@mthrok mthrok requested review from vincentqb and cpuhrsch May 12, 2020 16:01
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, but travis is failing.

ImportError: cannot import name '_torchaudio' from 'torchaudio' 

@mthrok
Copy link
Collaborator Author

mthrok commented May 13, 2020

LGTM, but travis is failing.

ImportError: cannot import name '_torchaudio' from 'torchaudio' 

Ref: #636

@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #630 into master will not change coverage.
The diff coverage is 75.86%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #630   +/-   ##
=======================================
  Coverage   88.82%   88.82%           
=======================================
  Files          21       21           
  Lines        2220     2220           
=======================================
  Hits         1972     1972           
  Misses        248      248           
Impacted Files Coverage Δ
torchaudio/__init__.py 78.63% <63.15%> (ø)
torchaudio/_sox_backend.py 93.75% <100.00%> (ø)
torchaudio/sox_effects.py 95.65% <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 9835db7...5a8b395. Read the comment docs.

@mthrok mthrok marked this pull request as draft May 14, 2020 02:46
@mthrok mthrok marked this pull request as ready for review May 14, 2020 02:47
@mthrok
Copy link
Collaborator Author

mthrok commented May 14, 2020

@vincentqb

Copy link
Contributor

@cpuhrsch cpuhrsch left a comment

Choose a reason for hiding this comment

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

Find with me, but please mark as BC breaking.

@mthrok mthrok changed the title Reorganize C++ resources [BC Breaking] Reorganize C++ resources May 15, 2020
@mthrok mthrok merged commit 44af0de into pytorch:master May 15, 2020
@mthrok mthrok deleted the libtorchaudio branch May 15, 2020 15:25
@mthrok
Copy link
Collaborator Author

mthrok commented May 15, 2020

Find with me, but please mark as BC breaking.

Done so. Thanks!

bhargavkathivarapu pushed a commit to bhargavkathivarapu/audio that referenced this pull request May 19, 2020
This PR
 - Changes the location of C++ codes from
     - from `torchaudio/torch_sox.cpp` to `torchaudio/csrc/sox.cpp`
     - from `torchaudio/torch_sox.h` to `torchaudio/csrc/sox.h`
 - Changes the location where the resulting library is placed
     - from `_torch_sox.so` (outside of `torchaudio` module)
        to `torchaudio/_torchaudio.so`.
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

3 participants