Skip to content

Conversation

@younesbelkada
Copy link
Contributor

What does this PR do?

This PR fixes the doctest transformers.models.blenderbot.modeling_blenderbot.BlenderbotForConditionalGeneration.forward . Link to failing job is here: https://github.com/huggingface/transformers/actions/runs/4002271138/jobs/6869333719

Updating the prediction with the correct result seems to be the correct fix. I am usure whether this was tested before so I cannot compare for now.

One thing that I suspect is that we get different results across different PT versions, but not sure

cc @ydshieh

- add correct expected value
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jan 25, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@younesbelkada
Copy link
Contributor Author

younesbelkada commented Jan 25, 2023

Actually, as discussed with @ydshieh offline, there seems to be a discrepency between BlenderbotTokenizer & BlenderbotTokenizerFast.
The PR #21225 changed the docstring to use AutoTokenizer instead of BlenderbotTokenizer. This lead to loading BlenderbotTokenizerFast. You can reproduce the discrepency with the script below:

from transformers import BlenderbotTokenizer, BlenderbotTokenizerFast, BlenderbotForConditionalGeneration, AutoTokenizer

mname = "facebook/blenderbot-400M-distill"
model = BlenderbotForConditionalGeneration.from_pretrained(mname)

tokenizer = BlenderbotTokenizer.from_pretrained(mname)
tokenizer_fast = BlenderbotTokenizerFast.from_pretrained(mname)

def generate(tokenizer):
    UTTERANCE = "My friends are cool but they eat too many carbs."

    inputs = tokenizer([UTTERANCE], return_tensors="pt")

    NEXT_UTTERANCE = (
        "My friends are cool but they eat too many carbs.</s> <s>That's unfortunate. "
        "Are they trying to lose weight or are they just trying to be healthier?</s> "
        "<s> I'm not sure."
    )
    inputs = tokenizer([NEXT_UTTERANCE], return_tensors="pt")
    next_reply_ids = model.generate(**inputs)
    print("Bot: ", tokenizer.batch_decode(next_reply_ids, skip_special_tokens=True)[0])

generate(tokenizer)
>>> Bot:   That's too bad. Have you tried encouraging them to change their eating habits? 
generate(tokenizer_fast)
>>> Bot:   I see. Well, it's good that they're trying to change their eating habits.

I am not sure if this is a known bug or intended. Maybe the changes I proposed is not the correct fix here

@ydshieh
Copy link
Collaborator

ydshieh commented Jan 25, 2023

Thanks for digging deeper @younesbelkada! Could you check what's the difference between the fast and slow tokenizer from the checkpoint used in this doc example? And compare the difference of inputs = tokenizer([UTTERANCE], return_tensors="pt") between these 2 tokenizers.

Another similar issue (but not related to this one)
#21254

@sgugger
Copy link
Collaborator

sgugger commented Jan 25, 2023

I think it's good as we want to default to fast tokenizers (which is the reason we switched to AutoTokenizer) so the fix is the right one in my opinion.

@ydshieh
Copy link
Collaborator

ydshieh commented Jan 25, 2023

I agree - but just thinking if we should find out what's going wrong and potentially fix the inconsistency between these 2 tokenizers (or something in our codebase).

The fix is good for me, and you can merge @younesbelkada !

@younesbelkada
Copy link
Contributor Author

younesbelkada commented Jan 25, 2023

Thanks everyone!
I will open an issue to describe the bug
EDIT: #21305

@younesbelkada younesbelkada merged commit 015443f into huggingface:main Jan 25, 2023
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

Successfully merging this pull request may close these issues.

4 participants