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.

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

QuestionAnsweringPipeline cannot handle impossible answer #14277

Closed
julian-risch opened this issue Nov 4, 2021 · 18 comments
Closed

QuestionAnsweringPipeline cannot handle impossible answer #14277

julian-risch opened this issue Nov 4, 2021 · 18 comments

Comments

@julian-risch
Copy link

Environment info

  • transformers version: latest master. I think the bug was introduced by this PR: Fixing question-answering with long contexts  #13873 so it's part of transformers since the 4.11.3 release and I can confirm that I didn't see this bug with the 4.11.2 release.
  • Platform: linux
  • Python version: 3.8
  • PyTorch version (GPU?): 1.9 (same with 1.10)
  • Using GPU in script?: yes
  • Using distributed or parallel set-up in script?:

Who can help

Hi @Narsil I hope you could look again at #13873 and check the changes it makes for the case when handle_impossible_answer is True. Thanks a lot!

To reproduce

Steps to reproduce the behavior:

  1. Find the run_pipeline_test test in test_pipelines_question_answering.py
  2. Set handle_impossible_answer to Truein the question_answerer so that the code is the following:
    def run_pipeline_test(self, question_answerer, _):
        outputs = question_answerer(
            question="Where was HuggingFace founded ?", context="HuggingFace was founded in Paris.", handle_impossible_answer=True
        )
        self.assertEqual(outputs, {"answer": ANY(str), "start": ANY(int), "end": ANY(int), "score": ANY(float)})

        outputs = question_answerer(
            question=["In what field is HuggingFace working ?", "In what field is HuggingFace working ?"],
            context="HuggingFace was founded in Paris.",
        )
  1. When running this modified test, it fails with a ValueError:
            # Normalize logits and spans to retrieve the answer
            start_ = np.exp(start_ - np.log(np.sum(np.exp(start_), axis=-1, keepdims=True)))
            end_ = np.exp(end_ - np.log(np.sum(np.exp(end_), axis=-1, keepdims=True)))

            if handle_impossible_answer:
>               min_null_score = min(min_null_score, (start_[0] * end_[0]).item())
E               ValueError: can only convert an array of size 1 to a Python scalar

../src/transformers/pipelines/question_answering.py:415: ValueError

Expected behavior

Test should run through.

Additional Info

I came across this problem when upgrading the transformers dependency of haystack and ran our tests with different versions of transformers to find the last working release/first failing release: deepset-ai/haystack#1659

@Narsil
Copy link
Contributor

Narsil commented Nov 4, 2021

Thanks for the detail issue, very easy to reproduce, and everything is correct.

I am creating a PR to fix this, however do you have an example where this argument is needed in an obvious way ? I would love to add a meaningful test for this option (already added unit test for it)

@julian-risch
Copy link
Author

I think you could copy the run_pipeline_test test and change the copy in a way such that the context does not contain the answer to the question. In that case you could set handle_impossible_answer=True and the expected result of the test is that the model does not return a prediction because we know that any predicted answer string would be wrong.

@Narsil
Copy link
Contributor

Narsil commented Nov 9, 2021

The fast tests use random networks, so we don't really have any way to control that.

Do you have a specific example with a given model that would display the desired behavior ? That will be included in slow tests.

@julian-risch
Copy link
Author

Unfortunately, I don't have any specific example no. What I had in mind is something like:

def run_pipeline_test_no_answer(self, question_answerer, _):
        outputs = question_answerer(
            question="Where was deepset founded ?", context="HuggingFace was founded in Paris.", handle_impossible_answer=True
        )

@julian-risch
Copy link
Author

In this old issue there is another example: #5563

@Narsil
Copy link
Contributor

Narsil commented Nov 9, 2021

None of the examples in the other issue you mentioned yield an error now, so I am unclear what the problem is.

TBH, I am not really sure what handle_impossible_answer is supposed to do, as there's a harcoded index (0) that is supposed to be [CLS] (looking at the comments and related PRs).

I am not sure all models even possess a CLS token.

It seems this was added to prevent indexing errors, but I don't think that's what you expect from this argument am I correct ?

@julian-risch
Copy link
Author

handle_impossible_answer should add an answer with an empty string to the list of predicted answers for every questions. Without that setting, the model will always return some answer even if it doesn't make sense at all.
As a result of the added empty answer, this empty answer or "no_answer" is ranked together with the other predictions and could even end up being the top-ranked answer. To allow ranking it together with the other predictions, it has a min_null_score. The calculation of that score is currently broken, I think.
A "no_answer" is typically annotated as having start and end index equal to 0. So in start_ and end_ we would need to look for the probability mass that has not been assigned to any other possible answer. I think we can find that probability mass at start_[0, 0] and send_[0, 0] so that looks good to me and it was just the batch size missing.

@julian-risch
Copy link
Author

Thank you for your help. Looking forward to the next release. 👍

@Narsil
Copy link
Contributor

Narsil commented Nov 10, 2021

Closing this, feel free to reopen.

@Narsil Narsil closed this as completed Nov 10, 2021
@CyrilShch
Copy link

Was that issue fixed? Because I still get the same error

@Narsil
Copy link
Contributor

Narsil commented Nov 24, 2021

@CyrilShch are you using master ? A release is coming this week which could help.

@CyrilShch
Copy link

@Narsil Yes, I'm using master. Thanks! Looking forward!

@Narsil
Copy link
Contributor

Narsil commented Nov 24, 2021

Can you provide a reproducible script ?

The one that used to not work:

import os

from transformers import pipeline


pipe = pipeline("question-answering", handle_impossible_answer=True)
out = pipe(question="This", context="that")
print(" - " * 20)
print(out)
print(" - " * 20)

seems to be working fine.

@CyrilShch
Copy link

@Narsil e.g., if you open a new google colab notebook and run the very same example that you provided!

!pip install transformers

import os

from transformers import pipeline


pipe = pipeline("question-answering", handle_impossible_answer=True)
out = pipe(question="This", context="that")
print(" - " * 20)
print(out)
print(" - " * 20)

You'll get this error:
image

And locally it seems to work for me as well when I fork the repository and try to change it.

@Narsil
Copy link
Contributor

Narsil commented Nov 24, 2021

Hi, this is not master you're installing but the latest release (which does not contain the fix yet).

Can you try

!pip install git+https://github.com/huggingface/transformers.git@master#egg=transformers

A new release should happen this week which will contain the fix !

@CyrilShch
Copy link

@Narsil Ops, my bad. Works fine now! Thanks a lot :) I guess the issue can be closed 👍

@Narsil
Copy link
Contributor

Narsil commented Nov 24, 2021

It is already closed so it's ok but thanks for the confirmation .

Cheers !

@shaked571
Copy link

Perfect, just found the bug myself and saw this fix.
Super cool! thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants