-
Notifications
You must be signed in to change notification settings - Fork 251
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
bert_base_zh
, bert_base_multi_cased
: Add BERT Base Variants
#319
Conversation
@abheesht17 I think we would want to avoid the add_pooling_layer parameter if possible. One thing we could check is the bert checkpoints on the original bert repo. Could you see if those include the pooling layer? |
My bad. The pooling layers are present in this ckpt after all: https://storage.googleapis.com/tf_model_garden/nlp/bert/v3/multi_cased_L-12_H-768_A-12.tar.gz. Missed them earlier |
bert_base_chinese
bert_base_chinese
, bert_base_multi_cased
: Add Checkpoint Conversion Notebooks
bert_base_chinese
, bert_base_multi_cased
: Add Checkpoint Conversion Notebooksbert_base_zh
, bert_base_multi_cased
: Add Checkpoint Conversion Notebooks
bert_base_zh
, bert_base_multi_cased
: Add Checkpoint Conversion Notebooksbert_base_zh
, bert_base_multi_cased
: Add BERT Variants
bert_base_zh
, bert_base_multi_cased
: Add BERT Variantsbert_base_zh
, bert_base_multi_cased
: Add BERT Base Variants
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.
This looks good! We recently renamed the inputs to the models, could you make the changes in your colab to match?
#327
I think we are fine with the weights we have now (as the variable names do not depend on the input names), but might be good to double check the rename did not break the weights we uploaded.
" )\n", | ||
"\n", | ||
"model.get_layer(\"pooled_dense\").kernel.assign(\n", | ||
" weights[f\"encoder/layer_with_weights-{i + 5}/kernel/.ATTRIBUTES/VARIABLE_VALUE\"]\n", |
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.
This is using a variable (i) that is not out of scope. Can't you just do num_layers + X?
Same comment for other colab.
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.
This looks good to me! Thanks!
Resolves #308
Converted model weights and vocab have been uploaded here: https://drive.google.com/drive/folders/19APUi7fobORdQjoe8YDyk8ou0x_L6hWu?usp=sharing.
Edit: Merging #320 with this PR. Might cause conflicts while merging with
master
, otherwise.