DPR usage of BertPooler

[PaulLerner]

Environment info

• transformers version: 4.8.2
• Platform: Linux-5.8.0-50-generic-x86_64-with-debian-bullseye-sid
• Python version: 3.7.4
• PyTorch version (GPU?): 1.5.1 (True)
• Tensorflow version (GPU?): not installed (NA)
• Flax version (CPU?/GPU?/TPU?): not installed (NA)
• Jax version: not installed
• JaxLib version: not installed

Who can help

RAG, DPR: @patrickvonplaten, @lhoestq

Information

DPR initializes BertModel with a BertPooler module which is not used in the end

Although this seems consistent with the original implementation, it is confusing for the user. One would expect that the pooled_output will come from the BertPooler module, if it is present, and the last layer of the model. Moreover it wastes memory and compute.

How to fix

Simply add the add_pooling_layer=False flag in https://github.com/huggingface/transformers/blob/master/src/transformers/models/dpr/modeling_dpr.py#L178
Some other parts of the code need also to be fixed, like https://github.com/huggingface/transformers/blob/master/src/transformers/models/dpr/modeling_dpr.py#L205
should be sequence_output = outputs[0]

[lhoestq]

Hi ! I think it was kept in case some version of DPR had a projection layer (but afaik none of the official versions have that and the paper doesn't mention that either).

I think it would also break the conversion script from DPR weights of the official repository to transformers weights.

Not sure exactly how such changes are welcomed in the library but @patrickvonplaten probably knows more?

[PaulLerner]

DPR has an optional projection layer in the original implementation but it is only applied on the sequence output, not on BertPooler's output.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

[patrickvonplaten]

Sorry to be so late on this. @PaulLerner - would you like to open a PR to fix it? Otherwise I can try to give it a shot

[PaulLerner]

No problem, I’m not sure to be familiar enough with the library to be able to fix it by myself.
For example, at first I thought one should simply add the add_pooling_layer=False flag in https://github.com/huggingface/transformers/blob/master/src/transformers/models/dpr/modeling_dpr.py#L178
But actually, some other parts of the code need also to be fixed, like https://github.com/huggingface/transformers/blob/master/src/transformers/models/dpr/modeling_dpr.py#L205
should be sequence_output = outputs[0]

And I have no clue about the conversion script for the weights that @lhoestq is talking about.

[patrickvonplaten]

Hey @PaulLerner, don't worry about the conversion script. I think it'll be enough to just fix everything as suggested by you in modeling_dpr.py - would you like to give it a try?

[PaulLerner]

Ok, I’ll let you know. I’m quite busy atm.