Uniformize kwargs for Layoutlm (2, 3, X) processors#32180
Uniformize kwargs for Layoutlm (2, 3, X) processors#32180leloykun wants to merge 3 commits intohuggingface:mainfrom
Conversation
zucchini-nlp
left a comment
There was a problem hiding this comment.
Thanks for working on this! I left a few comments, regarding the apply_ocr arg. I got you point, but we need to test it and make sure it's working for different cases.
So can you add the Mixin test to run general tests + write a special test for apply_ocr with different scenarios ?
1fafbe9 to
85b4aec
Compare
There was a problem hiding this comment.
The test group tests_exotic_models may be broken. It's not running any tests at all. And the tests for the layoutlm models can only be ran on this test group (cuz they require pytesseract which is only installed in the docker container for this test).
I've already fixed the bug above but while it was there, it didn't cause any of the tests to fail.
@NielsRogge are the layoutlmv3 models deprecated?
|
Note for reviewer(s): the failing tests are caused by the Jamba model |
.circleci/create_circleci_config.py
Outdated
There was a problem hiding this comment.
Update: I've just fixed it!
This test may have been broken for months now (perhaps up to 22 months). I'm not sure why, but I think it has to do with CircleCI's filtering mechanism. Directly listing out the files to test fixed the issue.
zucchini-nlp
left a comment
There was a problem hiding this comment.
Looks good to me overall! One thing left is to move all kwargs to ModalityKwargs and handle backwards compatibility for that
src/transformers/processing_utils.py
Outdated
There was a problem hiding this comment.
Cool! We can modify this later after everyone agrees on this workaround. Overall looks good to me, except for some nits I left in another PR.
IMO we can get a core maintainer review on this one which is easier to iterate and then use the agreed deprecation format in PR "all processors with extra args"
And ofc we'll need a test here 😄
a7e623f to
8dc5463
Compare
|
@zucchini-nlp @yonigozlan I've just rebased this to main the failing tests are due to Llava-Next: but yah, this PR should now be ready for review |
964be32 to
1ad69a5
Compare
What does this PR do?
tests_exotic_modelswhich totally prevents any test from being runFixes # (issue)
Who can review?
@zucchini-nlp @molbap @NielsRogge