Skip to content

Let torchaudio.load() and torchaudio.save() rely on load_with_torchcodec() and save_with_torchcodec(). #4039

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

Merged
merged 30 commits into from
Aug 18, 2025

Conversation

samanklesaria
Copy link
Collaborator

This PR wraps the load_with_torchcodec and save_with_torchcodec functions with functions of the name load and save so that code that depends on the old load and save functions can continue to work in the future once we remove backend-specific code.

Copy link

pytorch-bot bot commented Aug 12, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/audio/4039

Note: Links to docs will display an error until the docs builds have been completed.

❌ 8 New Failures

As of commit 498ce49 with merge base 02351a6 (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@meta-cla meta-cla bot added the CLA Signed label Aug 12, 2025
@samanklesaria
Copy link
Collaborator Author

Installing ffmpeg>4, which is necessary for torchcodec to be able to load files used during testing, seems to be incompatible with the current CI infrastructure. Perhaps we need a separate PR to install ffmpeg>4, and wait for the infrastructure to improve so that that PR can be merged.

@samanklesaria
Copy link
Collaborator Author

samanklesaria commented Aug 14, 2025

I'm coming across an interesting discrepancy in the load behavior here. In test_load_save_torchcodec.py, the function test_save_channels_first creates a random tensor called waveform. When I try it, I get waveform[0,1345] = -4.0688. Saving and loading the result with scipy gives waveform_ta[0,1345] = -4.0688. But saving with torchcodec and loading with scipy gives waveform[0,1345] = -1.. What exactly is happening here? Are we supposed to be truncating saved values between -1 and 1? Is the normalization that torchcodec is doing different than the normalization scipy does?
I guess the docstring for save_with_torchcodec says "TorchCodec AudioEncoder expects float32 samples in [-1, 1] range". Does this mean the behavior of this test is undefined when our random waveform has larger values?

An easy fix here is just to use rand instead of randn to initialize our random tensor.

@samanklesaria samanklesaria marked this pull request as ready for review August 14, 2025 03:46
@samanklesaria samanklesaria requested a review from a team as a code owner August 14, 2025 03:46
@NicolasHug
Copy link
Member

Your assessment on test_load_save_torchcodec.py are correct, it's would have been best to ensure that input values of the encoder are in [-1, 1] rather than to use randn.

However, this entire file is meant to test the old torchaudio load() and save() against load_with_torchcodec() and save_with_torchcodec()

Now that we are moving load() and save() to rely on load_with_torchcodec() and save_with_torchcodec() directly, these tests aren't worth running anymore. Even more, we are switching torchcodec for scipy - so these tests are not testing what they were meant to test anymore.

Basically, we can just skip them safely. Just add a big pytest.skip() statement at the top, indicating this test file was only relevant back when load_with_torchcodec() and save_with_torchcodec() were introduced and load() and save() still had their own implementation.

@samanklesaria samanklesaria marked this pull request as draft August 14, 2025 14:58
@samanklesaria
Copy link
Collaborator Author

samanklesaria commented Aug 14, 2025

Now we're failing because core dumped) python -c "import torch; import torchaudio; import torchcodec; print(torch.__version__, torchaudio.__version__, torchcodec.__version__)" fails. Because I never defined __version__ in my mock module. But do I need it? It might be easiest to remove this check in the install script.

@samanklesaria
Copy link
Collaborator Author

samanklesaria commented Aug 14, 2025

I'm going to remove the installation of torchcodec during testing, as it shouldn't be used anyway. We should rely on the mock.

@samanklesaria samanklesaria marked this pull request as ready for review August 15, 2025 16:46
@NicolasHug NicolasHug changed the title Torchcodec loading Let torchaudio.load() and torchaudio.save() rely on load_with_torchcodec() and save_with_torchcodec(). Aug 18, 2025
from typing import Union, BinaryIO, Optional, Tuple
import os
import torch
import sys
Copy link
Member

Choose a reason for hiding this comment

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

sys doesn't seem to be used

@NicolasHug NicolasHug merged commit 93f582c into main Aug 18, 2025
42 of 43 checks passed
@NicolasHug NicolasHug deleted the torchcodec_loading branch August 18, 2025 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants