Adding DocTest to TrOCR by arnaudstiegler · Pull Request #16398 · huggingface/transformers

arnaudstiegler · 2022-03-24T22:18:51Z

Adding TrOCR to DocTests

For this model, there was actually a single docstring in the entire file (for the forward method).

A couple of comments:

As far as I'm aware, there is no TF version of this model
TrOCR is an edge case because it's meant to be used as the decoder for a VisionEncoderDecoder. So the forward function of the TrOCR is not meant to be called directly. As a result, I gave some example code to run a forward pass with TrOCR within a VisionEncoderDecoder

Let me know if the docstring is relevant to the problem. I can also revert adapt it to actually just showcase the forward for the TrOCRForCausalLM outside of a VisionEncoderDecoder

@patrickvonplaten @ydshieh @patil-suraj

HuggingFaceDocBuilderDev · 2022-03-24T22:32:28Z

The documentation is not available anymore as the PR was closed or merged.

ydshieh · 2022-03-24T23:04:46Z

I took a quick look for now, and think it is indeed a nice addition, considering there is currently no example in modeling_trocr.py at all.
Thank you, @arnaudstiegler!

ydshieh

@arnaudstiegler

Very nice & clean! I left a few tiny comments, but this PR is ready to be merged!

ydshieh · 2022-03-25T08:22:43Z

src/transformers/models/trocr/modeling_trocr.py

Would be great to avoid import VisionEncoderDecoderModel twice

Good catch! Removed it

ydshieh · 2022-03-25T08:24:13Z

src/transformers/models/trocr/modeling_trocr.py

This is better to be after showing the dummy model, i.e. after the line

>>> model = VisionEncoderDecoderModel(encoder=encoder, decoder=decoder)

ydshieh · 2022-03-25T08:25:28Z

src/transformers/models/trocr/modeling_trocr.py

Maybe add a comment here

>>> # init vision2text model with random weights

ydshieh · 2022-03-25T08:29:18Z

src/transformers/models/trocr/modeling_trocr.py

Maybe put this line just before

>>> text = "hello world"

just a nit: pixel_values before text 🙂

The reason is: pixel_values is the encoder part, and text is the decoder part. In encoder-decoder architecture, encoder is run before the decoder.

ydshieh · 2022-03-25T08:33:17Z

utils/documentation_tests.txt

nice ❤️!

Are you able to run the doctest locally and get it pass?

Yep, runs fine locally! I ran the test again after addressing your comments

arnaudstiegler · 2022-03-25T13:57:00Z

Addressed the comments and re-ran the test locally!
There's one thing though: make fixup messes up the formatting of the docstring, and removes the additional blank at the end of the docstring (here). The issue is that without the additional line, doctest will fail when comparing outputs. Not sure why to be honest, but do you know how to prevent that make fixup from doing this? Otherwise, next time the file is touched, make fixup will introduce a bug in the docstring.

Here's the failure:

Expected:
    ['industry, " Mr. Brown commented icily. " Let us have a']
    ```
Got:
    ['industry, " Mr. Brown commented icily. " Let us have a']

Essentially, without the additional line, the test consider that ``` is part of the expected output. But that additional line gets removed by make fixup. I haven't found a good way around this yet

@ydshieh

ydshieh · 2022-03-25T14:38:09Z

Those blank lines issues should be treated by the places regarding utils/prepare_for_doc_test.py as shown in this guide doc.

This means you don't need to add extra blank line in the file. Did you run python utils/prepare_for_doc_test.py as in the guide before and after running the doctest?

arnaudstiegler · 2022-03-25T15:07:32Z

Got it! I did run the script and it seems like it's not being applied to the trocr (but I can see the added lines on other model files). I'll debug that today

ydshieh · 2022-03-25T15:20:29Z

OK, thank you. Don't hesitate to report it if you find this is some bug in prepare_for_doc_test.py.

arnaudstiegler · 2022-03-25T22:19:47Z

OK, thank you. Don't hesitate to report it if you find this is some bug in prepare_for_doc_test.py.

Figured out the issue: there was some formatting issue in one of the docstring (see last commit) that prevented the script to correctly add a line. From utils.prepare_for_doc_test.process_doc_file

splits = code.split("```")
splits = [s if i % 2 == 0 else process_code_block(s, add_new_line=add_new_line) for i, s in enumerate(splits)]

splits were incorrect because of that, and the script wouldn't add an additional \n
Now the doctest runs fine post make fixup
Should be good to go

arnaudstiegler · 2022-03-28T17:57:37Z

@ydshieh Is there anything else to do? I don't have writing access here, so I can't merge this

ydshieh

Hi, @arnaudstiegler,

I have left a few more comments: most of them are just some nits.
This PR is really ready, and I will merge once you can apply the suggestions 💯
Thank you so much!

src/transformers/models/trocr/modeling_trocr.py

ydshieh · 2022-03-28T19:22:48Z

src/transformers/models/trocr/modeling_trocr.py

just a nit: pixel_values before text 🙂

The reason is: pixel_values is the encoder part, and text is the decoder part. In encoder-decoder architecture, encoder is run before the decoder.

ydshieh · 2022-03-28T19:24:55Z

src/transformers/models/trocr/modeling_trocr.py

Would be great if you could add the expected loss here ❤️

Added the loss. I actually changed the text to reflect the actual text on the image because the loss was super high with the dummy text (loss=22.00 vs loss=4.00). Let me know what's best

Also added rounding + setting the torch seed just in case

Very nice for the change of target text! Awesome 👍

src/transformers/models/trocr/modeling_trocr.py

ydshieh · 2022-03-28T19:34:38Z

src/transformers/models/trocr/modeling_trocr.py

arnaudstiegler · 2022-03-29T01:43:26Z

Rebased on latest main and addressed the comments, the failing test is coming from main (failing on main as well)

ydshieh · 2022-03-29T01:44:13Z

Hi, the failed check seems irrelevant to your PR. No need to fix it here 😃. I will give a final look and merge. Thank you 💗

arnaudstiegler · 2022-03-29T01:44:40Z

Alright, thank you!

ydshieh · 2022-03-29T01:47:39Z

By the way, may I wonder why you choose to display 3 decimal numbers for the loss value? I remember we use 2 decimal numbers in doc.py. I will check once I am available.

ydshieh

Just added 3 final suggestions

No (real) need to set the seed
display only 2 decimal numbers

This is more about to be aligned with the code sample in doc.py.

src/transformers/models/trocr/modeling_trocr.py

arnaudstiegler · 2022-03-29T13:22:40Z

Applied your changes, tested them locally, and ran make fixup as this was needed after the changes

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>

ydshieh · 2022-03-29T13:50:58Z

If the remaining failed checks are build_pr_documentation and Add new model like template tests, you can keep the PR as it is. It is irrelevant I think.

arnaudstiegler · 2022-03-29T13:54:02Z

Ok, I tried rebasing on latest master and it doesn't seem to be doing the trick. Not sure what's causing that unfortunately, but unlikely to be due to the code changes I've done :)

ydshieh · 2022-03-29T14:19:11Z

Thank you again @arnaudstiegler ! (also for your patience)

I merge this PR now ❤️.

arnaudstiegler marked this pull request as draft March 24, 2022 22:34

arnaudstiegler marked this pull request as ready for review March 24, 2022 22:36

ydshieh approved these changes Mar 25, 2022

View reviewed changes

ydshieh reviewed Mar 28, 2022

View reviewed changes

src/transformers/models/trocr/modeling_trocr.py Outdated

Copy link

Collaborator

ydshieh Mar 28, 2022

Choose a reason for hiding this comment

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

very nice!

arnaudstiegler force-pushed the AS/TrOCR branch from 9466717 to 86c9c1c Compare March 28, 2022 22:17

ydshieh reviewed Mar 29, 2022

View reviewed changes

src/transformers/models/trocr/modeling_trocr.py Outdated Show resolved Hide resolved

src/transformers/models/trocr/modeling_trocr.py Outdated Show resolved Hide resolved

src/transformers/models/trocr/modeling_trocr.py Outdated Show resolved Hide resolved

arnaudstiegler added 10 commits March 29, 2022 09:43

docstring still WIP | adding to documentation_tests

68f1241

clean version | passes tests

adb8c8a

adding to documentation_test

7f0b056

adding forward for training pass

3bcc3aa

make fixup applied

5a8ae60

address comments

12f8e69

fix doctest

2b9e1f1

apply make fixup

c8ace4f

remove additional blank

1689352

fix file to have correct split for prepare_for_doc_test

c04e5f0

arnaudstiegler and others added 8 commits March 29, 2022 09:43

Update src/transformers/models/trocr/modeling_trocr.py

a82d053

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>

address comments

7300db5

changing text | adding loss check | make fixup

b6272b5

make fixup

bf97f0f

Update src/transformers/models/trocr/modeling_trocr.py

ea352ab

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>

Update src/transformers/models/trocr/modeling_trocr.py

a4623cc

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>

Update src/transformers/models/trocr/modeling_trocr.py

b6bab15

Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com>

make fixup

3f6859d

arnaudstiegler force-pushed the AS/TrOCR branch from 3d7134d to 3f6859d Compare March 29, 2022 13:43

ydshieh merged commit ed31ab3 into huggingface:main Mar 29, 2022

Conversation

arnaudstiegler commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Adding TrOCR to DocTests

Uh oh!

HuggingFaceDocBuilderDev commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydshieh commented Mar 24, 2022

Uh oh!

ydshieh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaudstiegler commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydshieh commented Mar 25, 2022

Uh oh!

arnaudstiegler commented Mar 25, 2022

Uh oh!

ydshieh commented Mar 25, 2022

Uh oh!

arnaudstiegler commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnaudstiegler commented Mar 28, 2022

Uh oh!

ydshieh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaudstiegler Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaudstiegler commented Mar 29, 2022

Uh oh!

ydshieh commented Mar 29, 2022

Uh oh!

arnaudstiegler commented Mar 29, 2022

Uh oh!

ydshieh commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydshieh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

arnaudstiegler commented Mar 24, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Mar 24, 2022 •

edited

Loading

arnaudstiegler commented Mar 25, 2022 •

edited

Loading

arnaudstiegler commented Mar 25, 2022 •

edited

Loading

ydshieh left a comment •

edited

Loading

arnaudstiegler Mar 29, 2022 •

edited

Loading

ydshieh commented Mar 29, 2022 •

edited

Loading

ydshieh left a comment •

edited

Loading