Self-contain codecs library by mthrok · Pull Request #625 · pytorch/audio

mthrok · 2020-05-08T22:00:43Z

This PR

always include codecs to _torchaudio.so so that it does not depend on external SoX.
adds python setup.py clean command which facilitates clean build.

These changes follow the identical pattern as pytorch/text#755 .

Note on build time requirements
- Before this change, it was assumed that SoX was built somewhere in the System. (could be one that was installed by package management system like apt, or manually built one)
- After this change, curl and CMake are required, so that it can build SoX and codecs.

vincentqb

Tests are failing.

vincentqb · 2020-05-11T20:52:00Z

change the library name to _torchaudio.so under torchaudio directory

Can that be BC breaking?

mthrok · 2020-05-12T03:12:05Z

change the library name to _torchaudio.so under torchaudio directory

Can that be BC breaking?

I do not think so.

Python naming convention tells that _torch_sox.so is not intended for external use.
We do not have any header file, so it is not possible to link against _torch_sox.so.

Added the list of things changed by PR in the description.

cpuhrsch · 2020-05-12T04:29:48Z

I think we should consider breaking this into a few more PRs. Changes like the move of the files or the rename of the library could be separated from the build scripts, etc. and make for a more manageable / revertable changeset. They will also be quicker to merge.

mthrok · 2020-05-12T12:44:00Z

Sounds good. Will work on. Moto 🛵

…

On Tue, May 12, 2020, 00:30 cpuhrsch ***@***.***> wrote: I think we should consider breaking this into a few more PRs. Changes like the move of the files or the rename of the library could be separated from the build scripts, etc. and make for a more manageable / revertable changeset. They will also be quicker to merge. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#625 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGQ6CQYOQJCQTMRW7VQKC3RRDGETANCNFSM4M4PWWEA> .

codecov · 2020-05-14T02:51:45Z

Codecov Report

Merging #625 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #625   +/-   ##
=======================================
  Coverage   88.84%   88.84%           
=======================================
  Files          21       21           
  Lines        2223     2223           
=======================================
  Hits         1975     1975           
  Misses        248      248

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 241ab1e...0f86469. Read the comment docs.

vincentqb · 2020-05-18T19:45:58Z

offline discussion with @mthrok -- how about compilation on the internal systems? e.g. internal

vincentqb · 2020-05-19T14:46:25Z

cc @seemethere for awareness

mthrok · 2020-05-21T20:34:24Z

offline discussion with @mthrok -- how about compilation on the internal systems? e.g. internal

I added bypass for the internal. In internal, nothing about dependencies changes. This PR cannot address T53444370.

vincentqb

LGTM

vincentqb · 2020-05-27T19:11:08Z

build_tools/setup_helpers/extension.py

+def _get_extra_objects():
+    objs = []
+    if _BUILD_DEPS:
+        # NOTE: The order of the library listed bellow matters.


nit: "below" :)

vincentqb · 2020-05-27T19:14:43Z

.circleci/config.yml

      - restore_cache:

          keys:
-            - env-v1-linux-{{ arch }}-py<< parameters.python_version >>-{{ checksum ".circleci/unittest/linux/scripts/environment.yml" }}-{{ checksum ".circleci-weekly" }}


nit: v1 will be deleted soon after merging this PR?

Caches on CCI will not be deleted, and we do not have control over them.
We change cache key on weekly basis, but this PR also changes what is cached, so it's safer to change the cache key from the previous one.
That way existing PR can still use the old style cache without rebasing, as long as there is no merge conflict.

mthrok requested review from cpuhrsch and vincentqb May 8, 2020 22:00

mthrok force-pushed the csrc branch 2 times, most recently from eeebf6c to 670994e Compare May 9, 2020 16:42

vincentqb suggested changes May 11, 2020

View reviewed changes

mthrok mentioned this pull request May 12, 2020

[BC Breaking] Reorganize C++ resources #630

Merged

mthrok marked this pull request as draft May 12, 2020 15:33

mthrok force-pushed the csrc branch 4 times, most recently from be1c032 to ac09a12 Compare May 14, 2020 02:19

mthrok changed the title ~~Clean up extension build mechanism and extension location~~ Self-contain codecs library May 14, 2020

mthrok force-pushed the csrc branch from ac09a12 to d83d157 Compare May 14, 2020 02:41

mthrok force-pushed the csrc branch from d83d157 to 3f21749 Compare May 15, 2020 16:27

mthrok marked this pull request as ready for review May 15, 2020 18:56

mthrok requested a review from vincentqb May 15, 2020 18:57

mthrok force-pushed the csrc branch from 3f21749 to d58f04f Compare May 16, 2020 02:24

mthrok mentioned this pull request May 17, 2020

Test building with Sox on Windows #648

Closed

mthrok force-pushed the csrc branch from d3205bb to b926656 Compare May 21, 2020 20:29

mthrok mentioned this pull request May 25, 2020

torchaudio Installation issue #657

Closed

Clean up extension build mechanism and extension location

2708ee8

mthrok force-pushed the csrc branch from c47aa02 to 2708ee8 Compare May 27, 2020 02:01

mthrok mentioned this pull request May 27, 2020

WIP: Make get_info torchscript-able #662

Closed

mthrok added 3 commits May 27, 2020 16:42

Add back the switch to depend on external sox

3270b34

Remove print

26ffa4f

Fix

0f86469

vincentqb approved these changes May 27, 2020

View reviewed changes

vincentqb merged commit d3c83ea into pytorch:master May 27, 2020

mthrok deleted the csrc branch May 27, 2020 19:57

mthrok mentioned this pull request Jun 4, 2020

Python 2 Deprecated #477

Closed

8 tasks

mpc001 pushed a commit to mpc001/audio that referenced this pull request Aug 4, 2023

Update model.py (pytorch#625)

632d385

Conversation

mthrok commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentqb left a comment

Choose a reason for hiding this comment

Uh oh!

vincentqb commented May 11, 2020

Uh oh!

mthrok commented May 12, 2020

Uh oh!

cpuhrsch commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mthrok commented May 12, 2020 via email

Uh oh!

codecov bot commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vincentqb commented May 18, 2020

Uh oh!

vincentqb commented May 19, 2020

Uh oh!

mthrok commented May 21, 2020

Uh oh!

vincentqb left a comment

Choose a reason for hiding this comment

Uh oh!

vincentqb May 27, 2020

Choose a reason for hiding this comment

Uh oh!

vincentqb May 27, 2020

Choose a reason for hiding this comment

Uh oh!

mthrok May 27, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mthrok commented May 8, 2020 •

edited

Loading

cpuhrsch commented May 12, 2020 •

edited

Loading

codecov bot commented May 14, 2020 •

edited

Loading