-
Notifications
You must be signed in to change notification settings - Fork 287
Speed up default FNet testing speedups. #894
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
Speed up default FNet testing speedups. #894
Conversation
This looks like it has some legitimate test failures to work through before we can merge. Related to sentencepiece it seems. |
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.
Let's fix the tests!
@mattdangerw, Will be working towards checking all the tests. |
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 the PR! One minor comment.
Meanwhile, could you post the time cost change before and after this PR? You can run
pytest keras_nlp/models/f_net
on master and your branch separately.
@@ -25,68 +25,56 @@ | |||
|
|||
class FNetBackboneTest(tf.test.TestCase, parameterized.TestCase): | |||
def setUp(self): | |||
self.model = FNetBackbone( | |||
self.backbone = FNetBackbone( | |||
vocabulary_size=1000, |
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.
Let's reduce the vocab size too.
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 the PR! One minor comment.
Meanwhile, could you post the time cost change before and after this PR? You can run
pytest keras_nlp/models/f_net
on master and your branch separately.
Sure, I have performed tests on my FNet-test-speedups
branch and master
branch, and here are the results.
Before Changes:
master
branch- Time taken: 140.05s (2mins and 20 sec)
- 55 Tests passed, 32 Tests skipped
After Changes:
FNet-test-speedups
branch- Time taken: 45.73s
- 40 Tests passed, 38 Tests skipped
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!
Partially fixes: #866