Use environment variable for switching SoX dep #669

mthrok · 2020-05-29T19:40:04Z

This PR changes

The way how to decide if codecs/SoX should be built when building torchaudio C++ extension.
It check whether BUILD_SOX env is defined or not. (Originally, it was checking the existence of .use_external_sox file.)
Revert the default behavior of codecs/SoX build.
It does not build codecs/SoX by default.

mthrok · 2020-05-29T20:30:43Z

README.md

+This is known to work on a few major Linux distributions such as Ubuntu and CentOS 7 and macOS.
+If you try this on a new system and found a solution to make it work, feel free to share it by opening and issue.
+
+#### Troubleshooting:


We can put this section (or the whole installation direction) into a separate file or Wiki too.

The installation section should stay in the readme. I do like the folds you are adding, which avoids cluttering part of the wiki, at least when rendered such as on github. I'd say to leave troubleshooting here, with the installation steps, but I'm ok either for this.

Since we may leave some pull requests opened for troubleshooting, the troubleshooting section may be redundant, and more work to keep up-to-date. That would be a vote for just using issues for troubleshooting.

vincentqb

Added minor comments, but otherwise LGTM.

vincentqb · 2020-05-29T21:10:25Z

build_tools/setup_helpers/extension.py

-# Temporary fix for building in fbcode
-# at the moment, we have to use external sox in fbcode
-_BUILD_DEPS = not (_ROOT_DIR / '.use_external_sox').exists()
+_BUILD_SOX = 'BUILD_SOX' in os.environ


I have seen more often a check for whether BUILD_SOX=1 instead of just the existence. I could imagine someone doing BUILD_SOX=0 and be surprise that sox is built.

So, for this kind of env var, there is no standard of what values are acceptable.
there are really a lot of variations like 0, 1, true, false, on, off, yes, no etc...
and since this is env var, we should not really raise an error if the value is something unexpected.
I am open in defining values for our case, but my personal preference for binary behavior is just define variable or not.

I changed the behavior to expect 0/1 (and some others)

README.md

vincentqb · 2020-05-29T21:18:40Z

README.md

+This is known to work on a few major Linux distributions such as Ubuntu and CentOS 7 and macOS.
+If you try this on a new system and found a solution to make it work, feel free to share it by opening and issue.
+
+#### Troubleshooting:


The installation section should stay in the readme. I do like the folds you are adding, which avoids cluttering part of the wiki, at least when rendered such as on github. I'd say to leave troubleshooting here, with the installation steps, but I'm ok either for this.

Since we may leave some pull requests opened for troubleshooting, the troubleshooting section may be redundant, and more work to keep up-to-date. That would be a vote for just using issues for troubleshooting.

README.md

vincentqb · 2020-05-29T21:23:29Z

More downloads mean more instances of files that can't be downloaded when running circleci :)

vincentqb · 2020-05-29T21:59:41Z

Closes #666?

mthrok

More downloads mean more instances of files that can't be downloaded when running circleci :)

I do not get what you mean. Can you elaborate ?

mthrok · 2020-05-29T22:38:00Z

build_tools/setup_helpers/extension.py

-# Temporary fix for building in fbcode
-# at the moment, we have to use external sox in fbcode
-_BUILD_DEPS = not (_ROOT_DIR / '.use_external_sox').exists()
+_BUILD_SOX = 'BUILD_SOX' in os.environ


So, for this kind of env var, there is no standard of what values are acceptable.
there are really a lot of variations like 0, 1, true, false, on, off, yes, no etc...
and since this is env var, we should not really raise an error if the value is something unexpected.
I am open in defining values for our case, but my personal preference for binary behavior is just define variable or not.

codecov · 2020-05-30T02:11:50Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #669   +/-   ##
=======================================
  Coverage   88.80%   88.80%           
=======================================
  Files          21       21           
  Lines        2225     2225           
=======================================
  Hits         1976     1976           
  Misses        249      249

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 17bc15f...28eb81b. Read the comment docs.

vincentqb · 2020-06-01T14:07:16Z

More downloads mean more instances of files that can't be downloaded when running circleci :)

I do not get what you mean. Can you elaborate ?

This was not meant as an actionable statement. :) I was just noting that if more files are downloaded when setting up circle ci, we may get more timeouts/etc.

mthrok · 2020-06-01T16:45:27Z

thanks!

mthrok added 2 commits May 29, 2020 20:12

Use env var for switching SoX build and default to not

3d93a43

Update README

aa5e6c8

mthrok force-pushed the external-sox branch from 4d71567 to aa5e6c8 Compare May 29, 2020 20:13

mthrok added 2 commits May 29, 2020 20:15

Fix packaging/CI script

587bde3

Add further trouble shooting

337ccb4

mthrok commented May 29, 2020

View reviewed changes

mthrok marked this pull request as ready for review May 29, 2020 20:36

mthrok requested a review from vincentqb May 29, 2020 20:36

mthrok mentioned this pull request May 29, 2020

torchaudio Installation issue #657

Closed

vincentqb approved these changes May 29, 2020

View reviewed changes

mthrok commented May 29, 2020

View reviewed changes

Address review feedback

d6f8d16

Change env var eval

8ed8498

Update warning

28eb81b

jimchen90 mentioned this pull request Jun 1, 2020

Torchaudio Compilation Error #674

Closed

mthrok merged commit 449b6ab into pytorch:master Jun 1, 2020

mthrok deleted the external-sox branch June 1, 2020 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use environment variable for switching SoX dep #669

Use environment variable for switching SoX dep #669

mthrok commented May 29, 2020 •

edited

mthrok May 29, 2020 •

edited

vincentqb May 29, 2020

vincentqb left a comment

vincentqb May 29, 2020

mthrok May 29, 2020

mthrok Jun 1, 2020

vincentqb May 29, 2020

vincentqb commented May 29, 2020

vincentqb commented May 29, 2020

mthrok left a comment •

edited

mthrok May 29, 2020

codecov bot commented May 30, 2020 •

edited

vincentqb commented Jun 1, 2020

mthrok commented Jun 1, 2020

Use environment variable for switching SoX dep #669

Use environment variable for switching SoX dep #669

Conversation

mthrok commented May 29, 2020 • edited

mthrok May 29, 2020 • edited

Choose a reason for hiding this comment

vincentqb May 29, 2020

Choose a reason for hiding this comment

vincentqb left a comment

Choose a reason for hiding this comment

vincentqb May 29, 2020

Choose a reason for hiding this comment

mthrok May 29, 2020

Choose a reason for hiding this comment

mthrok Jun 1, 2020

Choose a reason for hiding this comment

vincentqb May 29, 2020

Choose a reason for hiding this comment

vincentqb commented May 29, 2020

vincentqb commented May 29, 2020

mthrok left a comment • edited

Choose a reason for hiding this comment

mthrok May 29, 2020

Choose a reason for hiding this comment

codecov bot commented May 30, 2020 • edited

Codecov Report

vincentqb commented Jun 1, 2020

mthrok commented Jun 1, 2020

mthrok commented May 29, 2020 •

edited

mthrok May 29, 2020 •

edited

mthrok left a comment •

edited

codecov bot commented May 30, 2020 •

edited