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

TF/Numpy variants for all DataCollator classes #13105

Merged
merged 36 commits into from Aug 31, 2021
Merged

TF/Numpy variants for all DataCollator classes #13105

merged 36 commits into from Aug 31, 2021

Conversation

Rocketknight1
Copy link
Member

@Rocketknight1 Rocketknight1 commented Aug 12, 2021 •
edited

This is a draft PR again - I've written an example of what a TF variant of one of our data collators would look like. If we're happy with this format, it should be easy to expand it to support Numpy/JAX as well, and to do the same for other data collators, and I'll probably add most of the other data collators to this PR before merging it. Let me know what you think!

sgugger
sgugger approved these changes Aug 12, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Great addition! We could also add the NumPy part for the Flax/Jax folks

src/transformers/data/data_collator.py Outdated Show resolved Hide resolved
@Rocketknight1 Rocketknight1 mentioned this pull request Aug 12, 2021
Closed
4 tasks
sgugger
sgugger reviewed Aug 13, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Left a few more nits to polish the PR.

src/transformers/data/data_collator.py Outdated Show resolved Hide resolved
src/transformers/data/data_collator.py Outdated Show resolved Hide resolved
src/transformers/data/data_collator.py Show resolved Hide resolved
src/transformers/data/data_collator.py Outdated Show resolved Hide resolved
src/transformers/data/data_collator.py Outdated Show resolved Hide resolved
@Rocketknight1
Copy link
Member Author

More updates done - please note that tests will fail until all of the data collators are updated, because I removed the top-level imports. I definitely won't be merging this until that's done, don't worry!

@Rocketknight1 Rocketknight1 changed the title Adding a TF variant of the DataCollatorForTokenClassification TF/Numpy variants for all DataCollator classes Aug 19, 2021
@Rocketknight1
Copy link
Member Author

All the classes are in! Thank you to @aromans and @sdwalker62, whose PR #12199 I cannibalized for MLM and its variants. Next step is finishing tests and making sure all of this actually works.

sgugger
sgugger approved these changes Aug 30, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for adapting all of those and writing all the tests.

tests/test_data_collator.py Outdated Show resolved Hide resolved
src/transformers/data/data_collator.py Outdated Show resolved Hide resolved
Rocketknight1 and others added 19 commits August 30, 2021 18:54
@Rocketknight1
Adding a TF variant of the DataCollatorForTokenClassification to get …
6775d6d 
…feedback
@Rocketknight1
Added a Numpy variant and a post_init check to fail early if a missin…
0ae2587 
…g import is found
@Rocketknight1
Fixed call to Numpy variant
d577ac9
@Rocketknight1
Added a couple more of the collators
346a646
@Rocketknight1 @sgugger
Update src/transformers/data/data_collator.py
33dbb29 
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@Rocketknight1
Fixes, style pass, finished DataCollatorForSeqToSeq
70168b5
@Rocketknight1
Added all the LanguageModeling DataCollators, except SOP and Permutat…
050208b 
…ionLanguageModeling
@Rocketknight1
Adding DataCollatorForPermutationLanguageModeling
ce8dcc3
@Rocketknight1
Style pass
dda8906
@Rocketknight1
Add missing __call__ for PLM
f81068b
@Rocketknight1
Remove post_init checks for frameworks because the imports inside t…
abf8325 
…hem were making us fail code quality checks
@Rocketknight1
Remove unused imports
52cf828
@Rocketknight1
First attempt at some TF tests
4b823f9
@Rocketknight1
A second attempt to make any of those tests actually work
6cf757a
@Rocketknight1
TF tests, round three
6db05c6
@Rocketknight1
TF tests, round four
212b6e0
@Rocketknight1
TF tests, round five
dbb3848
@Rocketknight1
TF tests, all enabled!
2d59e3d
@Rocketknight1
Style pass
eed3c88
Rocketknight1 and others added 3 commits August 30, 2021 19:31
@Rocketknight1 @sgugger
Update src/transformers/data/data_collator.py
dc58c69 
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@Rocketknight1
Renaming mixin
375c316
@Rocketknight1
Updating docs
28606ec
LysandreJik
LysandreJik approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@LysandreJik LysandreJik 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 to me, thank you @Rocketknight1! Thanks for writing such extensive tests.

@Rocketknight1
Copy link
Member Author

Hi @aromans and @sdwalker62, we're ready to merge now. I just realized I'll need your Github no-reply e-mail addresses to add you though - see the docs here.

@sdwalker62
Copy link
Contributor

dalton_walker@icloud.com

@Rocketknight1
Copy link
Member Author

Thanks!

@aromans
Copy link
Contributor

aromans commented Aug 31, 2021 •
edited

andrew.romans@hotmail.com

@Rocketknight1 Rocketknight1 merged commit 854260c into master Aug 31, 2021
@Rocketknight1 Rocketknight1 deleted the tf_data_collators branch August 31, 2021 12:06
@Rocketknight1
Copy link
Member Author

It's in, and all authors have been properly credited! If you want to delete the messages with your e-mails (in case of spambot harvesting), feel free.

@albertvillanova albertvillanova mentioned this pull request Apr 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgugger sgugger sgugger approved these changes

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

None yet
5 participants
@Rocketknight1 @sdwalker62 @aromans @LysandreJik @sgugger