-
Notifications
You must be signed in to change notification settings - Fork 301
Remove the old sampler utilities #948
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
Remove the old sampler utilities #948
Conversation
The code developed here all lives on in the new sampler API!
/gcbrun |
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!
import time | ||
|
||
import tensorflow as tf | ||
from tensorflow import keras |
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 need to update this README with new metrics: https://github.com/keras-team/keras-nlp/blob/master/benchmarks/README.md
Love the work thank you! |
``` | ||
|
||
On running this script on Google Colab (with Tesla T4 GPU, and TensorFlow 2.10.0), | ||
On running this script on Google Colab (with 3090 GPU, and TensorFlow 2.11.0), |
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.
I should probably upgrade my PC 🛩️
Minor comment, but this change will break this example (after release): https://keras.io/examples/nlp/neural_machine_translation_with_keras_nlp/. Considering this example is kinda popular, we should open a "contributions welcome" issue for this. |
| Greedy Search | 470.23 | 61.79 | | ||
| Beam Search | 530.13 | 189.61 | | ||
| Top-k Search | 374.05 | 62.87 | | ||
| Top-p Search | 401.97 | 260.31 | |
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.
Damn, crazy speedup!
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.
It's a bit apples to oranges. Switched hardware. We should consider updating this to a "cached" version. That will be a huge speedup.
Yeah absolutely. Was going to file a contributor issue to do this as #896 lands. Doesn't make too much sense to do while we are still tweaking the sampler interface. |
/gcbrun |
1 similar comment
/gcbrun |
The code developed here all lives on in the new sampler API!