Fix kwargs handling in generate_with_fallback

[cifkao]

What does this PR do?

Fixes #29312.

1. changes the pop() to get() to avoid modifying kwargs between loop iterations,
2. makes sure a copy of kwargs is made as the first step in generate_with_fallback() to prevent any changes to it from propagating outside the method call.
3. makes sure the keys that were assigned to generation_config are removed from the keyword arguments to super().generate() (to avoid overriding the former), but this is done in a copy of kwargs that is not reused between iterations.

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline, Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the documentation guidelines, and here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

@patrickvonplaten @sanchit-gandhi @ylacombe

[ylacombe]

Hey @cifkao, thanks for the great work here, it's a nice catch.

The fix seems okay to me, I don't think we have a way to test if it does work, otherwise I'd have ask you that!

@sanchit-gandhi could we have your review here as well ?

[ylacombe]

temperature shouldn't be in kwargs as it's already an argument of .generate here right ?

It seems okay to check for do_sample and num_beams here

[cifkao]

Just wanted to be extra cautious here and make sure everything is safe locally, rather than relying on what gets passed down from 2 call frames up the stack. But I can remove temperature if you prefer.

[sanchit-gandhi]

Looks good to me as is - there's a preference for more explicit handling of kwargs than more buried ones

[ylacombe]

Why do you use dict(...) here and below? Is it to copy ? If yes, shouldn't we use copy.deepcopy instead ?

[cifkao]

Yes, it's just to make a copy. My thinking here was that a shallow copy (using dict() or copy.copy()) has the same effect as using the **kwargs syntax.

[ylacombe]

copy.deepcopy should make the trick then right ? I'm just afraid that using dict might no be self-explanatory

[cifkao]

I still don't think we want to make a deep copy (what if kwargs contains a large object like assistant_model, for example?). So I changed the dict to copy.copy, which is equivalent and more readable.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[ArthurZucker]

Thanks @cifkao! Sounds good, just want to make sure you have a reproducer!

[sanchit-gandhi]

Thanks for the great issue and super clear PR description @cifkao! The PR looks good to me. My only request is that we add a test to confirm beam search is working as expected. Could we modify your reproducer to do this, possibly with something like the following?

import datasets
from transformers import AutoProcessor, GenerationMixin, WhisperForConditionalGeneration
import numpy as np

processor = AutoProcessor.from_pretrained("openai/whisper-tiny")
model = WhisperForConditionalGeneration.from_pretrained("openai/whisper-tiny")

orig_generate = GenerationMixin.generate
NUM_BEAMS = 2

def generate(self, *args, **kwargs):
    assert args[1].num_beams == NUM_BEAMS
    return orig_generate(self, *args, **kwargs)


GenerationMixin.generate = generate

ds = datasets.load_dataset(
    "google/fleurs", "en_us", split="test", trust_remote_code=True
)
ds = ds = ds.cast_column("audio", datasets.Audio(sampling_rate=16000))
raw_audio = np.concatenate([x["array"].astype(np.float32) for x in ds[:16]["audio"]])

inputs = processor(
    [raw_audio],
    return_tensors="pt",
    truncation=False,
    padding="longest",
    return_attention_mask=True,
    sampling_rate=16_000,
)

model.generate(
    **inputs,
    num_beams=NUM_BEAMS,
    task="transcribe",
    language="en",
)

[cifkao]

@sanchit-gandhi Test added!

On main:

FAILED tests/models/whisper/test_modeling_whisper.py::WhisperModelIntegrationTests::test_whisper_longform_multi_batch_beam - assert 1 == 2

After fix:

PASSED tests/models/whisper/test_modeling_whisper.py::WhisperModelIntegrationTests::test_whisper_longform_multi_batch_beam

[ArthurZucker]

Thanks for iterating and adding a test!