WIP ModelExportInfra #3703

skanjila · 2023-10-08T20:24:41Z

Code Pull Requests

Please provide the following:

a set of classes to do model export in onnx and coreml

Documentation Pull Requests

Note that the documentation HTML files are in docs/ while the Markdown sources are in mkdocs/docs.

If you are proposing a modification to the documentation you should change only the Markdown files.

api.md is automatically generated from the docstrings in the code, so if you want to change something in that file, first modify ludwig/api.py docstring, then run mkdocs/code_docs_autogen.py, which will create mkdocs/docs/api.md .

ludwig/model_export/coreml_exporter.py

ludwig/model_export/onnx_exporter.py

ludwig/model_export/coreml_exporter.py

github-actions · 2023-10-08T21:43:53Z

Unit Test Results

      6 files ±      0       6 suites ±0 1h 4m 51s ⏱️ + 44m 7s
2 794 tests +2 782 2 770 ✔️ +2 761 23 💤 +20 1 ❌ +1
8 382 runs +8 322 8 310 ✔️ +8 268 69 💤 +51 3 ❌ +3

For more details on these failures, see this check.

Results for commit 48f0c55. ± Comparison against base commit f98b8f6.

This pull request removes 4 and adds 2786 tests. Note that renamed tests count towards both.

tests.regression_tests.model.test_old_models ‑ test_model_loaded_from_old_config_prediction_works
tests.regression_tests.model.test_old_models ‑ test_predict_deprecated_model[respiratory]
tests.regression_tests.model.test_old_models ‑ test_predict_deprecated_model[titanic]
tests.regression_tests.model.test_old_models ‑ test_predict_deprecated_model[twitter_bots]

tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_image_augmentation[augmentation_pipeline_ops0]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_image_augmentation[augmentation_pipeline_ops1]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_image_augmentation[augmentation_pipeline_ops2]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[None]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[augmentation_pipeline_ops1]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[augmentation_pipeline_ops2]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[augmentation_pipeline_ops4]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_invalid_augmentation_parameters[random_horizontal_flip]
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_load_model_with_augmentation_pipeline
tests.ludwig.augmentation.test_augmentation_pipeline ‑ test_local_model_training_with_augmentation_pipeline[preprocessing0-encoder0-False]
…

This pull request skips 2 tests.

tests.regression_tests.benchmark.test_model_performance ‑ test_performance[ames_housing.gbm.yaml]
tests.regression_tests.benchmark.test_model_performance ‑ test_performance[mercedes_benz_greener.gbm.yaml]

♻️ This comment has been updated with latest results.

saad-palapa · 2023-10-09T16:05:22Z

ludwig/model_export/base_model_exporter.py

+        pass
+
+    @abstractmethod
+    def quantize(path_fp32, path_int8):


I think it's better design to call this:

def quantize(model_path, quantized_path):

The reason is that not all unquantized models are fp32 and not all quantized models are int8

saad-palapa · 2023-10-09T21:24:30Z

ludwig/model_export/coreml_exporter.py

+
+        width = ludwig_model.config["input_features"][0]["preprocessing"]["width"]
+        height = ludwig_model.config["input_features"][0]["preprocessing"]["height"]
+        example_input = torch.randn(1, 3, width, height, requires_grad=True)


Should we have logic to check if this is channels_first or channels_last?

…delexport

for more information, see https://pre-commit.ci

saad-palapa

Looks good. Left a few comments to consider.

saad-palapa · 2023-10-10T03:23:16Z

ludwig/model_export/base_model_exporter.py

+        pass
+
+    @abstractmethod
+    def quantize(self, path_fp32, path_int8):


The variable naming assumes all non-quantized models to be fp32 and quantized to be int8. This is not always the case. For example, we can have fp16 => fp4 (https://youtu.be/MK4k64vY3xo?si=ULlFZQa_2DSvcgpX&t=1970)

Perhaps the function signature should be:

def quantize(self, model_path, quantized_model_path)

saad-palapa · 2023-10-10T03:24:36Z

ludwig/model_export/coreml_exporter.py

+    def quantize(self, path_fp32, path_int8):
+        import coremltools.optimize.coreml as cto
+
+        ludwig_model = LudwigModel.load(path_fp32)


For now this looks good. But can we add a @todo to add more quantization options.

saad-palapa · 2023-10-10T03:25:59Z

ludwig/model_export/onnx_exporter.py

+    def forward(self, x):
+        return self.model({"image_path": x})
+
+    def export_classifier(self, model_path, export_path):


This is missing export_args_override

saad-palapa · 2023-10-10T03:27:13Z

ludwig/model_export/base_model_exporter.py

+        pass
+
+    @abstractmethod
+    def export_classifier(self, model_path, export_path, export_args_override):


Can we remove this and only have export_args_override in OnnxExporter.

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Justin Zhao <[email protected]>

ludwig/model_export/coreml_exporter.py

…delexport

for more information, see https://pre-commit.ci

saad-palapa · 2023-10-24T02:23:49Z

ludwig/model_export/onnx_exporter.py

+
+
+class OnnxExporter(ABC):
+    def export_classifier(self, model_id, model_path, export_path, input_model_name, output_model_name):


Some of these params are not being used. It think the function signature should look like:

def export(self, model_path, export_path)

I don't think it's necessary to include _classifier in the function name

saad-palapa · 2023-10-24T02:32:27Z

ludwig/model_export/coreml_exporter.py

+from ludwig.model_export.base_model_exporter import BaseModelExporter
+
+
+class CoreMLExporter(BaseModelExporter):


Can we leave the CoreML stuff to another PR. Let's keep the task here small

saad-palapa · 2023-10-24T02:33:07Z

ludwig/model_export/base_model_exporter.py

+        pass
+
+    @abstractmethod
+    def quantize(self, path_fp32, path_int8):


Can we defer teh quantize function to another PR. Let's keep this one small

saad-palapa · 2023-10-24T02:43:56Z

ludwig/model_export/onnx_exporter.py

+            output_names=["combiner_hidden_1", "output", "combiner_hidden_2"],
+        )
+
+    def quantize_onnx(self, model_id, path_fp32, path_int8):


Can we leave quantization off to another PR?

saad-palapa · 2023-10-24T02:44:42Z

ludwig/model_export/onnx_exporter.py

+
+        quantize_dynamic(path_fp32, path_int8)  # type: ignore
+
+    def check_model_export(self, model_id, export_path, output_model_name):


Can we change this function signature to:

def check_model_export(self, path)

I don't think it's necessary to separate folder and filename

shell structure for the model exporter infra classes

f71da9a

skanjila changed the title ~~ModelExportInfra~~ WIP ModelExportInfra Oct 8, 2023

saad-palapa suggested changes Oct 8, 2023

View reviewed changes

saad-palapa reviewed Oct 8, 2023

View reviewed changes

ludwig/model_export/coreml_exporter.py Outdated Show resolved Hide resolved

saad-palapa reviewed Oct 9, 2023

View reviewed changes

skanjila and others added 4 commits October 9, 2023 19:28

pr feedback

0dbc7b0

Merge remote-tracking branch 'upstream/master' into computervision-mo…

fc79c04

…delexport

updated coreml code and other code cleanup

777de9d

[pre-commit.ci] auto fixes from pre-commit.com hooks

b196a79

for more information, see https://pre-commit.ci

saad-palapa approved these changes Oct 10, 2023

View reviewed changes

skanjila and others added 2 commits October 10, 2023 21:21

added test , more code cleanup

f43ae10

Add consumer complaints generation dataset (#3685)

742741b

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Justin Zhao <[email protected]>

saad-palapa reviewed Oct 11, 2023

View reviewed changes

ludwig/model_export/coreml_exporter.py Show resolved Hide resolved

skanjila and others added 3 commits October 19, 2023 18:57

code fixes and a unit test working

b7735f9

Merge remote-tracking branch 'upstream/master' into computervision-mo…

673b7b0

…delexport

[pre-commit.ci] auto fixes from pre-commit.com hooks

48f0c55

for more information, see https://pre-commit.ci

saad-palapa suggested changes Oct 24, 2023

View reviewed changes

skanjila closed this by deleting the head repository Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP ModelExportInfra #3703

WIP ModelExportInfra #3703

skanjila commented Oct 8, 2023

github-actions bot commented Oct 8, 2023 •

edited

Loading

saad-palapa Oct 9, 2023 •

edited

Loading

saad-palapa Oct 9, 2023 •

edited

Loading

saad-palapa left a comment

saad-palapa Oct 10, 2023

saad-palapa Oct 10, 2023

saad-palapa Oct 10, 2023

saad-palapa Oct 10, 2023

saad-palapa Oct 24, 2023

saad-palapa Oct 24, 2023

saad-palapa Oct 24, 2023

saad-palapa Oct 24, 2023

saad-palapa Oct 24, 2023



		class OnnxExporter(ABC):
		def export_classifier(self, model_id, model_path, export_path, input_model_name, output_model_name):

		from ludwig.model_export.base_model_exporter import BaseModelExporter


		class CoreMLExporter(BaseModelExporter):


		quantize_dynamic(path_fp32, path_int8) # type: ignore

		def check_model_export(self, model_id, export_path, output_model_name):

WIP ModelExportInfra #3703

WIP ModelExportInfra #3703

Conversation

skanjila commented Oct 8, 2023

Code Pull Requests

Documentation Pull Requests

github-actions bot commented Oct 8, 2023 • edited Loading

Unit Test Results

saad-palapa Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

saad-palapa Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

saad-palapa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 8, 2023 •

edited

Loading

saad-palapa Oct 9, 2023 •

edited

Loading

saad-palapa Oct 9, 2023 •

edited

Loading