-
Notifications
You must be signed in to change notification settings - Fork 27.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace as_target
context managers by direct calls
#18325
Conversation
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the audio processors and it looks very nice to me! Thanks for doing the refactor here. Like the simple naming of "audio"
and "text"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your efforts, this looks good!
... batch = self.processor.pad( | ||
... input_features, | ||
... padding=self.padding, | ||
... return_tensors="pt", | ||
... ) | ||
... with self.processor.as_target_processor(): | ||
... labels_batch = self.processor.pad( | ||
... label_features, | ||
... padding=self.padding, | ||
... return_tensors="pt", | ||
... ) | ||
... batch = self.processor.pad(input_features, padding=self.padding, return_tensors="pt") | ||
|
||
... labels_batch = self.processor.pad(labels=label_features, padding=self.padding, return_tensors="pt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a clean API :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for doing this refactor - the result looks 🤩
Only one comment where I think the call to the tokenizer might want to be different.
Co-authored-by: amyeroberts <amy@huggingface.co>
* Preliminary work on tokenizers * Quality + fix tests * Treat processors * Fix pad * Remove all uses of in tests, docs and examples * Replace all as_target_tokenizer * Fix tests * Fix quality * Update examples/flax/image-captioning/run_image_captioning_flax.py Co-authored-by: amyeroberts <amy@huggingface.co> * Style Co-authored-by: amyeroberts <amy@huggingface.co>
What does this PR do?
This PR deprecates the context managers
as_target_tokenizer
andas_target_processor
to the profit of passing more arguments to the__call__
method (or thepad
method for certain processors).Let's look at one example for a tokenizer in a seq2seq task. The current workflow is:
After this PR, this simply becomes:
which is more natural and way easier.
It gets tricky if:
In this case you still need to do two calls:
Like before, if you forget to indicate to the tokenizer you are tokenizing labels (here by passing them as
text_target=...
(and before by tokenizing under the context manager), the labels will be tokenized like the inputs.For processors, the same changes are done, except you can directly use modality names:
can now simply be:
Like before, you can also do it in two individual calls (with
audio
andtext
) to get the objects if you need to use different values of keyword arguments, or want to do a more complex merge than just taking the label input IDs.Padding is also treated: previous code required to do something like this:
This can now be done with:
or in two calls like before if something more involved (different keyword arguments for labels or accessing more than the labels input IDs) is needed.
This comes at no breaking change.
Current version does not touch any of the documentation, examples and tests (to double-check there is no breaking change), those will need to be adapted. This can be done in this PR or in followups if you prefer to read lighter diffs.