Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Added model file - CallmeMehdi #66

Merged
merged 12 commits into from
Aug 9, 2021
Merged

Conversation

CallmeMehdi
Copy link
Contributor

Any pull request you open is subject to the TensorFlow Hub Terms of Service at www.tfhub.dev/terms and Google's Privacy Policy at https://www.google.com/policies/privacy.

@google-cla
Copy link

google-cla bot commented Jul 26, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jul 26, 2021
Copy link

@MorganR MorganR left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Mehdi! Also please make sure to follow Google Bot's instructions and sign the CLA

assets/docs/CallmeMehdi/models/AraBERT/1.md Outdated Show resolved Hide resolved
assets/docs/CallmeMehdi/models/AraBERT/1.md Outdated Show resolved Hide resolved
assets/docs/CallmeMehdi/CallmeMehdi.md Outdated Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented Jul 27, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@CallmeMehdi
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Jul 27, 2021
@CallmeMehdi
Copy link
Contributor Author

Thanks for the PR Mehdi! Also please make sure to follow Google Bot's instructions and sign the CLA

@MorganR , I have done the changes you asked me to, and signed the CLA.

@WGierke
Copy link
Contributor

WGierke commented Jul 28, 2021

Hi @CallmeMehdi
Please provide the SavedModel in the tar.gz without any top-level directory like described here:

saved_model.tar.gz
├── assets/            # Optional.
├── assets.extra/      # Optional.
├── variables/
│     ├── variables.data-?????-of-?????
│     └──  variables.index
├── saved_model.pb
├── keras_metadata.pb  # Optional, only required for Keras models.
└── tfhub_module.pb    # Optional, only required for TF1 models.

The model files are currently containing within an arabert/ directory, which causes the validator to fail.

@CallmeMehdi
Copy link
Contributor Author

Thank you for the insight @WGierke , it looks like that was the problem.

Also, @MorganR, are there any other changes you are requesting me to do?


## License

[SOFTWARE LICENSE AGREEMENT - AraBERT](https://github.com/snakers4/silero-models/blob/master/LICENSE)
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely, I think I didn't save when I changed it. Changing it right now.

@CallmeMehdi
Copy link
Contributor Author

@MorganR I have added the changes you requested, can you please check them for me. Thank you.

Copy link

@MorganR MorganR left a comment

Choose a reason for hiding this comment

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

Looking much better! Nearly there

@@ -1,6 +1,6 @@
# Module callmemehdi/arabert/1

AraBERT is an Arabic pretrained lanaguage model based on Google's BERT architechture.
AraBERT is an Arabic pretrained lanaguage model based on Google's BERT base architechture.
Copy link

Choose a reason for hiding this comment

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

nit: architecture

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry Morgan I didn't get this remark.

Copy link

Choose a reason for hiding this comment

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

Just a small typo in architecture. You have an extra h part way through the word.

assets/docs/callmemehdi/models/AraBERT/1.md Show resolved Hide resolved
@@ -1,6 +1,6 @@
# Module callmemehdi/arabert/1
Copy link

Choose a reason for hiding this comment

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

Please update the 'arabert' casing here or in the folder name so that they match


```

We need to take the last hidden state with ``` outputs[0] ``` which has the following shape:
Copy link

Choose a reason for hiding this comment

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

It might be helpful to describe the outputs more. What else does the model output?

@CallmeMehdi
Copy link
Contributor Author

@MorganR, I've added the recent modification requests in the last commit, make sure to tell me if there are any notes or questions about it

Copy link

@MorganR MorganR left a comment

Choose a reason for hiding this comment

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

Just a few more small changes.

@@ -1,6 +1,6 @@
# Module callmemehdi/arabert/1

AraBERT is an Arabic pretrained lanaguage model based on Google's BERT architechture.
AraBERT is an Arabic pretrained lanaguage model based on Google's BERT base architechture.
Copy link

Choose a reason for hiding this comment

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

Just a small typo in architecture. You have an extra h part way through the word.

assets/docs/callmemehdi/models/AraBERT/1.md Show resolved Hide resolved

3- Hidden states: Tuple of tf.Tensor (one for the output of the embeddings + one for the output of each layer) of shape (batch_size, sequence_length, hidden_size).

We are interested in the last hidden output which is accessed with ``` outputs[0] ``` and has the following shape:
Copy link

Choose a reason for hiding this comment

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

I'm not sure this bit is needed. A person using this model may be interested in any of the outputs.

```
(1, sequence_length, 768)

With sequence_length the number of words in the input text
Copy link

Choose a reason for hiding this comment

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

This explanation of sequence length is helpful. It could be included in point 1 above.

@CallmeMehdi
Copy link
Contributor Author

@MorganR , I have done these changes you requested, please tell me if there is anything else you want me to change.

@MorganR MorganR merged commit 8d6ee20 into tensorflow:master Aug 9, 2021
@MorganR
Copy link

MorganR commented Aug 9, 2021

Thank you for your contribution. Your pull request has been accepted according to the TensorFlow Hub Terms of Service at www.tfhub.dev/terms and Google's Privacy Policy at https://www.google.com/policies/privacy. Your model should appear on tfhub.dev within a day.

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

Successfully merging this pull request may close these issues.

3 participants