Added openvino compiler with tensorflow interface #137

marcoschouten · 2023-01-05T17:00:59Z

No description provided.

diegofiori

Hello @marcoschouten can you please fix the issues I pointed out in the review?

workflows/tests.yml

nebullvm/tools/openvino.py

nebullvm/operations/optimizations/compilers/openvino.py

diegofiori · 2023-01-05T17:43:51Z

nebullvm/operations/optimizations/compilers/openvino.py

+
+    def execute(
+        self,
+        model: Union[str, Path],


the input for tensorflow compilers should be a tensorflow mode not a file. You should then save it to file and just at the end give it as input to the OpenVino CLI.

Sure, this is how I would do it:

Save the model to a file, (would we need to define a specific folder?):
model.save('saved_model/my_model')

Load the model:
my_model= tf.keras.models.load_model('saved_model/my_model')

Pass it as input to the OpenVino CLI
mo --input_model <my_model>.pb

You can skip step 2, the execute method receives a tensorflow model, you can then save it to file (remember to save it to a temp directory as we do in the other sections) and pass it to the mo command

diegofiori · 2023-01-05T17:45:32Z

nebullvm/operations/inference_learners/builders.py

@@ -146,6 +146,25 @@ def execute(
        )


+class TensorFlowOpenVINOBuildInferenceLearner(BuildInferenceLearner):


this class is actually useless. You can simply reuse the OpenVINOBuildInferenceLearner passing TensorFlow as source_dl_framework.

marcoschouten · 2023-01-05T18:25:37Z

Sure thing, I will fix the code once I will get home. Marco

…

On Thu, 5 Jan 2023, 6:48 pm Diego Fiori, ***@***.***> wrote: ***@***.**** requested changes on this pull request. Hello @marcoschouten <https://github.com/marcoschouten> can you please fix the issues I pointed out in the review? ------------------------------ In workflows/tests.yml <#137 (comment)>: > @@ -0,0 +1,120 @@ +name: Run tests I think this is a duplicate file: could you remove it? ------------------------------ In nebullvm/tools/openvino.py <#137 (comment)>: > @@ -0,0 +1,174 @@ +from pathlib import Path I think this is a duplicate file: could you remove it? ------------------------------ In nebullvm/operations/optimizations/compilers/openvino.py <#137 (comment)>: > @@ -25,6 +25,137 @@ from nebullvm.tools.transformations import MultiStageTransformation +# classe di base. Contains generic methods. please write comments in english ------------------------------ In nebullvm/operations/optimizations/compilers/openvino.py <#137 (comment)>: > + + check_quantization(quantization_type, metric_drop_ths) + train_input_data = input_data.get_split("train").get_numpy_list( + QUANTIZATION_DATA_NUM + ) + + # SYNTAX for MO command + # f"""mo + # --saved_model_dir "{model_path}" + # --input_shape "[1,224,224,3]" + # --mean_values="[127.5,127.5,127.5]" + # --scale_values="[127.5]" + # --model_name "{model_path.name}" + # --compress_to_fp16 + # --output_dir "{model_path.parent}" + # """ remove the commented code above ------------------------------ In nebullvm/operations/optimizations/compilers/openvino.py <#137 (comment)>: > +class TensorFlowOpenVINOCompiler(Compiler): + supported_ops = { + "cpu": [ + None, + # QuantizationType.STATIC, + # QuantizationType.HALF, + ], + "gpu": [], + } + + def __init__(self): + super().__init__() + + def execute( + self, + model: Union[str, Path], the input for tensorflow compilers should be a tensorflow mode not a file. You should then save it to file and just at the end give it as input to the OpenVino CLI. ------------------------------ In nebullvm/operations/inference_learners/builders.py <#137 (comment)>: > @@ -146,6 +146,25 @@ def execute( ) +class TensorFlowOpenVINOBuildInferenceLearner(BuildInferenceLearner): this class is actually useless. You can simply reuse the OpenVINOBuildInferenceLearner passing TensorFlow as source_dl_framework. — Reply to this email directly, view it on GitHub <#137 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKBLQ2AQMUDEKTZHKPCNHQDWQ4CPPANCNFSM6AAAAAATSFME7M> . You are receiving this because you were mentioned.Message ID: ***@***.***>

nebullvm/operations/optimizations/compilers/openvino.py

valeriosofi · 2023-01-09T12:07:13Z

nebullvm/operations/optimizations/compilers/openvino.py

@@ -40,7 +171,7 @@ def __init__(self):

    def execute(
        self,
-        model: Union[str, Path],
+        model: tf.keras.Model(),


model type should be only tf.keras.Model

nebullvm/operations/optimizations/compilers/openvino.py

valeriosofi · 2023-01-09T12:21:14Z

nebullvm/operations/optimizations/compilers/openvino.py

+
+
+        # save model to temporary folder
+        dirpath = tempfile.mkdtemp()


as temporary directory you can use the directory that is passed to each compiler execute with the parameter onnx_output_path, you can find it in nebullvm/operations/optimizations/base.py when compiler_op.to(self.device).execute is called. I would also rename the parameter to something more general (such as tmp_dir_path), and when you rename it make sure to rename it wherever it's used.

valeriosofi · 2023-01-09T16:20:36Z

nebullvm/operations/optimizations/compilers/openvino.py

+
+
+        # save model to temporary folder   
+        tmp_dir_path = Compiler.onnx_output_path     


Be careful, the Compiler class doesn't have an onnx_output_path attribute.
onnx_output_path is an argument passed to this method from the caller (it's passed to each execute method of each compiler), so you should add it to the arguments above of the execute (line 46->51) like this:

def execute( self, model: tf.keras.Model, model_params: ModelParams, onnx_output_path: str, input_tfms: MultiStageTransformation = None, metric_drop_ths: float = None, quantization_type: QuantizationType = None, input_data: DataManager = None, **kwargs, ):

This parameters was introduced in the DeepSparseCompiler, that needs to export the original PyTorch model to onnx and so it needs the path to a temporary dir to generate the onnx model (that's why it's called onnx_output_path). In this case we are going to use it to convert a model to openvino, so I would suggest to rename it to tmp_dir_path that is a more general name (please reame it also in the DeepSparseCompiler and in the caller).

valeriosofi · 2023-01-11T09:23:09Z

@marcoschouten I ran the unit tests but they are not passing, can you check?

marcoschouten · 2023-01-11T15:26:32Z

The error is:
Can't instantiate abstract class TensorFlowOpenVINOCompiler with abstract method _quantize_model

I discovered that there was commented code for the quantization, which I uncommented.

How did you run the unit tests?

valeriosofi · 2023-01-11T15:37:16Z

The error is: Can't instantiate abstract class TensorFlowOpenVINOCompiler with abstract method _quantize_model

I discovered that there was commented code for the quantization, which I uncommented.

How did you run the unit tests?

They are triggered for each new commit on this PR, you can also run them locally by running the pytest command in the speedster folder

marcoschouten · 2023-01-12T17:00:24Z

The error is: Can't instantiate abstract class TensorFlowOpenVINOCompiler with abstract method _quantize_model.
I discovered that there was commented code for the quantization, which I uncommented.
How did you run the unit tests?

They are triggered for each new commit on this PR, you can also run them locally by running the pytest command in the speedster folder

From the error message, I noticed that the _quantize_model() method must be implemented.
Because it is required by the Class interface defined in compilers\base.py.

@abc.abstractmethod
  def _quantize_model(self, **kwargs) -> Any:
      raise NotImplementedError()

Other classes, such as class OpenVINOCompiler(Compiler) or class PytorchBackendCompiler(Compiler) have different implementations (i.e. input parameters) of the _quantize_model() method.

It is not clear to me how the quantization is done, and how the _quantize_model() class method should be implemented for the specific TensorFlow-OpenVino conversion.

I read about quantization for Openvino here

https://docs.openvino.ai/2020.4/pot_compression_algorithms_quantization_README.html
Each framework has its own parameters and formatting.

I recall that the original goal was to translate a model from TensorFlow to OpenVino.

So it might make sense to convert first the model to Openvino (by running the execute command) and, at a later point in time, perform the quantization on the Openvino model (then following the Openvino quantization functions and the appropriate parameters.

What exactly would you like me to do?

Added openvino compiler with tensorflow interface

21ba2df

diegofiori requested changes Jan 5, 2023

View reviewed changes

marcoschouten added 3 commits January 9, 2023 11:43

Refactor: Removed comments & duplicate code

8df7a41

Refactor: removed duplicate code and comments

260a908

Saved TF model to temp directory & passed to CMD

4e5f201

valeriosofi requested changes Jan 9, 2023

View reviewed changes

Added: save TensorFlow model to tmp folder

172a784

valeriosofi requested changes Jan 9, 2023

View reviewed changes

Renamed tmp_dir_path

6e15480

removed some comments for Quantization

2999748

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added openvino compiler with tensorflow interface #137

Added openvino compiler with tensorflow interface #137

marcoschouten commented Jan 5, 2023

diegofiori left a comment

diegofiori Jan 5, 2023

marcoschouten Jan 9, 2023 •

edited

Loading

valeriosofi Jan 9, 2023

diegofiori Jan 5, 2023

marcoschouten commented Jan 5, 2023 via email

valeriosofi Jan 9, 2023

valeriosofi Jan 9, 2023

valeriosofi Jan 9, 2023

valeriosofi commented Jan 11, 2023

marcoschouten commented Jan 11, 2023

valeriosofi commented Jan 11, 2023

marcoschouten commented Jan 12, 2023

		@@ -146,6 +146,25 @@ def execute(
		)


		class TensorFlowOpenVINOBuildInferenceLearner(BuildInferenceLearner):



		# save model to temporary folder
		dirpath = tempfile.mkdtemp()



		# save model to temporary folder
		tmp_dir_path = Compiler.onnx_output_path

Added openvino compiler with tensorflow interface #137

Are you sure you want to change the base?

Added openvino compiler with tensorflow interface #137

Conversation

marcoschouten commented Jan 5, 2023

diegofiori left a comment

Choose a reason for hiding this comment

diegofiori Jan 5, 2023

Choose a reason for hiding this comment

marcoschouten Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

valeriosofi Jan 9, 2023

Choose a reason for hiding this comment

diegofiori Jan 5, 2023

Choose a reason for hiding this comment

marcoschouten commented Jan 5, 2023 via email

valeriosofi Jan 9, 2023

Choose a reason for hiding this comment

valeriosofi Jan 9, 2023

Choose a reason for hiding this comment

valeriosofi Jan 9, 2023

Choose a reason for hiding this comment

valeriosofi commented Jan 11, 2023

marcoschouten commented Jan 11, 2023

valeriosofi commented Jan 11, 2023

marcoschouten commented Jan 12, 2023

marcoschouten Jan 9, 2023 •

edited

Loading