🚨🚨🚨 Uniformize kwargs for TrOCR Processor#34587
Conversation
qubvel
left a comment
qubvel
left a comment
There was a problem hiding this comment.
Hi @tibor-reiss, thanks for working on this! And sorry for the delay, the team was on the offsite. Don't hesitate to ping for review a few times 🤗
There was a problem hiding this comment.
Not sure we need this *args, may lead to silent missing of text if it passed not as a keyword argument
There was a problem hiding this comment.
I'll add, it's acceptable to use this pattern to capture arguments that are absolutely necessary and are not capturable by any other means, and indeed it should typically be after at least images and text - are you trying to capture a specific arg?
There was a problem hiding this comment.
Thanks for the discussion. I don't have any specific arg in mind, it's purely for backwards compatibility. I agree though that best would be to remove *args completely, but that would break BC, because it is used both when calling tokenizer and image_processor. If this is not a concern here, I am happy to remove it :)
Regarding text: from the old code: text = kwargs.pop("text", None). If I am not mistaken, this is the only way to set text inside __call__. The proposed solution keeps this convention, i.e. if someone specifies text as positional, it will still not work. Unfortunately, it's not possible yet to combine variadic number of positional arguments with the keyword-only delimiter, so this is the best I was able to come up with.
There was a problem hiding this comment.
I see, maybe a better tradeoff would be to remove it or to force users to use keyword arguments for everything other than images, I mean use pure * in the signature instead of *args.
There was a problem hiding this comment.
We can also add 🚨 for current PR to indicate it is not fully BC
There was a problem hiding this comment.
I have removed args.
f336291 to
d93ef00
Compare
qubvel
changed the title
qubvel
left a comment
qubvel
left a comment
There was a problem hiding this comment.
@tibor-reiss thanks for working on this and adding tests 🤗
@ArthurZucker please review whenever you have bandwidth, see also a comment below.
| def __call__( | ||
| self, | ||
| images: ImageInput = None, | ||
| *, |
There was a problem hiding this comment.
Comment for @ArthurZucker:
Slight breaking change (*) to force users to use keyword arguments in case previously positional args were used. Otherwise, we might either miss some args or get it assigned to the wrong keyword parameters and getting not a clear error message.
There was a problem hiding this comment.
tiny comment: let's add maybe a #TODO here to keep in mind to deprecate this pattern in this future 🙏
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
thanks for the description! Regarding the breaking change, let's either properly break or deprecate!
| def __call__( | ||
| self, | ||
| images: ImageInput = None, | ||
| *, # TODO deprecate/remove this |
There was a problem hiding this comment.
Up to you, there is no deprecation cycle so this is not very actionnable, neither for us nor for the user!
|
Thanks for working on this @tibor-reiss! |
* Make kwargs uniform for TrOCR * Add tests * Put back current_processor * Remove args * Add todo comment * Code review - breaking change
4 participants
Adds uniformized processors for TrOCR following #31911
Added tests (which also pass in main branch).
@qubvel @molbap