Skip to content

Remove comparison dependencies and skipIfs #4031

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 15 commits into from
Aug 12, 2025
Merged

Conversation

samanklesaria
Copy link
Collaborator

As previous PRs have precomputed the results to various calls to external libraries, we no longer need to depend on those libraries when running the test suite. This PR removes instructions to install sox, librosa and kaldi in the linux test suite runner and removes the skipIf declarations on the associated external library comparison tests.

Copy link

pytorch-bot bot commented Aug 11, 2025

🔗 Helpful Links

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

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

❌ 9 New Failures, 1 Unrelated Failure

As of commit c1049e9 with merge base 879faa0 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

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

We're now failing because we lack sklearn. But sklearn wasn't even in our dependencies before! All I can think of is that something else (like librosa) depended on it, and we were able to import it because pip installing librosa gave us access to sklearn too.

@NicolasHug
Copy link
Member

On the sklearn issue, I hope this is the only once, but if that's the case then let's just ignore the corresponding test. We can probably just skip it and comment-out the offending import line in test/torchaudio_unittest/example/hubert/__init__.py ?

@samanklesaria
Copy link
Collaborator Author

We can't just remove the change to the python path, as that will break test/example/hubert/test_crop_audio_label.py. But the test itself doesn't use the kmeans functionality. The learn_kmeans function that uses sklearn is never tested. So as long as we hide the import inside the call to learn_kmeans, we're okay.

@samanklesaria
Copy link
Collaborator Author

samanklesaria commented Aug 11, 2025

It seems like we need to remove the backends. If they're in the repository, they'll get added to the deprecations tests. But because the dependencies are there, we'll get errors. But that's a much larger change than we probably want to include in this PR. If we want to make the CI green for this PR without removing the sox backend from the codebase, the easiest fix I see is a hack to make the deprecation tests ignore anything from a sox module.

@samanklesaria
Copy link
Collaborator Author

samanklesaria commented Aug 11, 2025

It also seems like the kaldi sliding window test expected results we're properly added when we merged. This is probably because in the upstream commit (the librosa tests) I changed the naming convention to remove cpu/cuda naming differences. And the kaldi files were't regenerated after the change.

@samanklesaria samanklesaria marked this pull request as ready for review August 11, 2025 22:38
@samanklesaria samanklesaria requested review from nateanl and a team as code owners August 11, 2025 22:38
@samanklesaria samanklesaria marked this pull request as draft August 12, 2025 16:07
@samanklesaria samanklesaria marked this pull request as ready for review August 12, 2025 17:11
@samanklesaria
Copy link
Collaborator Author

samanklesaria commented Aug 12, 2025

Verbally approved by Nicholas on slack. Merging.

@samanklesaria samanklesaria merged commit bc3de07 into main Aug 12, 2025
40 of 43 checks passed
@samanklesaria samanklesaria deleted the remove_test_deps branch August 12, 2025 19:07
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