Correct order of overflowing tokens for LayoutLmV2 tokenizer #13495

Apoorvgarg-creator · 2021-09-09T12:43:38Z

What does this PR do?

This PR objective is same as #13179
Fixes #13148
The issue was resolved for every tokenizer except the LayoutLmV2 tokenizer.

Before submitting

Did you read the contributor guideline,
Pull Request section? Yes 👍🏻.
Was this discussed/approved via a Github issue or the forum? Yes👍🏻 , Slow tokenizers return overflowing tokens in reversed order #13148
Did you make sure to update the documentation with your changes? Yes 👍🏻.
Did you add any necessary tests?
The following test have been added which check the sequence of overflowing tokens, bbox sequence and input_ids.
- test_maximum_encoding_length_pair_input
- test_maximum_encoding_length_single_input

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@LysandreJik @sgugger @SaulLu @NielsRogge

SaulLu · 2021-09-09T13:10:58Z

Thank you very much for the PR.

As it seems to me that we didn't see any tests failing for LayoutLMv2 at the time your previous PR was merged, could you confirm that this behaviour is tested for LayoutLMv2 now? As I see that in the PR you did not change any tests, I'd like to make sure we catch the problem with the tests next time 🙂 .

Apoorvgarg-creator · 2021-09-09T13:23:05Z

As I see that in the PR you did not change any tests, I'd like to make sure we catch the problem with the tests next time 🙂 .

@SaulLu, The test test_maximum_encoding_length_pair_input and test_maximum_encoding_length_single_input were skipped because of the reason LayoutLMv2 tokenizer requires boxes besides sequences.
I will make the necessary changes to the file test_tokenization_layoutlmv2.py to ensure that we catch the problem next time.
Thank you

Apoorvgarg-creator · 2021-09-17T17:23:21Z

@SaulLu, I have added a function get_clean_sequences which gives us list of words along with their ids and boxes, have taken reference from the test_tokenization_common.py.
Furthermore, I have modified the test_maximum_encoding_length_single_input to ensure the correct order of overflowing order.
Please can you review the changes.
Thank you

…order-fix

SaulLu

Thank you very much for all this work @Apoorvgarg-creator. 🎊

I left you mostly comments about the testing part. Especially as LayoutLmV2 has extra boxes, I think we should also add checks on the corresponding sequences.

Finally, if I'm not mistaken, there is no test_maximum_encoding_length_pair_input test for lajoutlmv2, it would be nice to have it too. 😄

Let me know if you need some help!

tests/test_tokenization_layoutlmv2.py

Apoorvgarg-creator · 2021-09-30T02:37:45Z

@SaulLu, I have gone through the reviews. I will make the changes.
I didn't commit test_maximum_encoding_length_pair_input function because I wanted to see the changes that are required in the get_clean_sequences and test_maximum_encoding_length_single_input.
Thank you for reviewing and suggesting the changes.

…order-fix

Apoorvgarg-creator · 2021-10-15T18:52:40Z

@SaulLu @NielsRogge @LysandreJik, Sorry for the delay in the PR. I am done with most of the work, but I would like to ask for help in Comparing bbox sequence for fast tokenizer for test_maximum_encoding_length_pair_input. That test isn't working properly. Can you help me from where I should read about this ?
I am sorry once again for delaying the work.
Update: I have resolved the problem I was facing.

Apoorvgarg-creator · 2021-10-16T01:38:06Z

@SaulLu, Working on test_maximum_encoding_length_pair_input. Will Commit the changes by end of the day.

Apoorvgarg-creator · 2021-10-21T19:20:23Z

@SaulLu @LysandreJik @NielsRogge, could you please review the test added and changes.
Thank you

…order-fix

Apoorvgarg-creator · 2021-10-30T11:55:32Z

@patrickvonplaten @SaulLu @LysandreJik @NielsRogge , Could you please review the PR.
Thank you.

LysandreJik

Thank you for the very extensive tests, great work!
Regarding the format:

Could you add some comments explaining what is done? There is a lot of code so I fear it will become unmaintainable without clear comments explaining what is done.
Please replace all assert with unittest's specific methods.

I'll let @NielsRogge review the logic.

tests/test_tokenization_layoutlmv2.py

Apoorvgarg-creator · 2021-11-01T13:37:41Z

Could you add some comments explaining what is done? There is a lot of code so I fear it will become unmaintainable without clear comments explaining what is done.

Sure, Thank you for reviewing.

src/transformers/models/layoutlmv2/tokenization_layoutlmv2.py

…hub.com/Apoorvgarg-creator/transformers into LayoutLmV2-overflowing-tokens-order-fix

NielsRogge

This PR looks good to me.

Pinging @SaulLu for a final review.

SaulLu

Thank you very much for all your work @Apoorvgarg-creator !

Thanks especially for adding all those tests! I have one last little request which is about the overflowing boxes check, but to save you time I have opened a PR on your branch which includes the changes I am referring to. If you like these proposals, don't hesitate to merge this PR in your branch. 🙂

tests/test_tokenization_layoutlmv2.py

Co-authored-by: SaulLu <55560583+SaulLu@users.noreply.github.com>

add overflowing bbox test

…order-fix

SaulLu

Thank you for these latest changes! It's all good for me!

Apoorvgarg-creator · 2021-11-09T05:39:46Z

@SaulLu @LysandreJik, I have been getting these mails, I tried checking the reason for their failing but couldn't understand the issue.
Message : This request was automatically failed because there were no enabled runners online to process the request for more than 1 days.
Here the Screenshot

sgugger · 2021-11-09T12:50:00Z

Thnaks a gain for all your work on this!

…face#13495) * correct order of overflowing tokens for LayoutLmV2 tokenizer * test to check order of overflowing_tokens for a seq of input_ids * fix up quality * added suggested changes * check that tests the bbox sequence * pair_input test added * pass quality test * check bbox sequence added * unittest method * comments added * add overflowing bbox test * improved "seq_1" Co-authored-by: SaulLu <55560583+SaulLu@users.noreply.github.com> * improve code quality Co-authored-by: SaulLu <lucilesaul.com@gmail.com> Co-authored-by: SaulLu <55560583+SaulLu@users.noreply.github.com>

correct order of overflowing tokens for LayoutLmV2 tokenizer

d48719c

Apoorvgarg-creator changed the title ~~Correct order of overflowing tokens for LayoutLmV2 tokenizer~~ [WIP] Correct order of overflowing tokens for LayoutLmV2 tokenizer Sep 9, 2021

Apoorvgarg-creator added 2 commits September 17, 2021 11:22

test to check order of overflowing_tokens for a seq of input_ids

1a6465f

fix up quality

930cd3d

Apoorvgarg-creator added 2 commits September 20, 2021 12:19

Merge branch 'huggingface:master' into LayoutLmV2-overflowing-tokens-…

ca9725a

…order-fix

Merge branch 'huggingface:master' into LayoutLmV2-overflowing-tokens-…

7cb7f0f

…order-fix

LysandreJik requested a review from NielsRogge September 21, 2021 19:35

Merge branch 'huggingface:master' into LayoutLmV2-overflowing-tokens-…

f6b627d

…order-fix

SaulLu reviewed Sep 29, 2021

View reviewed changes

Apoorvgarg-creator and others added 2 commits October 1, 2021 14:50

Merge branch 'huggingface:master' into LayoutLmV2-overflowing-tokens-…

c46735d

…order-fix

added suggested changes

db81bf0

Apoorvgarg-creator and others added 2 commits October 16, 2021 00:28

Merge branch 'master' into LayoutLmV2-overflowing-tokens-order-fix

079791e

check that tests the bbox sequence

4916005

Apoorvgarg-creator changed the title ~~[WIP] Correct order of overflowing tokens for LayoutLmV2 tokenizer~~ Correct order of overflowing tokens for LayoutLmV2 tokenizer Oct 15, 2021

Apoorvgarg-creator requested a review from SaulLu October 15, 2021 20:11

Apoorvgarg-creator added 3 commits October 16, 2021 11:49

pair_input test added

ee7ba88

pass quality test

1908175

check bbox sequence added

2d37643

Apoorvgarg-creator added 2 commits October 25, 2021 09:32

Merge branch 'huggingface:master' into LayoutLmV2-overflowing-tokens-…

48fdfc4

…order-fix

Merge branch 'huggingface:master' into LayoutLmV2-overflowing-tokens-…

ab53643

…order-fix

LysandreJik reviewed Nov 1, 2021

View reviewed changes

tests/test_tokenization_layoutlmv2.py Outdated Show resolved Hide resolved

NielsRogge reviewed Nov 1, 2021

View reviewed changes

src/transformers/models/layoutlmv2/tokenization_layoutlmv2.py Show resolved Hide resolved

Apoorvgarg-creator added 2 commits November 2, 2021 12:58

unittest method

5527d92

Merge branch 'LayoutLmV2-overflowing-tokens-order-fix' of https://git…

f125e43

…hub.com/Apoorvgarg-creator/transformers into LayoutLmV2-overflowing-tokens-order-fix

Apoorvgarg-creator changed the title ~~Correct order of overflowing tokens for LayoutLmV2 tokenizer~~ [WIP] Correct order of overflowing tokens for LayoutLmV2 tokenizer Nov 2, 2021

NielsRogge approved these changes Nov 5, 2021

View reviewed changes

comments added

0f798fb

Apoorvgarg-creator changed the title ~~[WIP] Correct order of overflowing tokens for LayoutLmV2 tokenizer~~ Correct order of overflowing tokens for LayoutLmV2 tokenizer Nov 7, 2021

add overflowing bbox test

8db85b9

SaulLu mentioned this pull request Nov 8, 2021

add overflowing bbox test Apoorvgarg-creator/transformers#2

Merged

SaulLu reviewed Nov 8, 2021

View reviewed changes

tests/test_tokenization_layoutlmv2.py Outdated Show resolved Hide resolved

Apoorvgarg-creator and others added 4 commits November 8, 2021 23:47

improved "seq_1"

b3ffbd2

Co-authored-by: SaulLu <55560583+SaulLu@users.noreply.github.com>

Merge pull request #2 from SaulLu/add-overflowing-bbox-test

06c179b

add overflowing bbox test

Merge branch 'huggingface:master' into LayoutLmV2-overflowing-tokens-…

91d8987

…order-fix

improve code quality

6535bb4

huggingface deleted a comment Nov 8, 2021

SaulLu self-requested a review November 8, 2021 18:52

SaulLu approved these changes Nov 8, 2021

View reviewed changes

Apoorvgarg-creator requested a review from LysandreJik November 9, 2021 04:42

sgugger merged commit 6326aa4 into huggingface:master Nov 9, 2021

Apoorvgarg-creator deleted the LayoutLmV2-overflowing-tokens-order-fix branch November 9, 2021 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct order of overflowing tokens for LayoutLmV2 tokenizer #13495

Correct order of overflowing tokens for LayoutLmV2 tokenizer #13495

Apoorvgarg-creator commented Sep 9, 2021 •

edited

Loading

SaulLu commented Sep 9, 2021

Apoorvgarg-creator commented Sep 9, 2021 •

edited

Loading

Apoorvgarg-creator commented Sep 17, 2021

SaulLu left a comment

Apoorvgarg-creator commented Sep 30, 2021

Apoorvgarg-creator commented Oct 15, 2021 •

edited

Loading

Apoorvgarg-creator commented Oct 16, 2021

Apoorvgarg-creator commented Oct 21, 2021

Apoorvgarg-creator commented Oct 30, 2021

LysandreJik left a comment

Apoorvgarg-creator commented Nov 1, 2021 •

edited

Loading

NielsRogge left a comment

SaulLu left a comment

SaulLu left a comment

Apoorvgarg-creator commented Nov 9, 2021 •

edited

Loading

sgugger commented Nov 9, 2021

Correct order of overflowing tokens for LayoutLmV2 tokenizer #13495

Correct order of overflowing tokens for LayoutLmV2 tokenizer #13495

Conversation

Apoorvgarg-creator commented Sep 9, 2021 • edited Loading

What does this PR do?

Before submitting

Who can review?

SaulLu commented Sep 9, 2021

Apoorvgarg-creator commented Sep 9, 2021 • edited Loading

Apoorvgarg-creator commented Sep 17, 2021

SaulLu left a comment

Choose a reason for hiding this comment

Apoorvgarg-creator commented Sep 30, 2021

Apoorvgarg-creator commented Oct 15, 2021 • edited Loading

Apoorvgarg-creator commented Oct 16, 2021

Apoorvgarg-creator commented Oct 21, 2021

Apoorvgarg-creator commented Oct 30, 2021

LysandreJik left a comment

Choose a reason for hiding this comment

Apoorvgarg-creator commented Nov 1, 2021 • edited Loading

NielsRogge left a comment

Choose a reason for hiding this comment

SaulLu left a comment

Choose a reason for hiding this comment

SaulLu left a comment

Choose a reason for hiding this comment

Apoorvgarg-creator commented Nov 9, 2021 • edited Loading

sgugger commented Nov 9, 2021

Apoorvgarg-creator commented Sep 9, 2021 •

edited

Loading

Apoorvgarg-creator commented Sep 9, 2021 •

edited

Loading

Apoorvgarg-creator commented Oct 15, 2021 •

edited

Loading

Apoorvgarg-creator commented Nov 1, 2021 •

edited

Loading

Apoorvgarg-creator commented Nov 9, 2021 •

edited

Loading