Fixes #17128 . #17356
Fixes #17128 . #17356
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
| @@ -344,7 +339,7 @@ def preprocess(self, example, padding="do_not_pad", doc_stride=None, max_questio | |||
| for i, feature in enumerate(features): | |||
| fw_args = {} | |||
| others = {} | |||
| model_input_names = self.tokenizer.model_input_names + ["p_mask"] | |||
| model_input_names = self.tokenizer.model_input_names + ["p_mask", "token_type_ids"] | |||
There was a problem hiding this comment.
nitpick, but if the tokenizer already has token_type_ids in model_input_names (for example BERT), then token_type_ids will be here twice. Why is this line necessary compared to before?
There was a problem hiding this comment.
There are two ways to pass arguments to the _forward function, and the autobatcher will act differently if it's a Tensor or a list. (lists are unbatched, just lists of lists) while tensors get batched (with padded value and direction).
The problem occurs here becaus token_type_ids are used in the postprocessing (like p_mask) so they need to be correctly passed as tensors. So the hint self.tokenizer.model_input_names is not enough and this line ensures that the postprocessing will see a tensor aligned with the others.
If token_type_idsis listed twice, then maybe we can just (lmodel_input_names = set(model_input_names) ? (Actually the fw_args and others are already dictionnary so I think there's only 1 level of duplication.
There was a problem hiding this comment.
Yes that's one solution! Anyway it's a nitpick given that it won't change much. We're using this list to check if the name of a variable is in it, so if it's in it twice, it'll result in the same!
There was a problem hiding this comment.
Hi both. I got notification that this issue has been marked as stale. If there's anything to do be done, please let me know. If you both require more time to reach a conclusion, that's fine by me.
Thanks,
There was a problem hiding this comment.
@LysandreJik are you ok with merging this ?
|
Thank you for your contribution, @mygithubid1! |
VisibleDeprecationWarning is addressed by specifying dtype=object when creating numpy array. Update code based on review feedback. Undo whitespace changes to tokenization_utils_base.py. Co-authored-by: I like data <[email protected]>
VisibleDeprecationWarning is addressed by specifying dtype=object when creating numpy array. Update code based on review feedback. Undo whitespace changes to tokenization_utils_base.py. Co-authored-by: I like data <[email protected]>
What does this PR do?
Finalize changes as per this PR.
Sorry. I messed up changes during merge and hence this new PR.
Fixes #17128
Before submitting
Pull Request section?
RUN_PIPELINE_TESTS=yes python -m unittest discover -s tests/pipelines -p "test_pipelines_question_answering.py" -t . -v -fpython -m unittest discover -s . -p "test_tokenization_wav2vec2.py" -t . -v -fWho 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.
@LysandreJik
@Narsil