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

Need suggestion on contributing TFDPR #8171

Closed
3 tasks done
ratthachat opened this issue Oct 30, 2020 · 8 comments
Closed
3 tasks done

Need suggestion on contributing TFDPR #8171

ratthachat opened this issue Oct 30, 2020 · 8 comments

Comments

@ratthachat
Copy link
Contributor

ratthachat commented Oct 30, 2020

🌟 New model addition

Model description

Hi, I would love to try contributing TFDPR . This is the first time to me, so I need some suggestions.
I have followed @sshleifer 's great PR on TFBart model on 4 files : __init__.py , convert_pytorch_checkpoint_to_tf2.py , utils/dummy_tf_objects.py and (newly created) modeling_tf_dpr.py

Now the TF model works properly and can load Pytorch's weights successfully the same output as Pytorch's counterparts except small random noise (1e-5) which I suspect of some dtypes different , but I could not find the cause.

I guess I need to add document on docs/source/model_doc/dpr.rst , and that's all ?
My question is do I need to change / fix any other files ? and/or do I need to do some other thing before making PR ?

To resolve TF vs. Pytorch naming issues, there's one change regarding TFBertModel vs. TFBertMainLayer as discussed here .
Thanks to @sshleifer for his help to solve the issue.

Open source status

@LysandreJik
Copy link
Member

Hello! Thanks for offering to contribute the TF implementation of the DPR model! Something that may help you is to open a PR very early on, even if you have a lot of questions. This way we can help provide pointers, and we can guide you in the right direction.

Another aspect that may be of tremendous help, would be to follow the checklist when adding a new model. It is available here. If you open a PR, we recommend to put this checklist in the description so that everybody can follow better.

Let me know if I can help further.

@ratthachat
Copy link
Contributor Author

ratthachat commented Oct 30, 2020

@LysandreJik Thanks for your suggestion and the checklist which is just what I want!
I will try to follow the checklist as much as possible and then PR. (UPDATED : already open a PR with checklist)
Please let me know if I should close this issue.

@ratthachat ratthachat mentioned this issue Oct 31, 2020
26 tasks
@LysandreJik
Copy link
Member

This is great, only the tests are left! No need to close the issue here, we can close this issue once the PR is merged.

@ratthachat
Copy link
Contributor Author

ratthachat commented Nov 2, 2020

Thanks for your kind words @LysandreJik !
At first, I have no idea how to test. Now I know I have to translate test_modeling_dpr.py and see an example on the recent test_modeling_tf_bart.py .

@ratthachat
Copy link
Contributor Author

ratthachat commented Nov 4, 2020

@LysandreJik :D
After several hours of testing and debugging, my current model is alreay passed 27 tests :D
The test run is in Colab here : (in the last cell)
https://colab.research.google.com/drive/1czS_m9zy5k-iSJbzA_DP1k1xAAC_sdkf?usp=sharing

My current repo already contained test_modeling_tf_dpr.py
Could you please suggest me the next step (make a repo update with latest Transformers ?)

@LysandreJik
Copy link
Member

The next steps would be for us to review what you've contributed until now! We'll take a look as soon as possible.

@ratthachat
Copy link
Contributor Author

Thanks Lysandre! I actually have aimed for TFRag . Meanwhile, I will make a new branch and use TFDPR on translating TFRag .

@ratthachat
Copy link
Contributor Author

Close the issue as TFDPR is already merged. Very happy. Thanks a lot everybody!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants