Optimum ONNX Runtime API improvement by michaelbenayoun · Pull Request #515 · huggingface/optimum

michaelbenayoun · 2022-11-24T17:28:48Z

What does this PR do?

ORTModel, ORTModelDecoder and ORTModelForConditionalGeneration can load any ONNX model files regardless of their names, allowing to load optimized and quantized models without having to specify the file_name argument.
ORTModel._from_transformers now downloads and loads the model in a temporary directory instead of the cache, which was not a right place to store it.
ORTQuantizer saves both the model configuration and the tokenizer / processor / feature extractor, making the exported directory usable end-to-end.
ORTOptimizer was already saving the model configuration but not the tokenize / processor / feature extractor, it does now.
OnnxConfigWithPast now has two class atributes (that will be used when no value is specified in the __init__ to avoid having to overwrite the __init__ just for that) :
- USE_PAST_FOR_INPUTS: if True, past key values will be in the inputs of the model
- USE_PRESENT_FOR_OUTPUTS: if True, present key values will be in the outputs of the model
- This is useful for decoder and encoder-decoder models (cc @mht-sharma)

Those features combined allow the following:

from transformers import AutoTokenizer
from optimum.onnxruntime import (
    ORTQuantizer,
    ORTOptimizer,
    ORTConfig,
    AutoQuantizationConfig,
    AutoOptimizationConfig,
    ORTModelForSequenceClassification
)
model_name = "distilbert-base-uncased-finetuned-sst-2-english" 

model = ORTModelForSequenceClassification.from_pretrained(model_name, from_transformers=True)
optimizer = ORTOptimizer.from_pretrained(model)

optimizer.optimize(
AutoOptimizationConfig.with_optimization_level("O2"), "optimized_model"
)

quantizer = ORTQuantizer.from_pretrained(model)
quantizer.quantize(
     AutoQuantizationConfig.avx512_vnni(is_static=False), "quantized_model"
)

optimized_model = ORTModelForSequenceClassification.from_pretrained("optimized_model")
quantized_model = ORTModelForSequenceClassification.from_pretrained("quantized_model")

tokenizer = AutoTokenizer.from_pretrained("optimized_model")
inputs = tokenizer("This is easier now!", return_tensors="pt")

print(model(**inputs))
print(optimized_model(**inputs))
print(quantized_model(**inputs))

This was not possible before.

Fixes: #493

HuggingFaceDocBuilderDev · 2022-11-24T17:47:05Z

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

mht-sharma · 2022-12-05T11:32:02Z

-        feature_extractor.save_pretrained(args.output.parent)
-    except Exception:
-        pass
+    maybe_save_tokenizer_or_processor_or_feature_extractor(args.model, args.output.parent)


I remember discussing to move this inside export, is that still the case?

I actually do not know if we should put this inside the export itself, good question.

mht-sharma · 2022-12-05T11:32:57Z

+            task,
+            model_id,
+            subfolder=subfolder,
+            revision=revision,
+            cache_dir=cache_dir,
+            config=config,
+            use_auth_token=use_auth_token,
+            local_files_only=local_files_only,
+            force_download=force_download,
+        )
+
+        model_type = model.config.model_type.replace("_", "-")
+        model_name = getattr(model, "name", None)
+
+        onnx_config_constructor = TasksManager.get_exporter_config_constructor(
+            model_type, "onnx", task=task, model_name=model_name
+        )
+        onnx_config = onnx_config_constructor(model.config, use_present_in_outputs=True)


I see this being repeated in multiple files. Would it make sense to have a helper function for these lines?

I tried implementing it, but end up with circular imports!

fxmarty · 2022-12-05T13:48:17Z

        os.makedirs(save_directory, exist_ok=True)

        self.config.save_pretrained(save_directory)
+        for preprocessor in self._preprocessors:


PreTrainedModel does not save preprocessors with save_pretrained, why would we introduce this asymmetry in Optimum? Personnally I don't understand the motivation for this self._preprocessors. What's the issue with fully dissociating preprocessing and modeling, like it's done in transformers?

I also think it would be better to break down these huge PRs in smaller ones

If it is not there, we do not do anything, otherwise we save it. The reason for this is because let's say I export a model with the ORTModel classes, I want to be able to use the save directory as a model repo end-to-end. Same thing for graph optimization / quantization. Otherwise we would need to provide a different name for the ORTModel.from_pretrained and the AutoTokenizer.from_pretrained.

fxmarty · 2022-12-05T13:59:05Z

-                os.path.join(model_id, subfolder, decoder_with_past_file_name) if use_cache else None
+        model_path = Path(model_id)
+
+        def validate_filename(filename):


Would this be better out of _from_pretrained. Otherwise this is redefined every time _from_pretrained is called, am I correct?

Yes, which is not a big deal for functions with a one-time usage.

why have it as a function in the first time then?

We use it multiple time inside this function, but never outside. We could make that a method, but that is not needed specifically. While it is true that there is redundancy between this and modeling_seq2seq.py, I wanted to keep the function simple (making it a method woulrd require more arguments and so on).

fxmarty · 2022-12-05T13:59:49Z

+                succeeded = False
+            return succeeded
+
+        def infer_filename(pattern: str, argument_name: str, fail_if_not_found: bool = True) -> str:


Same as above (Can we add a short comment as to what this function does + wouldn't it be better out)

I do not think this is needed, this is basically a temporary function.

I believe the name is enough, I renamed the first one since the name was not explicit.

fxmarty · 2022-12-05T14:02:09Z

 from ..exporters import TasksManager
 from ..exporters.onnx import export
 from ..modeling_base import FROM_PRETRAINED_START_DOCSTRING, OptimizedModel
+from ..utils.save_utils import maybe_load_preprocessors, maybe_save_tokenizer_or_processor_or_feature_extractor


At first glance I don't understand what the maybe means in the functions name

Because it might load / save or not. If nothing is found, it won't do anything.

fxmarty · 2022-12-05T14:10:24Z

+    ):
+        io_binding = self.session.io_binding()

-        if "TensorrtExecutionProvider" in self.providers and self.use_io_binding:


Is this removed? I could not find it elsewhere. If removed, why?

It is in ORTModel, now ORTModelForConditionalGeneration uses the base class initialiazer, so we do not need that anymore here.

fxmarty · 2022-12-06T09:24:35Z

Why was it merged? I would have liked to rereview and I don't think all my points were addressed.

michaelbenayoun · 2022-12-06T09:31:45Z

Which point do you want to discuss? We can discuss them, and I will make the PR for those changes (if needed).

fxmarty · 2022-12-06T09:36:46Z

#515 (comment) => I still don't understand why we introduce self._preprocessors that does not exist in transformers.

#515 (comment) => could not find a description

#515 (comment) => same, I disagree it is not needed, it would help readability

#515 (comment) => could not find a documentation, but could have missed it

michaelbenayoun · 2022-12-06T10:17:43Z

The idea behind self._preprocessors is to be able to save all the model's preprocessors when possible, this will make using the saved model much easier.

Example: You load bert-base-uncased and quantize it. Now you have a bert_quantized directory.

With self._preprocessors we are able to save the preprocessors during quantization, meaning that you ca do:

model_name = "bert_quantized"
tok = AutoTokenizer.from_pretrained(model_name)
quantized_model = ORTModel.from_pretrained(model_name)

instead of:

tok = AutoTokenizer.from_pretrained("bert-base-uncased")
quantized_model = ORTModel.from_pretrained("bert_quantized")

This makes things a lot easier in terms of usage, especially if we think about production.

I am not really in favor of documenting inner functions, especially this one, it does what the name implies: it infers the filename of the onnx file. But I will document it if you think this is needed here.
Same comment, validate_file_exists is quite explicit it terms of naming, it checks that the file called "filename" exists. I will put a comment if you think this is needed too.
You mean that you want to say in the docstring that we use a regex to find potential files?

fxmarty · 2022-12-07T08:40:00Z

@michaelbenayoun What is the difference between the two code samples?

michaelbenayoun force-pushed the ort_quantizer_and_optimizer_export branch 4 times, most recently from 4ba8d37 to ff0042b Compare December 2, 2022 15:56

michaelbenayoun marked this pull request as ready for review December 2, 2022 15:57

michaelbenayoun requested review from echarlaix, fxmarty and mht-sharma December 2, 2022 17:08

michaelbenayoun and others added 16 commits December 5, 2022 10:45

ORTQuantizer saves model config when possible

7ef778a

Fix docstring

3023141

ORTModel can load any name for the ONNX model

6177194

Use regex now

b39370d

Almost ready

814a190

modeling_ort

8cff703

test

d2d4c7b

Works for ORTModel

55bae42

Fixed bugs

f1d032a

remove pdb

0b01c3c

Remove unnecessary attribute

5871bc9

ORTDecoderModel ready

9da9edd

Fix issue

b8715ac

Make style

094c1f0

fix issues

143bda9

Rebase and style

f503ea6

michaelbenayoun force-pushed the ort_quantizer_and_optimizer_export branch from 2d8b31d to f503ea6 Compare December 5, 2022 09:49

michaelbenayoun changed the title ~~ORTQuantizer saves model config when possible~~ Optimum ONNX Runtime API improvement Dec 5, 2022

Fix

f02703d

mht-sharma reviewed Dec 5, 2022

View reviewed changes

regisss reviewed Dec 5, 2022

View reviewed changes

Comment thread optimum/exporters/onnx/base.py

Comment thread optimum/onnxruntime/modeling_seq2seq.py Outdated