Skip to content
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

fix Parameter dtype in audio models #30310

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

ylacombe
Copy link
Contributor

@ylacombe ylacombe commented Apr 18, 2024

What does this PR do?

tests/pipelines/test_pipelines_automatic_speech_recognition.py::AutomaticSpeechRecognitionPipelineTests::test_wav2vec2_conformer_float16 was failing because masked_spec_embed is initialized as a fp32 tensor.

self.masked_spec_embed = nn.Parameter(torch.FloatTensor(config.hidden_size).uniform_())

I thus changed the tensor from FloatTensor to Tensor. Ideally, I would have moved the .uniform_, to the _init_weights, but turns that model.apply doesn't iterate on Parameter.

Slow tests on every modeling tests of the files I've changed all passed, but I might have missed some other tests to run, any ideas @ydshieh ?

cc @ydshieh because of the failing test and @ArthurZucker because we've talked about it

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

LGTM

@ydshieh
Copy link
Collaborator

ydshieh commented Apr 18, 2024

Let me check CI

@ydshieh
Copy link
Collaborator

ydshieh commented Apr 18, 2024

The original failed pipeline test also passes. I will run a whole pipeline test and let's see. For model testing, since you checked every model modified, I also think it's fine and don't see what else should be checked manually.

Copy link
Contributor

@sanchit-gandhi sanchit-gandhi left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @ylacombe! Fine for me to keep the .uniform() in the init for simplicity as well

@ydshieh
Copy link
Collaborator

ydshieh commented Apr 18, 2024

all good! merge now, thanks a lot.

@ydshieh ydshieh merged commit 68be1d3 into huggingface:main Apr 18, 2024
19 checks passed
ydshieh pushed a commit that referenced this pull request Apr 23, 2024
itazap pushed a commit that referenced this pull request May 14, 2024
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.

5 participants