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

GPT2 Text Generation APIs #592

Merged
merged 33 commits into from
Jan 27, 2023
Merged

GPT2 Text Generation APIs #592

merged 33 commits into from
Jan 27, 2023

Conversation

chenmoneygithub
Copy link
Contributor

@chenmoneygithub chenmoneygithub commented Dec 21, 2022 •
edited
Loading

You can view the demo via this link

@chenmoneygithub chenmoneygithub marked this pull request as draft December 21, 2022 02:13
@chenmoneygithub chenmoneygithub force-pushed the text-generation-playground branch 4 times, most recently from 8cd580c to e3a82a6 Compare January 3, 2023 23:29
@chenmoneygithub chenmoneygithub force-pushed the text-generation-playground branch from e3a82a6 to 0eb68f6 Compare January 4, 2023 00:01
@chenmoneygithub chenmoneygithub mentioned this pull request Jan 4, 2023
Merged
jbischof
jbischof reviewed Jan 10, 2023
View reviewed changes
Copy link
Contributor

@jbischof jbischof left a comment

Choose a reason for hiding this comment

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

Looks very clean, thanks for sharing! I would love to understand the CasualLMPreprocessor better.

The samplers/ code should be moved to a different PR.

keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
mattdangerw
mattdangerw requested changes Jan 11, 2023
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.

Not a full review, but left some quick comments!

keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
@chenmoneygithub chenmoneygithub force-pushed the text-generation-playground branch from 18fe549 to 8206103 Compare January 19, 2023 20:16
@chenmoneygithub chenmoneygithub force-pushed the text-generation-playground branch from 4d9a9b7 to 4fa8fc5 Compare January 20, 2023 02:38
@chenmoneygithub chenmoneygithub marked this pull request as ready for review January 20, 2023 02:39
mattdangerw
mattdangerw requested changes Jan 20, 2023
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.

Left some initial comments!

keras_nlp/samplers/sampler.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm_preprocessor.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Show resolved Hide resolved
@chenmoneygithub chenmoneygithub force-pushed the text-generation-playground branch from ac24a04 to cb12604 Compare January 23, 2023 23:35
jbischof
jbischof reviewed Jan 24, 2023
View reviewed changes
Copy link
Contributor

@jbischof jbischof left a comment

Choose a reason for hiding this comment

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

A lot of progress here! Left some comments.

Can you add a link to the demo colab? Would love to play around with this a bit.

keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Show resolved Hide resolved
keras_nlp/samplers/sampler.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm_preprocessor.py Show resolved Hide resolved
mattdangerw
mattdangerw requested changes Jan 25, 2023
View reviewed changes
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/samplers/sampler.py Outdated Show resolved Hide resolved
mattdangerw
mattdangerw approved these changes Jan 26, 2023
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.

LGTM! Let's consider a out of box option to prepend an "end of text" token to input sequences. I believe this is standard practice, but we can check.

Since this is a big one, probably good to wait for an ack from @jbischof too!

keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_preprocessor.py Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm.py Outdated Show resolved Hide resolved
keras_nlp/samplers/sampler.py Outdated Show resolved Hide resolved
jbischof
jbischof approved these changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@jbischof jbischof left a comment

Choose a reason for hiding this comment

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

The colab needs some updating but the PR is great. Thank you!

keras_nlp/samplers/sampler.py Outdated Show resolved Hide resolved
keras_nlp/models/gpt2/gpt2_causal_lm_preprocessor.py Show resolved Hide resolved
@chenmoneygithub chenmoneygithub merged commit 5737da9 into keras-team:master Jan 27, 2023
@jbischof jbischof mentioned this pull request Jan 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbischof jbischof jbischof approved these changes

@mattdangerw mattdangerw mattdangerw approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@chenmoneygithub @jbischof @mattdangerw