-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Backport patch for torchaudio, cleanups #27
base: main
Are you sure you want to change the base?
Conversation
…nda-forge-pinning 2022.07.25.18.25.33
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
> ⚠️ the upstream has a fix https://sourceforge.net/p/sox/code/ci/bb38934e11035c8fab141f70dabda3afdd17da36/, | ||
> but this fix seems to make `is_seekable` return `true` for the in-memory file | ||
> object case (need to verify this), which is the opposite of the behavior we | ||
> want in torchaudio implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the issue for torchaudio with seekable in-memory file objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mmcauliffe, thanks for the quick response!
What's the issue for torchaudio with seekable in-memory file objects?
We'd have to ask @mthrok who did the debugging/digging/fixing in pytorch/audio#1297 ff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's described in the upstream patch, the original implementation fstat(fileno((FILE*)ft->fp), &st)
did not check the return value of fileno
. and the fstat
is failing but ignored, as a result st.st_mode
attribute is not initialized and has random value. So it caused inconsistent behavior.
I think the patch from the upstream fixes this properly and elegantly, while my patch makes the code return the opposite result, which makes it impossible for torchaudio to adopt the upstream libsox codebase.
So I do not recommend applying my patch. I plan to get rid of this in torchaudio by migrating the in-memory decoding/encoding capability to FFmpeg.
I do not know the policy of condo-forge about BC-breaking, but once you apply my patch, you will have technical debt hard to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the response!
I think the patch from the upstream fixes this properly and elegantly, while my patch makes the code return the opposite result, which makes it impossible for torchaudio to adopt the upstream libsox codebase.
I don't understand the need for divergence (if their solution is elegant, why couldn't it work for torchaudio? If it doesn't work for torchaudio, how can it be fixed properly?)
I do not know the policy of conda-forge about BC-breaking, but once you apply my patch, you will have technical debt hard to fix.
Not sure what you mean with BC, maybe binary compatibility? We shouldn't break except where upstream does so as well (e.g. major releases). Thanks for the heads up. I might have to bite the bullet and take the vendored sox for torchaudio then until you get rid of it (IIUC)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the need for divergence (if their solution is elegant, why couldn't it work for torchaudio? If it doesn't work for torchaudio, how can it be fixed properly?)
I debugged the issue myself and created the patch tailored for torchaudio's need, and after a while I realized that the upstream had fix which would not work for torchaudio.
Not sure what you mean with BC
Applying the patch from upstream will fix the issue of is_seekable()
returning random value, and it will return false
for in-memory audio data. If you apply my patch, then is_seekable()
will return true for in-memory audio data. If you apply my patch, and when torchaudio gets rid of my patch, libsox from condo-forge will be the only one with the custom behavior, which I think is something you want to get rid of at some point. Then getting rid of my patch incurs backward-incompatible change, which will disturb user experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, so far we don't have torchaudio at all, so there's no user experience to break yet.
And IIUC, you're going to keep the torchaudio-specific behaviour also when you switch to ffmpeg as a backend?
However, the sox builds in conda-forge (with an "a" 😉) are not just used by torchaudio, so we have to keep that in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaves three options:
- do nothing --> users stay exposed to the hard to debug behaviour
- use the torchaudio-fix --> should be an improvement, but they might get used to the "wrong" behaviour (assuming sox ever does another release, last one was 7.5 years ago...)
- use the upstream fix --> incompatible with torchaudio, but compatible with possible future sox releases (also means we have to vendor lots of stuff into torchaudio, which is also unappealing).
They all have their pros & cons TBH. Not sure if anyone else from @conda-forge/sox has an opinion on this?
Ok, yeah, not a huge deal for the difference between upstream and the patch here, mostly just curious. I don't think it would impact any of my use cases, since MFA just uses file objects on disk. But yeah, looks good to me! |
CC @mmcauliffe, since I saw the other patch was from you - would appreciate your input on this.