Skip to content

Use new parametrization based weight norm if available#24030

Merged
sgugger merged 3 commits into
huggingface:mainfrom
ezyang:weight-norm
Jun 6, 2023
Merged

Use new parametrization based weight norm if available#24030
sgugger merged 3 commits into
huggingface:mainfrom
ezyang:weight-norm

Conversation

@ezyang

@ezyang ezyang commented Jun 5, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

In pytorch/pytorch#103001 I introduce a new
parametrization based version of weight_norm. One big benefit of
the new API is that the resulting model is deepcopy'able; today, you can't
deepcopy Wav2Vec2 models.

Since the new API isn't even in PyTorch main yet, I'd like to feature
gate it here, so that it gets used whenever PyTorch is recent enough to
support it. It would be a big help for me if you could take this change
earlier rather than later; otherwise I will have to patch transformers
in our own CI to get our benchmark harness working on Wav2Vec2.

Signed-off-by: Edward Z. Yang [email protected]

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?

cc @sanchit-gandhi

sgugger
sgugger approved these changes Jun 5, 2023
View reviewed changes

@sgugger sgugger left a comment •
edited
Loading

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 as it's properly gated. @sanchit-gandhi can you have a second look?
You'll just need to run make style and make fix-copies on your branch to make the CI happy.

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Jun 5, 2023 •
edited
Loading

Copy link
Copy Markdown

The documentation is not available anymore as the PR was closed or merged.

ezyang added 2 commits June 5, 2023 13:49
@ezyang
handle copies
e465577 
Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang
black
9f1610e 
Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang

ezyang commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

PyTorch side PR has landed!

sanchit-gandhi
sanchit-gandhi approved these changes Jun 6, 2023
View reviewed changes

@sanchit-gandhi sanchit-gandhi left a comment

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.

LGTM too - thanks for the PR @ezyang! Here's the PR in the PyTorch repo for reference: pytorch/pytorch#103001

@sgugger
sgugger merged commit bc9ecef into huggingface:main Jun 6, 2023
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
@ezyang @novice03
Use new parametrization based weight norm if available (huggingface#2…
71c7b38 
…4030)

* Use new parametrization based weight norm if available

See pytorch/pytorch#103001

Signed-off-by: Edward Z. Yang <[email protected]>

* handle copies

Signed-off-by: Edward Z. Yang <[email protected]>

* black

Signed-off-by: Edward Z. Yang <[email protected]>

---------

Signed-off-by: Edward Z. Yang <[email protected]>
@ydshieh

ydshieh commented Jun 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@ezyang

With nightly pytorch, we get

AttributeError: encoder.pos_conv_embed.conv.weight_v not found in PyTorch model

when trying to load a pytorch model into a TF model.

The TF model is looking for encoder.pos_conv_embed.conv.weight_v but we now have encoder.pos_conv_embed.conv.parametrizations.weight.original0. (This is from our (TF)Wav2Vec2Model model class).

Question: In your PR pytorch/pytorch#103001, is this part

def _weight_norm_compat_hook()

that deals with the backward compatibility? If so, we will copy it :-)

@ydshieh ydshieh mentioned this pull request Jun 28, 2023
Merged
ydshieh added a commit that referenced this pull request Jun 28, 2023
@ydshieh
Update PT/TF weight conversion after #24030 (#24547)
6c57ce1 
* fix

* fix

* fix

---------

Co-authored-by: ydshieh <[email protected]>
@ezyang

ezyang commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

Yep. The change here is not FC so the ingester needs updating.

ydshieh added a commit that referenced this pull request Jun 28, 2023
@ydshieh
Update PT/Flax weight conversion after #24030 (#24556)
faae8d8 
* fix

* fix

* fix

---------

Co-authored-by: ydshieh <[email protected]>
@ydshieh ydshieh mentioned this pull request Jul 7, 2023
Closed
@sanchit-gandhi sanchit-gandhi mentioned this pull request Sep 4, 2023
Merged
@sterlind sterlind mentioned this pull request Oct 13, 2023
Closed
4 tasks
@eindenbom eindenbom mentioned this pull request Nov 20, 2023
Closed
4 tasks
@jonnyli1125 jonnyli1125 mentioned this pull request May 24, 2024
Closed
4 tasks
@kamilakesbi kamilakesbi mentioned this pull request Jul 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@sgugger sgugger sgugger approved these changes
@sanchit-gandhi sanchit-gandhi sanchit-gandhi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ezyang @HuggingFaceDocBuilderDev @ydshieh @sgugger @sanchit-gandhi