Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes #17128 . #17356

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Fixes #17128 . #17356

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions src/transformers/pipelines/question_answering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,6 @@ def preprocess(self, example, padding="do_not_pad", doc_stride=None, max_questio
truncation="only_second" if question_first else "only_first",
max_length=max_seq_len,
stride=doc_stride,
return_tensors="np",
return_token_type_ids=True,
return_overflowing_tokens=True,
return_offsets_mapping=True,
Expand All @@ -294,12 +293,10 @@ def preprocess(self, example, padding="do_not_pad", doc_stride=None, max_questio

# p_mask: mask with 1 for token than cannot be in the answer (0 for token which can be in an answer)
# We put 0 on the tokens from the context and 1 everywhere else (question and special tokens)
p_mask = np.asarray(
[
[tok != 1 if question_first else 0 for tok in encoded_inputs.sequence_ids(span_id)]
for span_id in range(num_spans)
]
)
p_mask = [
[tok != 1 if question_first else 0 for tok in encoded_inputs.sequence_ids(span_id)]
for span_id in range(num_spans)
]

features = []
for span_idx in range(num_spans):
Expand All @@ -316,8 +313,6 @@ def preprocess(self, example, padding="do_not_pad", doc_stride=None, max_questio
for cls_index in cls_indices:
p_mask[span_idx][cls_index] = 0
submask = p_mask[span_idx]
if isinstance(submask, np.ndarray):
submask = submask.tolist()
features.append(
SquadFeatures(
input_ids=input_ids_span_idx,
Expand All @@ -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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LysandreJik are you ok with merging this ?


for k, v in feature.__dict__.items():
if k in model_input_names:
Expand Down