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.

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

[TFBart] Split TF-Bart #9497

Merged
merged 34 commits into from
Jan 12, 2021
Merged

[TFBart] Split TF-Bart #9497

merged 34 commits into from
Jan 12, 2021

Conversation

patrickvonplaten
Copy link
Contributor

@patrickvonplaten patrickvonplaten commented Jan 10, 2021 •
edited
Loading

What does this PR do?

TF mirror of: #9343

After PR is merged TODO:

  • Open issue about facebook/blenderbot_small-90M tokenizer - cannot download files from hub. Weird issue

jplu
jplu approved these changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@jplu jplu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Awesome work!! Just left few smalll comments. I think we should first find a proper fix #9478 and then merging this one. Switching on/off some tests everytime we touch a model is really not a long term solution, I think a proper template as to be stated first and then afterwards we do the models.

src/transformers/models/bart/modeling_tf_bart.py Outdated Show resolved Hide resolved
src/transformers/models/bart/modeling_tf_bart.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_bart.py Show resolved Hide resolved
@patrickvonplaten
Copy link
Contributor Author

Awesome work!! Just left few smalll comments. I think we should first find a proper fix #9478 and then merging this one. Switching on/off some tests everytime we touch a model is really not a long term solution, I think a proper template as to be stated first and then afterwards we do the models.

IMO this PR should be merged and the s2s fix should be applied afterward as said offline. This PR is blocking a new release currently

@jplu
Copy link
Contributor

jplu commented Jan 11, 2021

IMO this PR should be merged and the s2s fix should be applied afterward as said offline. This PR is blocking a new release currently

Ok, nevermind, I didn't know you wanted to have it in the next release.

LysandreJik
LysandreJik approved these changes Jan 11, 2021
View reviewed changes
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few nits, but great work all around! Thank you for taking care of this.

src/transformers/models/bart/modeling_tf_bart.py Outdated Show resolved Hide resolved
src/transformers/models/bart/modeling_tf_bart.py Show resolved Hide resolved
src/transformers/models/blenderbot/modeling_tf_blenderbot.py Show resolved Hide resolved
src/transformers/models/blenderbot_small/modeling_tf_blenderbot_small.py Outdated Show resolved Hide resolved
src/transformers/models/blenderbot_small/modeling_tf_blenderbot_small.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_bart.py
@slow
class TFBartModelIntegrationTest(unittest.TestCase):
def test_inference_no_head(self):
model = TFBartModel.from_pretrained("facebook/bart-large", from_pt=True)
model = TFBartForConditionalGeneration.from_pretrained("facebook/bart-large").model
Copy link
Member

Choose a reason for hiding this comment

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

Why not use TFBartModel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't work, the weights don't load correctly...this didn't work previously either. Could be dealt with in a future PR

tests/test_modeling_tf_bart.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_blenderbot.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_blenderbot_small.py Outdated Show resolved Hide resolved
tests/test_modeling_tf_pegasus.py Outdated Show resolved Hide resolved
@patrickvonplaten patrickvonplaten mentioned this pull request Jan 11, 2021
Merged
sgugger
sgugger approved these changes Jan 11, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this! There are just a few repeated mistakes, that probably come from the templates.

src/transformers/models/blenderbot/modeling_tf_blenderbot.py Outdated Show resolved Hide resolved
src/transformers/models/blenderbot/modeling_tf_blenderbot.py Show resolved Hide resolved
src/transformers/models/blenderbot_small/modeling_tf_blenderbot_small.py Outdated Show resolved Hide resolved
src/transformers/models/blenderbot_small/modeling_tf_blenderbot_small.py Outdated Show resolved Hide resolved
src/transformers/models/blenderbot_small/modeling_tf_blenderbot_small.py Outdated Show resolved Hide resolved
src/transformers/models/pegasus/modeling_tf_pegasus.py Outdated Show resolved Hide resolved
src/transformers/models/pegasus/modeling_tf_pegasus.py Show resolved Hide resolved
src/transformers/models/pegasus/modeling_tf_pegasus.py Outdated Show resolved Hide resolved
src/transformers/models/pegasus/modeling_tf_pegasus.py Show resolved Hide resolved
tests/test_modeling_tf_bart.py Outdated Show resolved Hide resolved
@patrickvonplaten patrickvonplaten merged commit 7f28613 into huggingface:master Jan 12, 2021
@patrickvonplaten patrickvonplaten deleted the split_tf_bart branch January 12, 2021 01:06
guyrosin pushed a commit to guyrosin/transformers that referenced this pull request Jan 15, 2021
@patrickvonplaten @guyrosin
[TFBart] Split TF-Bart (huggingface#9497)
5bfb189 
* make templates ready

* make add_new_model_command_ready

* finish tf bart

* prepare tf mbart

* finish tf bart

* add tf mbart

* add marian

* prep pegasus

* add tf pegasus

* push blenderbot tf

* add blenderbot

* add blenderbot small

* clean-up

* make fix copy

* define blend bot tok

* fix

* up

* make style

* add to docs

* add copy statements

* overwrite changes

* improve

* fix docs

* finish

* fix last slow test

* fix missing git conflict line

* fix blenderbot

* up

* fix blenderbot small

* load changes

* finish copied from

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

@sgugger sgugger sgugger approved these changes

@LysandreJik LysandreJik LysandreJik approved these changes

@jplu jplu jplu approved these changes

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
@patrickvonplaten @jplu @LysandreJik @sgugger