Add GOT-OCR 2.0 to Transformers #34721
Conversation
yonigozlan
force-pushed
the
add-got-ocr2
branch
from
93b1d19 to
af8035d
Compare
yonigozlan
force-pushed
the
add-got-ocr2
branch
from
f8e1ac9 to
4007fb2
Compare
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
docs/source/en/model_doc/qwen2_vl.md
Outdated
There was a problem hiding this comment.
interesting this formatting was missing?
There was a problem hiding this comment.
Yes I guess my format-on-save did that 😅
| write_tokenizer( | ||
| tokenizer_path="qwen.tiktoken", | ||
| save_dir=args.output_dir, | ||
| instruct=args.instruct, |
There was a problem hiding this comment.
Does this model have an instruct version needed?
There was a problem hiding this comment.
Oh I forgot to remove that, thanks. I'd say the model is indeed instruct, but users are not really expected to have conversation with it, and the possible prompt are explicitly defined in the processor so I'm not sure in which categories this model falls and if we should add support for a chat template here.
I'd say it might make sense to add a chat template when and if we end up adding support for fine-tuning, but to me it doesn't make much sense in this state, I might be wrong though.
What do you think @molbap @Ucas-HaoranWei ?
There was a problem hiding this comment.
From afar, maybe @Ucas-HaoranWei has a different opinion, I'd just remove the instruct - as you say users are not expected to converse with it. If it's instruct-tuned in a later version, it might make sense, but it's an OCR model as it stands, it sounds confusing to label it as instruct.
There was a problem hiding this comment.
Yes, I agree that there should not be a chat template, as they are fixed prompts in this state. @molbap @yonigozlan. If the later versions can be fine-tuned, then the template can be maintained to ensure that users use their new prompts.
There was a problem hiding this comment.
Sounds good thank you!
| resized_image = image.resize((target_width, target_height)) | ||
|
|
||
| # split the image into patches | ||
| processed_images = [] |
There was a problem hiding this comment.
these are processed_patches, rather, yes?
There was a problem hiding this comment.
Yes indeed, but the reason I did not rename it that is because we add to it a "thumbnail" image which is the whole image resized
| format = output_kwargs["text_kwargs"].pop("format", False) | ||
| num_image_tokens = output_kwargs["images_kwargs"].pop("num_image_tokens", 256) | ||
| box = output_kwargs["images_kwargs"].pop("box", [None]) | ||
| color = output_kwargs["images_kwargs"].pop("color", None) | ||
| multi_page = output_kwargs["images_kwargs"].pop("multi_page", False) | ||
| crop_to_patches = output_kwargs["images_kwargs"].pop("crop_to_patches", False) |
There was a problem hiding this comment.
here we could use the default values that are preset above
There was a problem hiding this comment.
Do you mean something like GotOcr2ProcessorKwargs._defaults["images_kwargs"].get("crop_to_patches")?
There was a problem hiding this comment.
yes, for instance! what I mean is the default values in the pop method should not be specified in two places, because if they change, it's more likely to cause errors/mismatches
There was a problem hiding this comment.
That makes sense. I might be missing something, but if we have default kwargs (for multi_page, crop_to_patches, min_patches, max_patches etc.), it seems to me that there shouldn't be any issues with using "pop" without default? if the kwarg is not specified by the user, pop will return the default, and it will return the user-specified value otherwise?
There was a problem hiding this comment.
@molbap just pinging you on this last question
There was a problem hiding this comment.
Ah, you're right - I guess it's just weird to me to see the default values written in two distinct places, does not seem necessary 🤔
| class GotOcr2VisionAdapter(nn.Module): | ||
| def __init__(self, language_hidden_size: int, vision_output_channels: int): | ||
| super().__init__() | ||
| self.conv_up1 = nn.Conv2d( |
There was a problem hiding this comment.
I assume up stands for upsampler like in swin2sr, but a more explicit name would be better
|
Hey @ArthurZucker
|
yonigozlan
force-pushed
the
add-got-ocr2
branch
from
7e88dbe to
df94db3
Compare
|
HI all -- eager to try this model in transformers in the new year |
What does this PR do?
Add GOT-OCR 2.0 to Transformers.
Left TODOs:
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.