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

Adding BERT for MS-MARCO Passage re-ranking #277

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

atif93
Copy link
Collaborator

@atif93 atif93 commented Dec 20, 2019

Adding BERT fine-tuned on MS-MARCO for passage re-ranking task (https://arxiv.org/abs/1901.04085)

Since this is a pretrained classifier, we had to add final linear layer parameters in PretrainedBERTMixin. Based on the pretrained_model_name, the weights of the final classifier layer will be loaded if they are present.

resolve #254

@atif93 atif93 self-assigned this Dec 20, 2019
@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #277 into master will decrease coverage by 0.05%.
The diff coverage is 21.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
- Coverage   83.07%   83.01%   -0.06%     
==========================================
  Files         195      195              
  Lines       15323    15338      +15     
==========================================
+ Hits        12729    12733       +4     
- Misses       2594     2605      +11
Impacted Files Coverage Δ
texar/torch/data/tokenizers/bert_tokenizer.py 88.88% <ø> (ø) ⬆️
texar/torch/modules/classifiers/bert_classifier.py 84.88% <100%> (+0.54%) ⬆️
texar/torch/modules/pretrained/bert.py 17.75% <6.25%> (-1.2%) ⬇️

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 54f9fee...6e3920a. Read the comment docs.

@atif93
Copy link
Collaborator Author

atif93 commented Dec 20, 2019

Wanted to get an idea of what you guys think of this design for loading a pretrained BERTClassifier config.
Next I'll be writing an example to test that we can reproduce the results of the paper using this.

@@ -71,6 +71,8 @@ def __init__(self,

super().__init__(hparams=hparams)

self.load_pretrained_config(pretrained_model_name, cache_dir)
Copy link
Collaborator

@gpengzhi gpengzhi Dec 20, 2019

Choose a reason for hiding this comment

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

Will load_pretrained_config and init_pretrained_weights be called twice (once in BERTClassifier, and once in BERTEncoder)?

Copy link
Collaborator

@gpengzhi gpengzhi Dec 20, 2019

Choose a reason for hiding this comment

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

If that is the case, we probably should not load the pre-trained weights in self._encoder (BERTEncoder).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed offline.
We can pass pretrained_model_name as None while instantiating the encoder in BERTClassifier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you set pretrained_model_name and pretrained_model_name in hparams to be None, BERTEncoder won't load the pre-trained weights.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made both the changes.


# BERT for MS-MARCO
'bert-msmarco-base': 512,
'bert-msmarco-large': 512,
Copy link
Member

Choose a reason for hiding this comment

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

This won't be the last/best Bert model for MS-Marco, so probably we'd come up with more specific names, say bert-msmarco-nguyen2019

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure let me change that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the names

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.

Support Re-ranking BERT
3 participants