-
Notifications
You must be signed in to change notification settings - Fork 309
Autogenerate preset table #690
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
Conversation
mattdangerw
left a comment
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! Left some initial comments.
|
Thanks @Cyber-Machine ! I timed out this week, but will look at this next Monday! One thing you could do if you want to keep moving on this is start looking at the keras.io side. We probably want to ready the PRs in tandem, as we might discover changes we need here when rendering the table for our documentation. There's a few things you'll need to do there:
This will all be somewhat tricky as we are coordinating across repos, so if you have any question I can try to help out next Monday! |
|
Sure @mattdangerw, over this weekend I will look into ways how we can generate table on keras-io. Will keep you updated! |
mattdangerw
left a comment
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.
Looks great! Just a few small things.
| "Extra Large size of ALBERT where all input is lowercased. " | ||
| "Trained on English Wikipedia + BooksCorpus." | ||
| ), | ||
| "params": 222595584, |
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.
We can actually add "official_name" and "path" for all models, even ones that are not yet documented on keras.io.
The path should always match the directory the model is in in KerasNLP. The official_name should match the original paper.
| mask_positions, | ||
| mask_ids, | ||
| ) = tf_text.mask_language_model( | ||
| (token_ids, mask_positions, mask_ids,) = tf_text.mask_language_model( |
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.
If you update your install of black (pip install -U black), you should get the latest formatting here.
mattdangerw
left a comment
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! This looks good.
I think we should move this dict to the top of each preset, and I see some spots where our descriptions could still use some updating, but I can fix those up as I merge.
PR for #657.
Added metadata for generating preset table on keras-io.