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

BartForQuestionAnswering #4908

Merged
merged 12 commits into from Jun 12, 2020

Conversation

patil-suraj
Copy link
Contributor

This PR adds BartForQuestionAnswering.

Decided to add this models as BART is intended for both NLU and NLG tasks and also achieves comparable performance to ROBERTa on SQuAD.

Also fine-tuned the model here. The metrics are slightly worse than given in the paper. Got following metrics on SQuADv1
{'exact_match': 86.80227057710502, 'f1': 92.73424907872341}

@sshleifer , @patrickvonplaten

@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #4908 into master will increase coverage by 0.03%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4908      +/-   ##
==========================================
+ Coverage   76.99%   77.02%   +0.03%     
==========================================
  Files         128      128              
  Lines       21602    21635      +33     
==========================================
+ Hits        16633    16665      +32     
- Misses       4969     4970       +1     
Impacted Files Coverage Δ
src/transformers/__init__.py 99.13% <ø> (ø)
src/transformers/modeling_bart.py 96.26% <93.93%> (-0.15%) ⬇️
src/transformers/modeling_auto.py 78.40% <100.00%> (ø)
src/transformers/modeling_utils.py 90.49% <0.00%> (-0.12%) ⬇️
src/transformers/modeling_tf_utils.py 87.42% <0.00%> (+0.15%) ⬆️
src/transformers/file_utils.py 73.49% <0.00%> (+0.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac99217...63eb191. Read the comment docs.

Copy link
Contributor

@sshleifer sshleifer left a comment

Choose a reason for hiding this comment

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

LGTM

src/transformers/modeling_bart.py Outdated Show resolved Hide resolved
tests/test_modeling_bart.py Outdated Show resolved Hide resolved
@sshleifer sshleifer changed the title Bart for question answering BartForQuestionAnswering Jun 10, 2020
@sshleifer
Copy link
Contributor

Thanks for the contribution @patil-suraj !

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.

Hi! Very cool @patil-suraj.

Could you also add BartForQuestionAnswering to the all_model_classes in test_modeling_bart.py?

@patil-suraj
Copy link
Contributor Author

Hi! Very cool @patil-suraj.

Could you also add BartForQuestionAnswering to the all_model_classes in test_modeling_bart.py?

Hi, @LysandreJik
After adding BartForQuestionAnswering in all_model_classes I also had to add output_attention parameter to forward.

Now for some reason test_attention_outputs is failing, I am not sure why, could you help me fix it ?
Thanks !

@patrickvonplaten
Copy link
Contributor

Awesome work @patil-suraj - I can help you with this test :-)

@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Jun 11, 2020

I see what the problem is...it's actually not related to your PR at all. Can we you for now just remove BartForQuestionAnswering from the all_models tuples in the tests. @LysandreJik @sshleifer I will open a new PR after this one to fix it :-)

@patil-suraj
Copy link
Contributor Author

I see what the problem is...it's actually not related to your PR at all. Can we you for now just remove BartForQuestionAnswering from the all_models tuples in the tests. @LysandreJik @sshleifer I will open a new PR after this one to fix it :-)

Thank you @patrickvonplaten . I've removed it from all_models tuple for now

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.

None yet

4 participants