[onnx][importer] Add support for externalized params #18880

vinayakdsci · 2024-10-24T14:14:58Z

This patch adds support to externalize params, and store them to the given path as an IRPA file.

The IR imported with externalization should now prevent possible OOM errors happening due to large inlined parameters.

vinayakdsci · 2024-10-24T14:16:51Z

cc @kumardeepakamd.

vinayakdsci · 2024-10-24T14:33:23Z

There is a failure for models with dynamic dims when the parameters are externalized: #18881.

However, it appears to be unrelated to this patch.

stellaraccident

Thanks. I think you have the basic idea of what to do here, but I need to take a closer look: I think there are ways to do this with less code duplication.

vinayakdsci · 2024-10-24T14:51:22Z

Thanks. I think you have the basic idea of what to do here, but I need to take a closer look: I think there are ways to do this with less code duplication.

Yeah, I am sure about that too. But I had some issues with calls to super(), so I had to duplicate some code to get it working.

P.S: In case it helps, here is the IR generated by the importer for ResNet152_vaiq_int8 https://gist.github.com/vinayakdsci/1e75b651fb66be238f4f6bd9d5e50502

ScottTodd · 2024-10-24T14:53:41Z

Please tag related issues on PRs (#18289 here). Also, have you read through the discussions on #18365?

(the top of https://iree.dev/developers/general/contributing/ has a note about communicating so work isn't duplicated)

vinayakdsci · 2024-10-24T15:06:04Z

Also, have you read through the discussions on #18365?

Thanks, read it now.

(The changes there, though similar, seem to me to be unrelated to this PR, and it is mentioned there that this should have had been implemented separately).

compiler/bindings/python/iree/compiler/tools/import_onnx/__main__.py

ScottTodd · 2024-10-28T17:55:27Z

Also, have you read through the discussions on #18365?

Thanks, read it now.

(The changes there, though similar, seem to me to be unrelated to this PR, and it is mentioned there that this should have had been implemented separately).

Yep, I just want us to be coordinated via public GitHub issues discussions. The issue and PR I linked were tossing around the idea of replacing iree-import-onnx with a new API, while this PR extends the existing tool. There are architectural discussions to be had there on the issue. This component needs an owner that can manage those discussions and coordinate work between contributors/maintainers.

compiler/bindings/python/iree/compiler/tools/import_onnx/__main__.py

vinayakdsci · 2024-11-04T13:09:38Z

@ScottTodd my apologies for the delay, I have addressed all your comments in the latest commit. Also added a new flag that allows configuring the parameter scope.

zjgarvey

I think this is in a good spot to merge for now. @ScottTodd do you have any lingering comments?

ScottTodd

A few style comments and a request for test coverage. This is pretty close otherwise. Thanks!

compiler/bindings/python/iree/compiler/tools/import_onnx/importer_externalization_overrides.py

zjgarvey · 2024-11-12T20:13:19Z

How big is the model.onnx file? I'd be fine with a test using small weights just to make sure the api works.

vinayakdsci · 2024-11-12T20:19:07Z

How big is the model.onnx file? I'd be fine with a test using small weights just to make sure the api works.

The model file is 9.8M in size. Would you like me to make it smaller?

zjgarvey · 2024-11-12T20:22:44Z

Yeah, we want a functionality test, so the file size isn't necessary. Just big enough to work for the default size threshold would be ideal.

vinayakdsci · 2024-11-12T20:40:17Z

@zjgarvey reduced the model file size to 8.0K.

zjgarvey · 2024-11-12T21:04:32Z

Okay, resolve outdated conversations with @ScottTodd and we will wait for his approval.

benvanik · 2024-11-12T22:11:29Z

8KB is fine - thanks for making it small - git never forgets and every KB added is future pain.

ScottTodd · 2024-11-13T18:26:34Z

compiler/bindings/python/iree/compiler/tools/import_onnx/__main__.py

I won't have time to review this again for a few days. One thing that could be worked on in the meantime is documentation at https://iree.dev/guides/ml-frameworks/onnx/ for parameter usage, similar to https://iree.dev/guides/ml-frameworks/pytorch/#using-external-parameters. The source of that page is here: https://github.com/iree-org/iree/blob/main/docs/website/docs/guides/ml-frameworks/onnx.md

ScottTodd

Found a bit of time to review. LGTM!

Please move the test function down in the file
Will you follow up with documentation, or should we file an issue?

ScottTodd · 2024-11-14T16:33:15Z

compiler/bindings/python/test/tools/import_onnx_test.py

+)
+
+
+class ImportOnnxwithExternalizationTest(unittest.TestCase):


Please move this test case under ImportOnnxTest in the file (just before if __name__ == "__main__":), so the test file starts simple and then has more complex test cases.

ScottTodd · 2024-11-14T18:24:43Z

compiler/bindings/python/test/tools/import_onnx_test.py

+    def setUp(self):
+        with tempfile.NamedTemporaryFile(delete=False) as f:
+            self.outputPath = f.name
+
+    def tearDown(self) -> None:
+        if os.path.exists(self.outputPath):
+            os.unlink(self.outputPath)
+        if os.path.exists("custom_params_file.irpa"):
+            os.unlink("custom_params_file.irpa")
+        if os.path.exists(str(self.outputPath) + "_params.irpa"):
+            os.unlink(str(self.outputPath) + "_params.irpa")


This code pattern for opening a temporary file, storing the name of that file, and then later deleting it may not work on all platforms.

edit: I ran this on Windows and it worked. Phew.

vinayakdsci · 2024-11-14T19:26:10Z

@ScottTodd thanks for the approval! I have addressed the remaining comments and rebased the PR.

Will you follow up with documentation, or should we file an issue?

I should be able to open a PR to follow up with the documentation too.

zjgarvey · 2024-11-14T20:02:44Z

@vinayakdsci would you like me to merge this now?

As discussed on #18630 (comment), now that `iree-import-onnx` supports upgrading the model version itself (added in #18880), we don't need to also maintain this logic in the `iree.build` package.

ScottTodd · 2024-11-20T21:50:18Z

compiler/bindings/python/iree/compiler/tools/import_onnx/importer_externalization_overrides.py

+import copy
+import random
+import string
+import iree.runtime as rt


I missed this during review, but this is the only place in the compiler Python bindings that depends on the runtime Python bindings. It is used for rt.ParameterIndex and create_archive_file. That makes

pip install iree-base-compiler[onnx]

insufficient for running iree-import-onnx now, as iree-base-runtime must also be installed

We could

have this check the import and fail gracefully

add a compiler API for creating parameter archives so the runtime API is not needed

have the compiler Python package depend on the runtime Python package

I'm working on a few improvements in #19217. Will make a conditional import for this in that patch.

@ScottTodd I think it would be best to fail gracefully in case the import fails, at least for now. I am not very sure about how having the compiler python package depend on the runtime package should be implemented (maybe you could look at that if you find the time?), and adding a compiler API for creating parameter archives could possibly lead to code duplication IMO.

I can create a PR for the first option right away.

I think the first option, importing conditionally on whether "--externalize-parameters" is passed, with a message saying something like "externalizing parameters requires runtime api" would be the most efficient route.

This patch adds support to externalize params, and store them to the given path as an IRPA file. The IR imported with externalization should now prevent possible OOM errors happening due to large inlined parameters.

…rg#19210) As discussed on iree-org#18630 (comment), now that `iree-import-onnx` supports upgrading the model version itself (added in iree-org#18880), we don't need to also maintain this logic in the `iree.build` package.

This patch adds support to externalize params, and store them to the given path as an IRPA file. The IR imported with externalization should now prevent possible OOM errors happening due to large inlined parameters. Signed-off-by: Giacomo Serafini <179146510+giacs-epic@users.noreply.github.com>

…rg#19210) As discussed on iree-org#18630 (comment), now that `iree-import-onnx` supports upgrading the model version itself (added in iree-org#18880), we don't need to also maintain this logic in the `iree.build` package. Signed-off-by: Giacomo Serafini <179146510+giacs-epic@users.noreply.github.com>

vinayakdsci marked this pull request as draft October 24, 2024 14:15

vinayakdsci requested review from stellaraccident, zjgarvey, vivekkhandelwal1, ScottTodd and rsuderman October 24, 2024 14:15

vinayakdsci marked this pull request as ready for review October 24, 2024 14:16

stellaraccident reviewed Oct 24, 2024

View reviewed changes

vinayakdsci requested a review from stellaraccident October 24, 2024 16:41

zjgarvey reviewed Oct 24, 2024

View reviewed changes

vinayakdsci force-pushed the importer-external-weight-support branch from 5eb7b87 to b6ecf5a Compare October 25, 2024 17:56

vinayakdsci requested a review from zjgarvey October 25, 2024 17:59

vinayakdsci force-pushed the importer-external-weight-support branch from b6ecf5a to 4f52c82 Compare October 25, 2024 18:10

ScottTodd reviewed Oct 28, 2024

View reviewed changes

ScottTodd mentioned this pull request Oct 31, 2024

[python] Add an iree.build package with API/tooling for program building. #18630

Merged

vinayakdsci requested a review from ScottTodd November 4, 2024 13:09

zjgarvey approved these changes Nov 4, 2024

View reviewed changes

ScottTodd reviewed Nov 5, 2024

View reviewed changes

vinayakdsci force-pushed the importer-external-weight-support branch from b5db48b to 13b5638 Compare November 11, 2024 06:01

vinayakdsci requested review from ScottTodd and zjgarvey November 12, 2024 20:00

vinayakdsci force-pushed the importer-external-weight-support branch from 1da3d7a to 9a5f443 Compare November 12, 2024 20:37

ScottTodd added the integrations/onnx ONNX integration work label Nov 13, 2024

ScottTodd reviewed Nov 13, 2024

View reviewed changes

zjgarvey mentioned this pull request Nov 14, 2024

Support large onnx model intake nod-ai/SHARK-TestSuite#395

Open

2 tasks

ScottTodd self-requested a review November 14, 2024 16:40

ScottTodd approved these changes Nov 14, 2024

View reviewed changes

vinayakdsci added 9 commits November 15, 2024 00:52

[onnx][importer] Add support for externalized params

5da4e60

Refactor code to remove duplications

61a7438

Fix IR import for integer to signless integer conversions

5e7313d

Address comments

eefb662

Move overrides to separate file

cf08fce

Address comments

40cbcb7

Address comments-2

8f32746

Add tests for externalization

0836529

Move externalization tests in test file

c586034

vinayakdsci force-pushed the importer-external-weight-support branch from 9a5f443 to c586034 Compare November 14, 2024 19:24

zjgarvey merged commit d497571 into iree-org:main Nov 14, 2024
36 checks passed

ScottTodd mentioned this pull request Nov 19, 2024

Use iree-import-onnx --opset-version N in ImportOnnxAction. #19210

Merged

ScottTodd reviewed Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[onnx][importer] Add support for externalized params #18880

[onnx][importer] Add support for externalized params #18880

vinayakdsci commented Oct 24, 2024

vinayakdsci commented Oct 24, 2024

vinayakdsci commented Oct 24, 2024 •

edited

Loading

stellaraccident left a comment

vinayakdsci commented Oct 24, 2024

ScottTodd commented Oct 24, 2024

vinayakdsci commented Oct 24, 2024

ScottTodd commented Oct 28, 2024

vinayakdsci commented Nov 4, 2024

zjgarvey left a comment

ScottTodd left a comment

zjgarvey commented Nov 12, 2024

vinayakdsci commented Nov 12, 2024

zjgarvey commented Nov 12, 2024

vinayakdsci commented Nov 12, 2024

zjgarvey commented Nov 12, 2024

benvanik commented Nov 12, 2024

ScottTodd Nov 13, 2024

ScottTodd left a comment

ScottTodd Nov 14, 2024

ScottTodd Nov 14, 2024

vinayakdsci commented Nov 14, 2024

zjgarvey commented Nov 14, 2024

ScottTodd Nov 20, 2024

zjgarvey Nov 20, 2024

vinayakdsci Nov 20, 2024 •

edited

Loading

zjgarvey Nov 20, 2024

		)


		class ImportOnnxwithExternalizationTest(unittest.TestCase):

[onnx][importer] Add support for externalized params #18880

[onnx][importer] Add support for externalized params #18880

Conversation

vinayakdsci commented Oct 24, 2024

vinayakdsci commented Oct 24, 2024

vinayakdsci commented Oct 24, 2024 • edited Loading

stellaraccident left a comment

Choose a reason for hiding this comment

vinayakdsci commented Oct 24, 2024

ScottTodd commented Oct 24, 2024

vinayakdsci commented Oct 24, 2024

ScottTodd commented Oct 28, 2024

vinayakdsci commented Nov 4, 2024

zjgarvey left a comment

Choose a reason for hiding this comment

ScottTodd left a comment

Choose a reason for hiding this comment

zjgarvey commented Nov 12, 2024

vinayakdsci commented Nov 12, 2024

zjgarvey commented Nov 12, 2024

vinayakdsci commented Nov 12, 2024

zjgarvey commented Nov 12, 2024

benvanik commented Nov 12, 2024

ScottTodd Nov 13, 2024

Choose a reason for hiding this comment

ScottTodd left a comment

Choose a reason for hiding this comment

ScottTodd Nov 14, 2024

Choose a reason for hiding this comment

ScottTodd Nov 14, 2024

Choose a reason for hiding this comment

vinayakdsci commented Nov 14, 2024

zjgarvey commented Nov 14, 2024

ScottTodd Nov 20, 2024

Choose a reason for hiding this comment

zjgarvey Nov 20, 2024

Choose a reason for hiding this comment

vinayakdsci Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

zjgarvey Nov 20, 2024

Choose a reason for hiding this comment

vinayakdsci commented Oct 24, 2024 •

edited

Loading

vinayakdsci Nov 20, 2024 •

edited

Loading