-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
add task_type_id to BERT to support ERNIE-2.0 and ERNIE-3.0 models #18686
Conversation
The documentation is not available anymore as the PR was closed or merged. |
Hey @nghuyong, thanks for your PR! In this situation, we'd rather have a new model class "Ernie" rather than modifying the "Bert" model class. This will result in a larger PR, but it should be very little additional work for you. I encourage you to follow the following guide: https://github.com/huggingface/transformers/tree/main/templates/adding_a_new_model#add-new-model-like-command It seems like it would be as simple as using the scfript to add a new model like BERT, but with the ERNIE name; then applying the changes above. @ydshieh, would you be down to help @nghuyong if they run into any problem? Thanks a lot! |
Thanks for your advice. I will try to do this work. |
Agree with @LysandreJik to have a new model file for this. And it should be fairly straightforward. Glad to know you already have the checkpoints available! Let me me if you need any help, @nghuyong for the PR. |
11c5c73
to
1e954cd
Compare
@ydshieh @LysandreJik I have updated this PR and add Ernie model. |
Wonderful, thank you @nghuyong! |
Hi @nghuyong, Great job! I will review the PR. Currently, the failing tests are caused by E AttributeError: module transformers.models.ernie has no attribute BasicTokenizer which is from your change in
I will discuss my colleagues to see how should we do for the tokenizers. |
@ydshieh So, is there anything that needs to be updated now? |
Hi @nghuyong Thanks a lot :-). I will take a full review this week. The current failing tests (most of them) could be fixed by updating your branch. You can do it as git checkout main
git pull upstream main
git checkout [YOUR-WORKING-BRANCH]
git rebase main
git push --force-with-lease Before doing so, it would be a good idea to keep a backup of current branch in case of the commit history being messed up. git checkout add_task_type_id
git checkout -b add_task_type_id_backup Once the PR branch is updated, you can also try to fix the style/quality issues by running make style
make quality You can check the CI results in ci/circleci: check_code_quality
ci/circleci: check_repository_consistency for the details and suggestions. Let me know if you encounter any difficulty. |
de52c3c
to
eb48d3d
Compare
Thanks, @ydshieh my branch has been synced with master now |
@nghuyong |
@HUSTHY, still not, and |
已收到!谢谢! ——黄洋
|
@nghuyong 好吧 我还以为能用了呢 那你实现的这个分支的代码是没有问题的吧。。。 我直接把代码放在本地应该OK的 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you a lot @nghuyong for adding this model. It is in great shape already, and the review is quite smooth 🤗 .
I left some comments to be addressed.
Once everything is ready, we have a few steps to take before merge. For example,
make fix-copies
is required. But we can do this at the final commit(s).
@ydshieh OK, thanks!! |
You can ignore the test failure in |
@ydshieh OK, |
@ydshieh OK, thanks a lot |
@sgugger This is model is just BERT, but with a new argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! I left a couple of comments, mainly around documentation.
Also please rename everywhere ErnieLMHeadModel
to ErnieForCausalLM
(we can't do the rename for BERT for backward compatibility reasons, but for new models we prefer this terminology).
do not expose ErnieLayer update doc
Thanks again for your contribution! @ydshieh can't merge until you change your "Request changes" to an approval. |
Thank you @nghuyong! I pushed a final commit to remove the remaining @sgugger I approved :-) |
failed test is irrelevant to this PR. |
…uggingface#18686) * add_ernie * remove Tokenizer in ernie * polish code * format code style * polish code * fix style * update doc * make fix-copies * change model name * change model name * fix dependency * add more copied from * rename ErnieLMHeadModel to ErnieForCausalLM do not expose ErnieLayer update doc * fix * make style * polish code * polish code * fix * fix * fix * fix * fix * final fix Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
What does this PR do?
ERNIE2.0 and ERNIE3.0 are a series of powerful models based on BERT, especially in Chinese tasks. These models introduce
task_type_embeddings
in the embedding layer, so this PR is to support this feature.the config of ERNIE2.0 / ERNIE3.0 models have the following two params:
the released ERNIE2.0 / ERNIE3.0 models have the weight of
bert.embeddings.task_type_embeddings.weight
Before submitting
Pull Request section?
to it if that's the case. https://github.com/huggingface/transformers/issues?q=is%3Aissue+ernie
documentation guidelines, and
here are tips on formatting docstrings.
I write a script to convert the official released ERNIE3.0 model (paddlepaddle version). And I have checked that the model results before and after transformation are consistent (with
task_type_embedding
added)Who can review?
@LysandreJik