Adding support for multiple mask tokens. #14716
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Narsil
force-pushed
the
fill_mask_multi_mask
branch
from
a3fca5e
to
34c02f4
Compare
Narsil
force-pushed
the
fill_mask_multi_mask
branch
2 times, most recently
from
8068d62
to
61686e2
Compare
- Original implem: huggingface#10222 Co-authored-by: njafer <naveen.jafer@oracle.com>
we add information to the tasks to specify tasks where we know for sure if we need the tokenizer/feature_extractor or not.
Narsil
force-pushed
the
fill_mask_multi_mask
branch
from
61686e2
to
5b0189e
Compare
LysandreJik
reviewed
Dec 14, 2021
Comment on lines
+271
to
+279
| NO_FEATURE_EXTRACTOR_TASKS = set() | ||
| NO_TOKENIZER_TASKS = set() | ||
| for task, values in SUPPORTED_TASKS.items(): | ||
| if values["type"] == "text": | ||
| NO_FEATURE_EXTRACTOR_TASKS.add(task) | ||
| elif values["type"] in {"audio", "image"}: | ||
| NO_TOKENIZER_TASKS.add(task) | ||
| elif values["type"] != "multimodal": | ||
| raise ValueError(f"SUPPORTED_TASK {task} contains invalid type {values['type']}") |
There was a problem hiding this comment.
I like this approach, it should make the pipelines more robust to models with different capabilities in terms of preprocessors.
There was a problem hiding this comment.
Looks good to me as well!
NielsRogge
reviewed
Dec 14, 2021
| @@ -340,3 +338,27 @@ def fill_mask_with_duplicate_targets_and_top_k(self, model, tokenizer): | |||
| # The target list contains duplicates, so we can't output more | |||
| # than them | |||
| self.assertEqual(len(outputs), 3) | |||
|
|
|||
| def fill_mask_with_multiple_masks(self, model, tokenizer): | |||
There was a problem hiding this comment.
Can we perhaps add a test for Perceiver (similar to the image classification models)?
Or is this not required here?
There was a problem hiding this comment.
I think this PR is pretty orthogonal to Perceiver.
We could add a slow test for sure, but it doesn't have to be Perceiver specific.
In fact, I'll add something on the random model (it just needs to be consistent, actual values are less important)
LysandreJik
approved these changes
Dec 14, 2021
Albertobegue
pushed a commit
to Albertobegue/transformers
that referenced
this pull request
Jan 27, 2022
* Adding support for multiple mask tokens. - Original implem: huggingface#10222 Co-authored-by: njafer <naveen.jafer@oracle.com> * In order to accomodate optionally multimodal models like Perceiver we add information to the tasks to specify tasks where we know for sure if we need the tokenizer/feature_extractor or not. * Adding info in the documentation about multi masks. + marked as experimental. * Add a copy() to prevent overriding the same tensor over and over. * Fixup. * Adding small test for multi mask with real values.. Co-authored-by: njafer <naveen.jafer@oracle.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
When presented multiple masks, it's impossible to retrieve the conjugate probabilities.
Instead of trying to workaround that (see discussions in previous PR) this PR instead
just outputs the raw
top_kproposition at each locus, since it gets trickier to find a goodproxy for "joint probabilities". Instead of trying to solve this impossible problem we simply
show exactly what the model outputs.
@naveenjafer mentionned as co-author since much of this PR was pulled from there.
This PR was resurrected partly because Perceiver (byte-level model) need to do this type of masking to be useful.
Fixes # (issue)
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
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.