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

Add Base Preprocessor Class #638

Merged
merged 18 commits into from
Jan 9, 2023
Merged

Add Base Preprocessor Class #638

merged 18 commits into from
Jan 9, 2023

Conversation

abheesht17
Copy link
Collaborator

Resolves #636

mattdangerw
mattdangerw requested changes Jan 5, 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.

Looks good! A few coments.

keras_nlp/models/albert/albert_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/preprocessor.py Show resolved Hide resolved
keras_nlp/models/preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/bert/bert_preprocessor.py Show resolved Hide resolved
@abheesht17 abheesht17 requested a review from mattdangerw January 6, 2023 03:20
jbischof
jbischof reviewed Jan 6, 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.

Thanks @abheesht17 just some small comments! For sanity checking I'd recommend looking at a subclass "from_preset" docstring and running at least one class-level docstring in colab.

keras_nlp/models/bert/bert_preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/bert/bert_preprocessor.py Show resolved Hide resolved
keras_nlp/models/preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/preprocessor.py Outdated Show resolved Hide resolved
@abheesht17 abheesht17 requested a review from jbischof January 6, 2023 17:37
jbischof
jbischof reviewed Jan 6, 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.

One final concern: how can be make SentencePiece docstring examples runnable?

keras_nlp/models/deberta_v3/deberta_v3_preprocessor.py Show resolved Hide resolved
keras_nlp/models/preprocessor.py Outdated Show resolved Hide resolved
keras_nlp/models/xlm_roberta/xlm_roberta_preprocessor.py Show resolved Hide resolved
mattdangerw
mattdangerw approved these changes Jan 7, 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.

Just one last comment on my end! LGTM!

keras_nlp/models/roberta/roberta_preprocessor.py Show resolved Hide resolved
keras_nlp/models/deberta_v3/deberta_v3_preprocessor.py Show resolved Hide resolved
jbischof
jbischof approved these changes Jan 9, 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.

Amazing work, thank you!

@jbischof jbischof merged commit 402fc72 into keras-team:master Jan 9, 2023
@jbischof jbischof mentioned this pull request Jan 10, 2023
Merged
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

@jbischof jbischof jbischof 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.

Create Preprocessor base class
3 participants
@abheesht17 @jbischof @mattdangerw