Generate: move prepare_inputs_for_generation in encoder-decoder llms
#34048
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@zucchini-nlp this PR may have a conflict with your encoder-decoder+compile PR 👀 |
zucchini-nlp
approved these changes
Oct 10, 2024
| "past_key_values": past_key_values, | ||
| "encoder_hidden_states": model_kwargs.get("encoder_hidden_states", None), | ||
| "encoder_attention_mask": model_kwargs.get("encoder_attention_mask", None), | ||
| "is_decoder": True, |
There was a problem hiding this comment.
is it okay we're losing this? Seems like BLIP was forcefully passing this kwarg for later setting the cache?
O think we don't have tests for BlipText, neither for VLM part so we can't rely on tests for BLIP 😭 (I'll work on it soon, rn I'm working on Idefics models and BLIP will be next)
There was a problem hiding this comment.
uhmm perhaps -- is_decoder=True is the default everywhere (in forward, in the config), but the user could force it to False. Going to revert
(I suspect this class is never used with is_decoder=True, but too late to fix that :D )
There was a problem hiding this comment.
yeah, blip is a difficult case, better keep it overriden hehe
ArthurZucker
approved these changes
Oct 10, 2024
gante
force-pushed
the
encoder_decoder_prepare
branch
2 times, most recently
from
ca46d3b to
40d6c34
Compare
gante
force-pushed
the
encoder_decoder_prepare
branch
from
40d6c34 to
369b614
Compare
|
Ran the following slow tests before merging:
|
8 tasks
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
Oct 14, 2024
Don't assume that past_key_values is part of the model_kwargs. This fix is similar to huggingface#2140 but for encoder-decoder models. It became necessary after huggingface/transformers#34048 was merged into transformers.
BenjaminBossan
added a commit
to huggingface/peft
that referenced
this pull request
Oct 14, 2024
Don't assume that past_key_values is part of the model_kwargs. This fix is similar to #2140 but for encoder-decoder models. It became necessary after huggingface/transformers#34048 was merged into transformers.
yaswanth19
pushed a commit
to yaswanth19/peft
that referenced
this pull request
Oct 20, 2024
Don't assume that past_key_values is part of the model_kwargs. This fix is similar to huggingface#2140 but for encoder-decoder models. It became necessary after huggingface/transformers#34048 was merged into transformers.
yaswanth19
pushed a commit
to yaswanth19/peft
that referenced
this pull request
Oct 20, 2024
Don't assume that past_key_values is part of the model_kwargs. This fix is similar to huggingface#2140 but for encoder-decoder models. It became necessary after huggingface/transformers#34048 was merged into transformers.
BenjaminBossan
added a commit
to BenjaminBossan/peft
that referenced
this pull request
Oct 22, 2024
Don't assume that past_key_values is part of the model_kwargs. This fix is similar to huggingface#2140 but for encoder-decoder models. It became necessary after huggingface/transformers#34048 was merged into transformers.
SabaPivot
reviewed
Nov 22, 2024
Comment on lines
+3844
to
+3875
| def test_prepare_inputs_for_generation_encoder_decoder_llm(self): | ||
| """ | ||
| Same as `test_prepare_inputs_for_generation_decoder_llm` but for encoder-decoder models. Main difference: we | ||
| should look for `decoder_input_ids`, instead of `input_ids`. | ||
| """ | ||
| model = AutoModelForSeq2SeqLM.from_pretrained("hf-internal-testing/tiny-random-t5") | ||
| model = model.to(torch_device) | ||
|
|
||
| # 1. Sanity check: the model's `prepare_inputs_for_generation` comes from `GenerationMixin` | ||
| self.assertTrue("GenerationMixin" in str(model.prepare_inputs_for_generation)) | ||
|
|
||
| # 2. If we pass input ids by themselves, we should get back the same input ids -- with the encoder-decoder key | ||
| decoder_input_ids = torch.tensor([[1, 2, 3], [4, 5, 6]]).to(torch_device) | ||
| model_inputs = model.prepare_inputs_for_generation(decoder_input_ids) | ||
| self.assertTrue(torch.all(model_inputs["decoder_input_ids"] == decoder_input_ids)) | ||
|
|
||
| # 3. If we pass the attention mask too, we will get back the attention mask. Encoder-decoder models usually | ||
| # don't use `position_ids` | ||
| decoder_attention_mask = torch.tensor([[1, 1, 1], [1, 1, 1]]).to(torch_device) | ||
| model_inputs = model.prepare_inputs_for_generation( | ||
| decoder_input_ids, decoder_attention_mask=decoder_attention_mask | ||
| ) | ||
| self.assertTrue(torch.all(model_inputs["decoder_attention_mask"] == decoder_attention_mask)) | ||
| self.assertTrue("position_ids" not in model_inputs) | ||
|
|
||
| # 4. `use_cache` (and other kwargs, like the encoder outputs) are forwarded | ||
| self.assertFalse("use_cache" in model_inputs) # From the previous input, there is no `use_cache` | ||
| model_inputs = model.prepare_inputs_for_generation(decoder_input_ids, use_cache=True, encoder_outputs="foo") | ||
| self.assertTrue(model_inputs["use_cache"] is True) | ||
| self.assertTrue(model_inputs["encoder_outputs"] == "foo") | ||
| # See the decoder-only test for more corner cases. The code is the same, so we don't repeat it here. | ||
|
|
There was a problem hiding this comment.
Should I add this to my
AutoAdapterModel
to generate in adapters using T5?
There was a problem hiding this comment.
If you mean the tests, you should not need to add it anywhere as it is ran only to test the correctness of new modifications.
In general it is advised to post question in the forum if it is not a bug or feature request
BernardZach
pushed a commit
to BernardZach/transformers
that referenced
this pull request
Dec 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Part of step 6 in #32685
Follow-up to #33870
This PR:
GenerationMixin.prepare_inputs_for_generationto usedecoder_input_idsin encoder-decoder modelsprepare_inputs_for_generationin encoder-decoder llms 🔪 😎