Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Text Generation Functions: Add Benchmark Script #342

Merged
merged 30 commits into from
Sep 14, 2022
Merged

Text Generation Functions: Add Benchmark Script #342

merged 30 commits into from
Sep 14, 2022

Conversation

abheesht17
Copy link
Collaborator

@abheesht17 abheesht17 commented Sep 6, 2022 •
edited
Loading

@abheesht17 abheesht17 changed the title Text Generation Functions: Add Benchmark Text Generation Functions: Add Benchmark Script Sep 6, 2022
mattdangerw
mattdangerw requested changes Sep 6, 2022
View reviewed changes
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Thanks! Some high level organization comments for now. Will take a more in the weeds pass when those are addressed.

benchmark_scripts/text_generation/config.json Outdated Show resolved Hide resolved
benchmark_scripts/text_generation/run.py Outdated Show resolved Hide resolved
mattdangerw
mattdangerw requested changes Sep 7, 2022
View reviewed changes
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

Thanks! Few more comments.

keras_nlp/benchmarks/text_generation_config.py Outdated Show resolved Hide resolved
keras_nlp/benchmarks/text_generation.py Outdated Show resolved Hide resolved
keras_nlp/benchmarks/text_generation.py Outdated
return model


def run_graph(
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should just call this generate_text?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea was to have a separate function for eager mode. Is that worth adding to this script? Or does testing in graph mode suffice?

keras_nlp/benchmarks/text_generation.py Outdated Show resolved Hide resolved
mattdangerw
mattdangerw approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@mattdangerw mattdangerw left a comment

Choose a reason for hiding this comment

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

This looks great! Just one last comment. Also, can you rebase on the latest changes (we recently fixed our testing).

keras_nlp/benchmarks/text_generation.py Outdated Show resolved Hide resolved
@abheesht17
Copy link
Collaborator Author

This looks great! Just one last comment. Also, can you rebase on the latest changes (we recently fixed our testing).

Done!

@mattdangerw
Copy link
Member

This is good to go, we are just waiting for #341 to land, so we can land this PR with beam search.

@mattdangerw
Copy link
Member

@abheesht17 looks like this needs a reformat!

@abheesht17
Copy link
Collaborator Author

abheesht17 commented Sep 13, 2022 •
edited
Loading

@abheesht17 looks like this needs a reformat!

Yep, give me 15 minutes, I'm running the beam search test, will paste the output in the README

@abheesht17
Copy link
Collaborator Author

abheesht17 commented Sep 13, 2022 •
edited
Loading

@mattdangerw, this PR is ready to be merged.

chenmoneygithub
chenmoneygithub reviewed Sep 13, 2022
View reviewed changes
Copy link
Contributor

@chenmoneygithub chenmoneygithub left a comment

Choose a reason for hiding this comment

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

Thanks!

keras_nlp/benchmarks/README.md Outdated Show resolved Hide resolved
keras_nlp/benchmarks/text_generation.py Show resolved Hide resolved
mattdangerw
mattdangerw reviewed Sep 14, 2022
View reviewed changes
keras_nlp/benchmarks/README.md
| Beam Search | 564.23 | 615.17 |
| Random Search | 446.55 | 296.21 |
| Top-k Search | 458.68 | 302.66 |
| Top-p Search | 468.63 | 565.50 |
Copy link
Member

Choose a reason for hiding this comment

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

These are great results btw! Should we open up a bug for top-p being slower? Or do we already know why?

@mattdangerw mattdangerw merged commit 70ff7b8 into keras-team:master Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattdangerw mattdangerw mattdangerw approved these changes

@chenmoneygithub chenmoneygithub chenmoneygithub left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add some basic benchmarking for our text generation utilities
3 participants
@abheesht17 @mattdangerw @chenmoneygithub