Skip to content

Granite speech - minor fixes to support training with the HF trainer#38833

Merged
ArthurZucker merged 6 commits into
huggingface:mainfrom
avihu111:granite_speech_updates
Jun 24, 2025
Merged

Granite speech - minor fixes to support training with the HF trainer#38833
ArthurZucker merged 6 commits into
huggingface:mainfrom
avihu111:granite_speech_updates

Conversation

@avihu111

@avihu111 avihu111 commented Jun 15, 2025

Copy link
Copy Markdown
Contributor

What does this PR do?

Minor updates to granite_speech to enable finetuning it with HF trainers.

  • avoids a crash when trainers pass padding=True to the processor
  • ensure all trainable parameters have gradients (bugfix) - remove .data from a forward call
  • rename melspec to mel_filters to leverage this, which avoids a crash on save_pretrained

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

CC: @ArthurZucker @eustlb @alex-jw-brooks @avishaiElmakies @gsaon

avihu111 added 2 commits June 15, 2025 16:50
avoid unused parameters that DDP does not like
trainers often pass this argument automatically
@avihu111 avihu111 marked this pull request as draft June 15, 2025 14:19
**kwargs,
):
super().__init__(**kwargs)
self.sampling_rate = sampling_rate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this isn't used currently

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, it's not used. I added it to stay consistent with other audio feature extractors that have this property.

@avihu111 avihu111 marked this pull request as ready for review June 16, 2025 14:22

@avihu111 avihu111 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added some comments on each change, giving relevant context

audio_inputs = {}

text_inputs = self.tokenizer(prompt_strings, padding=True, **kwargs)
if "padding" not in kwargs:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

avoids a crash when trainers pass padding=True to the processor


query_output = self.qformer(
query_embeds=self.query.data,
query_embeds=self.query,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Bugfix. When using .data this trainable parameter did not receive gradients.

# Currently lazily initialized
self.melspec = None
requires_backends(self, ["torchaudio"])
self.mel_filters = torchaudio.transforms.MelSpectrogram(**self.melspec_kwargs)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed the lazy init, and renamed it to mel_filters. This specific name avoids a crash when serializing the processor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm sorry for the delay!

@ArthurZucker ArthurZucker merged commit be10d4d into huggingface:main Jun 24, 2025
14 checks passed
@avihu111

Copy link
Copy Markdown
Contributor Author

Awesome! Thanks @ArthurZucker!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants