Why is Marian to Torch converter hardcoded for tied vocab ?
Author: GokulNC
Pull request: no

I see the following condition:
https://github.com/huggingface/transformers/blob/16f0b7d72c6d4e122957392c342b074aa2c5c519/src/transformers/models/marian/convert_marian_to_pytorch.py#L462

While training my Marian model, I do not want to tie my source and target embeddings.
How do I convert such a model? (This is a very common thing in NMT)

I see that in `MarianConfig` itself, this is not supported:
https://github.com/huggingface/transformers/blob/16f0b7d72c6d4e122957392c342b074aa2c5c519/src/transformers/models/marian/configuration_marian.py#L46-L49

Can this be considered a **feature request** to make it generic?

---

Also, why is the `hidden-dim` required to be `512` in the converter?
https://github.com/huggingface/transformers/blob/16f0b7d72c6d4e122957392c342b074aa2c5c519/src/transformers/models/marian/convert_marian_to_pytorch.py#L478

What if I train transformer-big models?

Comments:
I understand that this was created only to add support for [baseline models released from Tatoeba Challenge](https://github.com/Helsinki-NLP/Tatoeba-Challenge/tree/master/models).
But it would be great if we can generalize it. Thanks!

cc @patil-suraj 

Hi @sshleifer
Just saw your comment on this thread: https://github.com/marian-nmt/marian-dev/issues/756#issuecomment-724738421 , so probably felt you can help.

Can you please let me know if any thoughts on the above issue? Thanks!

Hi @jorgtied 

Is there anyway we can convert Marian models (to HF) that are trained with `--tied-embeddings-all=false` and `--tied-embeddings-src=false` ?

For Tatoeba challenge models, I see that you are first creating SPMs specific to src and tgt langs, tokenizing the datasets, and finally concatenating the vocabs using `marian-vocab` so that the model can be trained using a shared vocab. Have you tried with different src and tgt vocabs to convert to PyTorch?

Thanks!

No, I haven't tried that yet and I agree that it would be great to also support separate vocabs in conversion. Why hidden-size and dim_emb is hard-coded to 512 I also don't really understand. Let's see if people at HF can help to answer those questions ...

hi @GokulNC , @jorgtied 

> why is the hidden-dim required to be 512 in the converter?

Not sure why it was done this way, but yes we can generalize it.


> I agree that it would be great to also support separate vocabs in conversion.

It should be possible to add this. Are there any officially released checkpoints with separate vocabs?


OK - nice. Can the condition about dimensionality simply be taken away? Or does that impact anything else?
About a release with 2 separate vocabs: We could use this one as a test case (English-Korean):
https://object.pouta.csc.fi/Tatoeba-MT-models/eng-kor/opusTCv20210807+bt-2021-11-10.zip

It has 2 separate vocab files for source and target. One minor complication, the vocabs here are stored as plain text lists of vocab items instead of using a yaml file. But it would be straightforward to yamlify it and I could add those as well if needed. The items are simply numbered in the same order they appear.

> Can the condition about dimensionality simply be taken away? Or does that impact anything else?

We can simply remove it.

> It has 2 separate vocab files for source and target.

So the model does share the embeddings between encoder and decoder?

I thought that they were not but now looking at the model they are actually tied. I didn't know that this is possible with two vocabs and then I don't really know what happens internally. I need to check that again and, in that case, maybe this is just another test case of a model to be converted (but not really the one I was thinking of ...)

I have uploaded another model that has separate vocabs and no tied source/target embeddings: https://object.pouta.csc.fi/Tatoeba-MT-models/fin-eng/opusTCv20210807+nopar+ft95-sepvoc_transformer-align_2022-01-28.zip

> I thought that they were not but now looking at the model they are actually tied.

if they are tied that means they use shared vocab, right? 

> I have uploaded another model that has separate vocabs and no tied source/target embeddings:

Awesome! I will use this for the tests. One more question: For this model, are the decoder(target) embeddings tied with the `lm_head` or not? 

The eng-kor model was trained with marian parameters 
```
[2021-11-03 16:34:05] [config] tied-embeddings: false
[2021-11-03 16:34:05] [config] tied-embeddings-all: true
[2021-11-03 16:34:05] [config] tied-embeddings-src: false
```

and the fin-eng model is trained with
```
[2022-01-23 02:10:50] [config] tied-embeddings: true
[2022-01-23 02:10:50] [config] tied-embeddings-all: false
[2022-01-23 02:10:50] [config] tied-embeddings-src: false
```
Both of them are provided with separate vocab files but it could be that the vocabs are concatenated in the eng-kor case as the embeddings are tied (but I don't know).  What it says about the optons in marian (sorry, it's a bit black-box for me):

```
  --tied-embeddings                     Tie target embeddings and output embeddings in output layer
  --tied-embeddings-src                 Tie source and target embeddings
  --tied-embeddings-all                 Tie all embedding layers and output layer
```

Another unrelated question: I happen to have models that have different activation functions in ffn (relu) and aan (swish). The conversion script now checks that they are equal. Could that also be relaxed? ... and also different dimensions in aan and ffn ....

>Both of them are provided with separate vocab files but it could be that the vocabs are concatenated in the eng-kor case as the embeddings are tied (but I don't know)

My guess is also that for eng-kor,  vocabs are concatenated since `tied-embeddings-all`  is `True` which ties src, target and output embeddings.


> I happen to have models that have different activation functions in ffn (relu) and aan (swish). The conversion script now checks that they are equal. Could that also be relaxed? ... and also different dimensions in aan and ffn

Yes! Could you share the checkpoint? I will use that for test and make the necessary changes in the modeling file to support this :)



Here you go: https://object.pouta.csc.fi/Tatoeba-MT-models/fin-eng/opusTCv20210807+bt-2021-12-08.zip

Thank you!

One more issue when converting the HF-Marian model to the corresponding HF Tensorflow class (not sure if it is relevant here).

After [converting a Marian model to HF (Torch)](https://github.com/huggingface/transformers/blob/16f0b7d72c6d4e122957392c342b074aa2c5c519/src/transformers/models/marian/convert_marian_to_pytorch.py), this works fine:
```py
model = MarianMTModel.from_pretrained(MODEL_DIR)
```

But this does not work:
```py
model = TFMarianMTModel.from_pretrained(MODEL_DIR, from_pt=True)
```

It says:
```
Some weights of the PyTorch model were not used when initializing the TF 2.0 model TFMarianMTModel: ['lm_head.weight']
- This IS expected if you are initializing TFMarianMTModel from a PyTorch model trained on another task or with another architecture (e.g. initializing a TFBertForSequenceClassification model from a BertForPreTraining model).
- This IS NOT expected if you are initializing TFMarianMTModel from a PyTorch model that you expect to be exactly identical (e.g. initializing a TFBertForSequenceClassification model from a BertForSequenceClassification model).
Some weights or buffers of the TF 2.0 model TFMarianMTModel were not initialized from the PyTorch model and are newly initialized: ['model.encoder.embed_positions.weight', 'model.decoder.embed_positions.weight']
You should probably TRAIN this model on a down-stream task to be able to use it for predictions and inference.
```

Can you please check if the conversion works for you?

---

However, I don't face this issue for the already available models on HF, like:
```py
model = TFMarianMTModel.from_pretrained('Helsinki-NLP/opus-mt-en-zh', from_pt=True)
```

---

OK, probably it's downloading an already uploaded old TF checkpoint by HF (eventhough I am passing `from_pt=True`).
This throws same logs as reported above, hence the issue is reproducible:

```py
model = MarianMTModel.from_pretrained('Helsinki-NLP/opus-mt-en-zh')
model.save_pretrained("tmp")
del model
model = TFMarianMTModel.from_pretrained("tmp", from_pt=True)
# Same errors
```

---

**NEVERMIND, WE CAN JUST IGNORE THOSE WARNINGS.**
It works using TF.

Also, conversion of the HF Marian model to TorchScript does not work. Sample code:

```py
class MarianMTGenerator(torch.nn.Module):
    def __init__(self, model):
        super().__init__()
        self.model = model.eval()
    
    def forward(self, input_ids, attention_mask):
        return self.model.generate(input_ids=input_ids, attention_mask=attention_mask)

model = MarianMTModel.from_pretrained(MODEL_DIR, torchscript=True)
generator = MarianMTGenerator(model)

torchscript_model = torch.jit.script(generator)
```

The errors were because of type-checking issues encountered by the TorchScript compiler in [`modeling_marian.py`](https://github.com/huggingface/transformers/blob/7732d0f/src/transformers/models/marian/modeling_marian.py). I tried fixing a few things, but I was unable to make it after some point. Can you please check this too? Thanks!

---

BTW, although converting to TorchScript in tracing mode works, it flattens out the decoding loop for a fixed no. of iterations (conditioned on the example input passed), hence does not work for larger sizes of input during runtime.

Sample code:
```py
inputs = tokenizer(["Testing"], return_tensors="pt", padding=True)

# Max pad
batch_size, seq_length = inputs['input_ids'].shape
input_ids_padding = torch.full((batch_size, model.config.max_length-seq_length), tokenizer.pad_token_id, dtype=torch.int64)
inputs['input_ids'] = torch.cat([inputs['input_ids'], input_ids_padding], dim=1)
attention_mask_padding = torch.zeros((batch_size, model.config.max_length-seq_length), dtype=torch.int64)
inputs['attention_mask'] = torch.cat([inputs['attention_mask'], attention_mask_padding], dim=1)

torchscript_model = torch.jit.trace(generator, [inputs['input_ids'], inputs['attention_mask']])
```

Although one can pass a very large text covering the maximum encoder sequence length and ensure that the decoder loop is unrolled for a very large number of iterations, during inference time, this is very inefficient.

Hence for auto-regressive models, I think it might be best to use `jit.script` mode. Please let me know if you have any other alternate thoughts. Thanks!

Hey @jorgtied @GokulNC 

We just merged https://github.com/huggingface/transformers/pull/15752 which now allows to convert the models which don't share embeddings between encoder and decoder and uses separate vocabs. The conversion script is also updated.

For models with two vocab files it looks for two yaml fils which should be named like `*src.vocab.yml`, `*trg.vocab.yml`.

The conditions for dimentionality check are also relaxed. Let me know if you try it and notice any issues. Thanks!

@GokulNC does your torchscript example using 
```
torchscript_model = torch.jit.script(generator)
```
work for you now? i tried it and it and it gave me some errors.

No Matthew, this is still unresolved:
https://github.com/huggingface/transformers/issues/15109#issuecomment-1040233897

Perhaps you can raise a separate issue for that. This issue was more regarding getting different src & tgt vocabs supported in HF's MarianModel.

@GokulNC thank you.