Skip to content

Add BetterTransformer support for RemBERT#545

Merged
younesbelkada merged 3 commits into
huggingface:mainfrom
hchings:rembert-better-transformer
Dec 5, 2022
Merged

Add BetterTransformer support for RemBERT#545
younesbelkada merged 3 commits into
huggingface:mainfrom
hchings:rembert-better-transformer

Conversation

@hchings

@hchings hchings commented Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Add BetterTransformer support for RemBERT (part of #20372)

Before submitting

  • [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

Details

  • RemBERT's primary change is decoupling in/output embeddings. Since RemBERT uses the same Attention mechanism in BERT, it can use the same BertLayerBetterTransformer. (please correct me if I'm wrong)
  • Minor code cleanup to sort model names alphabetically as the convention in transformers repo.
  • All tests for encoders passed on GPU

Screen Shot 2022-12-04 at 3 57 10 PM

cc @younesbelkada, @michaelbenayoun, @fxmarty. Thanks.

@hchings hchings mentioned this pull request Dec 5, 2022
Closed
14 tasks
@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown

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

younesbelkada
younesbelkada approved these changes Dec 5, 2022
View reviewed changes

@younesbelkada younesbelkada 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.

Thanks so much for adding BetterTransformer support for RemBERT 🎉
Very clean PR! Thanks also for taking care of the alphabetical order on the testing suite
Looking forward to your next contributions 💪

@hchings

hchings commented Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @younesbelkada or @michaelbenayoun, I see that "first-time contributor needs a maintainer to approve running workflows". Could you help to approve that? Thanks!

@younesbelkada

younesbelkada commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

Thanks for the heads-up! Just run the workflow and we'll merge once it's green

@hchings

hchings commented Dec 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @younesbelkada, there are two checks failed.

1/ test_inference_speed() in test_bettertransformer_encoded.py: this test only runs on bert-base-uncased model and will pass on GPU (test screenshot attached above) but can occasionally fail on CPU. All BetterTransformer-related PRs could easily have this check failed. Should we open an Issue to look into this?

 AssertionError: 0.05959849110001869 not less than 0.056976150400032566 : The converted model is slower than the original model.

I think it's either PyTorch's issue or sth has to be changed in BertLayerBetterTransformer because even on CPU BetterTransformer is supposed to be faster. Please advise, thanks.

2/ ONNX Runtime - I saw this test has been failing on main branch. Since this is completely irrelevant to the code changes in this PR, can we ignore it for merging this PR?

@younesbelkada

younesbelkada commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

Hi @hchings
Thanks for your message
For 2/ this is expected as some tests are flaky so don't worry about that
For 1/ this is a bit strange since it passes on mac-os and not on ubuntu. This test used to be flaky and fails sometime. Let me get back to you on this

@younesbelkada

younesbelkada commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

Ok the test was just flaky, let's merge!

@younesbelkada
younesbelkada merged commit eff4c2b into huggingface:main Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelbenayoun michaelbenayoun michaelbenayoun approved these changes
+1 more reviewer
@younesbelkada younesbelkada younesbelkada 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.

4 participants

@hchings @HuggingFaceDocBuilderDev @younesbelkada @michaelbenayoun