Making ONNX conversion directly load the model and tokenizer + adding tests
Author: abelriboulot
Pull request: yes
Labels: name: wontfix

This is a proposal for an update in the ONNX conversion:
- First, I investigated issue #4906 as I was having the same issue. This is due to a dependency on this [commit](https://github.com/pytorch/pytorch/commit/96989a2a114de9b77e7dd9495d62c4a8a549b40d) from the ONNX team available from version 1.5.0 of PyTorch, I therefore added it to the extra requirements and added to the messages to make it more obvious (along with keras2onnx for TF)
- The bigger part of this PR aims to remove the dependency of the script for specific pipelines. I believe this dependency is not needed as a conversion to ONNX simply requires a model, and a tokenizer. There are a few advantages to doing it this way:
1. This would solve #4788, and help greatly towards #5075. With this update, the conversion to ONNX no longer requires a given pipeline, and therefore any model can be converted (I have tested for instance with T5)
2. It is maybe clearer since the elements of the pipeline are not exported onto ONNX.
Let me know your thoughts on that one.
- I added some fast-running integration testing of the script to the existing tests.
- I made the onnx export compatible with the ModelOutput refactor (I believe previously it wasn't?)

A question I have is whether I should add a message or add the possibility for the user to provide a pipeline even if it is not used in order to make it back-compatible?

Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/5687?src=pr&el=h1) Report
> Merging [#5687](https://codecov.io/gh/huggingface/transformers/pull/5687?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/0267668c3d648c6e41afda97f5df8671ee880ac3&el=desc) will **increase** coverage by `0.89%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/5687/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/5687?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #5687      +/-   ##
==========================================
+ Coverage   77.01%   77.90%   +0.89%     
==========================================
  Files         128      146      +18     
  Lines       21615    25983    +4368     
==========================================
+ Hits        16646    20243    +3597     
- Misses       4969     5740     +771     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/5687?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/\_\_init\_\_.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9fX2luaXRfXy5weQ==) | `99.24% <ø> (+0.11%)` | :arrow_up: |
| [src/transformers/benchmark/benchmark.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9iZW5jaG1hcmsvYmVuY2htYXJrLnB5) | `74.01% <ø> (+5.16%)` | :arrow_up: |
| [src/transformers/benchmark/benchmark\_args.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9iZW5jaG1hcmsvYmVuY2htYXJrX2FyZ3MucHk=) | `86.04% <ø> (+0.68%)` | :arrow_up: |
| [src/transformers/benchmark/benchmark\_args\_tf.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9iZW5jaG1hcmsvYmVuY2htYXJrX2FyZ3NfdGYucHk=) | `87.50% <ø> (ø)` | |
| [src/transformers/benchmark/benchmark\_args\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9iZW5jaG1hcmsvYmVuY2htYXJrX2FyZ3NfdXRpbHMucHk=) | `89.13% <ø> (-7.75%)` | :arrow_down: |
| [src/transformers/benchmark/benchmark\_tf.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9iZW5jaG1hcmsvYmVuY2htYXJrX3RmLnB5) | `61.53% <ø> (ø)` | |
| [src/transformers/benchmark/benchmark\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9iZW5jaG1hcmsvYmVuY2htYXJrX3V0aWxzLnB5) | `69.50% <ø> (-3.60%)` | :arrow_down: |
| [src/transformers/configuration\_albert.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX2FsYmVydC5weQ==) | `100.00% <ø> (ø)` | |
| [src/transformers/configuration\_auto.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX2F1dG8ucHk=) | `100.00% <ø> (+7.14%)` | :arrow_up: |
| [src/transformers/configuration\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX2JhcnQucHk=) | `93.75% <ø> (+0.41%)` | :arrow_up: |
| ... and [108 more](https://codecov.io/gh/huggingface/transformers/pull/5687/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/5687?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/5687?src=pr&el=footer). Last update [dc31a72...977fc15](https://codecov.io/gh/huggingface/transformers/pull/5687?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


I added a bit to this. The wrapper model is now recursive, which lets models such as T5 export properly. Additionally, thanks to the ModelOutput refactor, now output names can be automatically extracted!

Any news on this @mfuntowicz ? I'm waiting to have this merged to attempt to make a T5 -> ONNX -> ONNX.js script.

I'm adding bunch of people to the PR as I would like to get their feeling on these changes 😊 

Thanks for the review @mfuntowicz and I definitely understand the need to get a consensus on this. To sum up why I think it might be a good idea to remove the dependency on pipelines is because at the moment the pipeline is only used as a way to load a model and a tokenizer. I came into this issue while trying to export T5 (and it seems that a few people also had that issue in the thread for #5518). Doing it directly without passing by the pipelines is closer in my opinion to what the script actually does (grabbing a model and a tokenizer), makes every ONNX-compatible model exportable by default, and avoids confusions such as that choosing a different pipeline for the same model would change the output ONNX model.
As for the other elements, thanks a lot for noting them, I completely agree with the points, and didn't know about the return_tuples parameter, that should save a lot of weirdness in the code! Although I guess this might lose the named outputs part, but I'm sure there's a cleaner way to do it.

If I may add some motivation to such PR, some tasks like MultiChoice questions are not managed by pipelines. Therefore we had to perform the conversion ourselves, and it appeared that depending of the model, there are some bugs in pytorch to onnx method (from pytorch lib) in the order input have to be provided (https://github.com/microsoft/onnxruntime/issues/4292, I know you have fixed a similar issue in this repo but for pipeline tasks only). Now, we are using some other model that have not such bug, we need to manage both cases, etc. Being able to rely on the lib code (well tested / documented) instead of having to maintain our own code would be a big improvement and may directly increase the number of teams putting Transformer based models in prod (onnx runtime providing big perf boost).

Hey @mfuntowicz !
Just wanted to check in to see whether I should adjust the PR with your comments or whether the team prefers to keep the pipelines as is.

Hi @abelriboulot, do you think your PR can solve https://github.com/huggingface/transformers/issues/6503 as well?

Hi @Zhen-hao, I took a look at your issue and I think it might. I haven't heard back from huggingface on this PR, so I think I might make a separate package to easily convert huggingface models to ONNX. If you're interested I'll keep you in the loop!

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.


In case anyone is looking for it, the package is [there](https://github.com/abelriboulot/onnxt5)! Hope it helps.

Is there any way to convert Helsinki-NLP/opus-mt-en-ROMANCE to onnx format ?

@patil-suraj was working on a library that might help with that, he might know better!