Skip to content

[STABLE FORCED_ALIGN ABI PORT] Remove Accessor use in forced_align #4022

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

[STABLE FORCED_ALIGN ABI PORT] Remove Accessor use in forced_align #4022

wants to merge 22 commits into from

Conversation

samanklesaria
Copy link
Collaborator

@samanklesaria samanklesaria commented Aug 5, 2025 •
edited
Loading

In preparation for migration to the stable ABI, this Pr removes Accessor use in forced_align. It does this by making its own version of an Accessor, assuming that strides are 1 and that everything is contiguous. This PR may need some additional improvements, but I leave that decision to you.

  • I made separate Accessor and MutAccessor classes which are basically the same, but differ based on whether their input is const or not. I tried to combine them both into a single templatized Accessor, but it didn't seem to work, so I went with the ugly solution. If you want, I can try harder to get the templated version to work.
  • This approach (an accessor class) could be adopted by my previous PRs that calculated the offsets explicitly (for backPtr).

@samanklesaria samanklesaria requested a review from a team as a code owner August 5, 2025 19:22
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Aug 5, 2025 •
edited
Loading

🔗 Helpful Links

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

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

❌ 38 New Failures, 2 Cancelled Jobs

As of commit 724606a with merge base 0c22347 (image):

NEW FAILURES - The following jobs have failed:

CANCELLED JOBS - The following jobs were cancelled. Please retry:

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

@meta-cla meta-cla bot added the CLA Signed label Aug 5, 2025
@samanklesaria samanklesaria marked this pull request as draft August 6, 2025 21:31
@samanklesaria
Copy link
Collaborator Author

Note: this commit now depends on the dtype and is_cpu methods of stable::Tensor in pytorch, which have yet to be merged in. I will keep this PR in draft state until they make their way to nightly.

@samanklesaria samanklesaria changed the title Remove Accessor use in forced_align [STABLE FORCED_ALIGN ABI PORT] Remove Accessor use in forced_align Aug 7, 2025
@samanklesaria
Copy link
Collaborator Author

This PR passes all tests for forced_align on CPU except expected failures. This is because the correct handling of the expected failures requires porting the amax and item ops to the stable ABI. I will port these ops next.

@MahmoudAshraf97
Copy link

Hello, is there a chance to get any features or improvements from #3787 merged? on the other side, I managed to remove the torch dependency completely from the FA api, so I guess it's doable, only the input log_probs array needs to be a torch tensor, all other arrays can be vectors or regular arrays, check https://github.com/MahmoudAshraf97/ctc-forced-aligner/blob/main/ctc_forced_aligner/forced_align_impl.cpp for reference

@NicolasHug
Copy link
Member

Hi @MahmoudAshraf97 , we're currently trying to migrate these ops to be ABI-stable, which is our current top priority. Unfortunately, we won't have the bandwidth to consider improvements like in #3787 at the moment, sorry. It's possible that we'll be able to re-consider in the future, but the near-term goal is for torchaudio to be stable while minimizing maintenance requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samanklesaria @MahmoudAshraf97 @NicolasHug