Encoder-decoder models: move embedding scale to nn.Module

[zucchini-nlp]

What does this PR do?

This PR moves embedding scale to nn.Module in encoder decoder models, so that users who want to pass in inputs_embeds to the forward will get the same results as if they passed input_ids via model.get_input_embeddings()(input_ids)

The generation from embeds is not supported for these models, that's why we did not see the inconsistency. I think we do not need specific tests and here I can add support for generation from embeds in another PR if needed.

All the tests (+slow) for the changes models are passing on my end

[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.

[gante]

LGTM, thank you for fixing 🙌

There are more models where this inconsistency happens (e.g. MVP, NllbMoe, ...), would you be able to propagate the pattern?

[gante]

@zucchini-nlp also, can you:

1. Check if there is a common test checking that the output of forward with input_ids=input_ids == output of forward with inputs_embeds=model.get_input_embeddings()(input_ids)
2. If there is not, can you add it? 🙏

We'll tag the core maintainer after we ensure there's a test!

[zucchini-nlp]

Hmm, okay, I thought the test for generation with inputs-embeds is enough. There is no such test, yes. If that's needed I can add it and that can trigger handling other models/cases if there are peculiarities :(

[gante]

Sorry for giving you tons of extra work here 😅 The result will be very cool in the long run, though 💪

[zucchini-nlp]

Seems like it's ready for core maintainer's review. Reverted the previous commit with skips and tested that checking signature is working for most cases. In cases where it does not work, model-specific test skipping is kept.

[gante]

Shouldn't this (and most other skips) be caught in the skip conditions of the main test? What's missing?

[zucchini-nlp]

Some models have all the things implemented, but do not use "ipnuts_embeds" any where, even though accept it in forward. I dod not remove unused args like this due to BC but can be cleaned up in another PR if needed

[gante]

IMO -- instead of adding custom test code to handle a silent failure, let's properly handle the failure (if the user passes inputs_embeds to forward, raise a NotImplementedError, which can be caught in the test)

You've opened a Pandora's box of work in this PR, but it will be great in the long run! 🤗

[zucchini-nlp]

Hmm, so in cases when the model actually accepts but not uses inputs_embeds, we should remove "get_input_embeddings()" method? Or raise error inside forward after the input_embeds are passed?

I would like to entirely remove inputs_embeds from the forward argument list, but this leads to my question: if we should have the same list of arguments in all forwards in all models, regardless of usage? And will it break anything?

[amyeroberts]

if we should have the same list of arguments in all forwards in all models

Not necessarily e.g. vision models don't accept input_ids. All models which are grouped together should have a common subset of inputs, which enable a full forward pass of the model. Most important is that the inputs have standardised names e.g. we don't have token_ids for one model, input_ids for another and that their behaviour is consistent e.g. input_embeds "means" the same thing across models.

In the case of text models, I'd say yes, if they accept input_ids, then they should also accept input_embeds, and then throw an error within the forward pass after input_embeds are passes.

[zucchini-nlp]

Okey, thanks for clarifying. I added NotImplemented errors where possible, leaving skip on some of the tests with explanations.

[zucchini-nlp]

Btw, now the "test_inputs_embeds" can also be cleaned-up and all model-specific skips removed. I can do it later in another PR :)

[zucchini-nlp]

Failing tests are passing for me locally, and are not related to the changes

[amyeroberts]

Thanks for fixing this - definitely a behaviour we want.

V. nicely handled and tested - just a few small things to address before merge

[amyeroberts]

Suggested change

print(inputs.keys())

[amyeroberts]

We shouldn't have any print statements in tests

[amyeroberts]

We shouldn't have try/except patterns in tests: either we're deliberately triggering an error or we're not. Raising exceptions is for within code, allowing us to elegantly handle errors at runtime. Instead, models which don't use input_embeds should explicitly skip this test, and the try/except block be removed

[zucchini-nlp]

Hmm I thought we are adding all the NotImplementedErrors to get rid of many copies "skipTests", as discussed with @gante above:

IMO -- instead of adding custom test code to handle a silent failure, let's properly handle the failure (if the user passes inputs_embeds to forward, raise a NotImplementedError, which can be caught in the test)

I can bring back all skips if that's needed for tests-consistency but if we are specifically catching only "NotImplementedErrors" isn't it okay?

[amyeroberts]

Ah, OK, sorry, I missed the bit about handling in the tests.

We should raise NotImplementedError on the model side, but let users handle that however they want, and then explicitly skip in the tests using unittest.skip for the specific models. This avoids accidentally skipping because a different NotImplementedError is raised. It's true we end up with more code, but it's better for tests to be DAMP i.e. very clear and explicit.

[gante]

but it's better for tests to be DAMP

hehe the opposite of what I've been telling @zucchini-nlp in this PR 🙈 My bad :D

[amyeroberts]

Not at all - the test suite is one of the most inconsistent places in our codebase 😬

[zucchini-nlp]

Ah, okay, TIL about the DAMP principle

[amyeroberts]

For the musicgen tests - a fix was pushed to main - rebasing should resolve

[zucchini-nlp]

@amyeroberts done, all the NotImplementedError models are skipped in their modeling_test files and the CI is green

[amyeroberts]

Beautiful - thanks for fixing this tricky issue and making sure it's well tested!