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

BartForCausalLM analogs to ProphetNetForCausalLM #9128

Merged
merged 43 commits into from
Feb 4, 2021

Conversation

sadakmed
Copy link
Contributor

What does this PR do?

Implementing BartForCausalLM anologs for ProphetNetForCausalLM

Fixes #9066

@patrickvonplaten
Copy link
Contributor

Hy @sadakmed,

let me know if you need help on the issue or if you don't find the time to tackle it. I'll then just make it open to the "public" again :-)

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

It's great to see progress here! Good job. Note that BartForCausalLM should have the exact same functionality in terms of computing the loss as the BartForConditionalGeneration model => this means we can copy the way BartForConditionalGeneration computes the loss into BartForCausalLM.

Let me know if you need help or are stuck :-)

@sadakmed
Copy link
Contributor Author

sadakmed commented Jan 3, 2021

@patrickvonplaten The loss function it what I stuck on, thank you very much for your guidance.

Let me know if you need help or are stuck :-)

for sure I will ;-)

@patrickvonplaten
Copy link
Contributor

Hey @sadakmed,

Do you have an update on the PR? It's been three weeks now and it would be great to merge this soon. Sorry, we're very fast-moving in this lib and other community contributors have started asking for this feature. By next week, I'll probably have to take a look myself or redistribute the issue.

@sadakmed
Copy link
Contributor Author

sadakmed commented Jan 9, 2021

Hi @patrickvonplaten my apologies,

Could you Please see it now, lemme know if anything is missing.

@sadakmed
Copy link
Contributor Author

sadakmed commented Jan 9, 2021

Hi @patrickvonplaten, working on the test 'BartStandaloneCausalLM': I dont know if the self.model_tester in 'setUp' should be 'BartDecoderTester' (needed to be implemented), or do u recommend something else.

@patrickvonplaten
Copy link
Contributor

Hey @sadakmed,

Thanks a lot for you additions here :-)
Yes, we need a new BartStandaloneDecoderModelTester analog to how it's done for ProphetNet in tests/test_modeling_prophetnet.py. Do you want to give it a try? Otherwise, I can go into your PR and see how to add the tests :-)

@sadakmed
Copy link
Contributor Author

sadakmed commented Jan 11, 2021

Hi @patrickvonplaten

Do you want to give it a try?

of course, I'm working on it,

@sadakmed
Copy link
Contributor Author

Hi @patrickvonplaten, I just pushed the test, could you please check it out!
thaaaanks

@sadakmed
Copy link
Contributor Author

sadakmed commented Feb 2, 2021

@patrickvonplaten could you check please!

@patrickvonplaten
Copy link
Contributor

UPDATE:

@LysandreJik @sgugger
This PR enables all Bart-like models to be used in combination with the Encoder-Decoder framework. The model BartForCausalLM is added for Bart and then copied to all other models via the copying mechanism. Also, a new model tester is added for all those models.
While working on this I found a small bug for a very edge-case scenario for Bart and corrected it here: https://github.com/huggingface/transformers/pull/9128/files#r569436360 . The newly added tests were failing, which made me aware of the bug.
Also, I had to slightly change the check_repo.py file so that it counts both classes from all_model_classes with 1 and 2 paratheses.

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.

This is fantastic work! Thanks a lot!

src/transformers/__init__.py Outdated Show resolved Hide resolved
utils/check_repo.py Show resolved Hide resolved
utils/check_repo.py Show resolved Hide resolved
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.

Impressive work, great work @sadakmed and @patrickvonplaten!

Very cool that you completed the templates as well.

There seems to be a few issues with the "copied by" scripts in the templates, but the test still passes, which is weird.

@patrickvonplaten
Copy link
Contributor

Great job @sadakmed

@patrickvonplaten patrickvonplaten merged commit 0003178 into huggingface:master Feb 4, 2021
@sadakmed
Copy link
Contributor Author

sadakmed commented Feb 4, 2021

Great job @sadakmed

wouldn't happen without you, thank you very much. see u in the next PR ;)

@LysandreJik LysandreJik mentioned this pull request Feb 4, 2021
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.

Add BartForCausalLM analogs to ProphetNetForCausalLM
4 participants