OPT-fix

[younesbelkada]

What does this PR do?

Quicklly fxing 3 testing issues!

cc @patrickvonplaten @ydshieh

[patrickvonplaten]

Ok let's merge this one first and then I'll rebase mine here: #17228

[patrickvonplaten]

Actually can you revert this one? I'll correct the generation test here: #17228

[patrickvonplaten]

@younesbelkada can you also replace:

 tokenizer = GPT2Tokenizer.from_pretrained("patrickvonplaten/opt_gpt2_tokenizer")

in the embedding test with

 tokenizer = GPT2Tokenizer.from_pretrained(self.path_model)

[HuggingFaceDocBuilderDev]

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

[ydshieh]

Hi, just a nit:

This line under OPTGenerationTest

model = OPTForCausalLM.from_pretrained(self.path_model)

has self.path_model undefined.

[ydshieh]

And there are still 2 patrickvonplaten/opt_gpt2_tokenizer in the current version, but probably these are intended? I will leave this part for you ang Patrick.

[ydshieh]

I don't understand very well why it is expected to be 512.

Fomr config, I see

 "hidden_size": 1024,
 "word_embed_proj_dim": 512,

So any special reason for last_hidden_state to be 512? It looks unusual if it is intended to be so - therefore a comment would be very helpful in this casee.

[younesbelkada]

If my understanding is correct, for some particular cases in OPT - the word embeddings have a different size than the dimension of the internal hidden states. That is why in the config file you have the word_embed_proj_dim arg.
So basically the hidden states that are computed here including the last hidden states will have a dimension of word_embed_proj_dim. The conversion from hidden_size to word_embed_proj_dim is ensured by the self.project_out and self.project_in modules. Basically this happens only when hidden_size is different than word_embed_proj_dim which is the case in OPT-350m.

[ydshieh]

Thank you @younesbelkada , I understand very well now 😄. I still think it would be a very good idea to add a (short) comment about what you mentioned above.

Regarding this, I personally think it would be much better to have something like:

@dataclass
class OPTModelOutput(ModelOutput):

    last_hidden_state  = None
    projected_word_embedding = None  # or whatever better name
   ... (remaining fields )

and use this for OPTDecoder/OPTModel.

I will leave @patrickvonplaten and @LysandreJik to judge though, they have much more experience than me.

[patrickvonplaten]

@younesbelkada is right here I think :-) But always good to double check!

[ydshieh]

Well, I still expect a comment to be included in the code 😢