Skip to content

Conversation

justinchuby
Copy link
Contributor

@justinchuby justinchuby commented Apr 18, 2025

Describe your changes

  • Avoid redundant disk IO when saving a dynamo exported model by saving the ir.Model directly
  • Refactor torch.onnx conversion passes
  • Fixed the neural compressor pass when models contain additional value info protos for initializers because neural-compressor doesn't handle them properly.

Checklist before requesting a review

  • Add unit tests for this change.
  • Make sure all tests can pass.
  • Update documents if necessary.
  • Lint and apply fixes to your code by running lintrunner -a
  • Is this a user-facing change? If yes, give a description of this change to be included in the release notes.
  • Is this PR including examples changes? If yes, please remember to update example documentation in a follow-up PR.

@justinchuby justinchuby requested review from titaiwangms, Copilot and jambayk and removed request for titaiwangms April 18, 2025 00:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the model conversion flow to avoid redundant disk IO when saving a Dynamo exported model and enables direct saving of ONNX IR models by introducing the ir_model_to_olive_model function.

  • Updated onnxscript_fusion.py to replace model_proto_to_olive_model with ir_model_to_olive_model.
  • Refactored conversion.py to add separate export methods for torchscript and dynamo exporters.
  • Added ir_model_to_olive_model in onnx/common.py to support saving IR models.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
olive/passes/onnx/onnxscript_fusion.py Updated the model conversion call to use ir_model_to_olive_model.
olive/passes/onnx/conversion.py Refactored export methods and introduced dynamo exporter changes.
olive/passes/onnx/common.py Added the new function ir_model_to_olive_model for saving IR models.

jambayk
jambayk previously approved these changes Apr 18, 2025
@justinchuby justinchuby marked this pull request as draft April 22, 2025 00:31
@justinchuby
Copy link
Contributor Author

justinchuby commented Apr 22, 2025

@titaiwangms
Copy link
Contributor

https://aiinfra.visualstudio.com/PublicPackages/_build/results?buildId=758867&view=logs&j=168020a6-ab1b-592e-b9e6-a6cd0eb1939c&t=f9534d71-ad75-594d-34df-0afc963a2a4d&l=10801

@titaiwangms do you have an idea about this error in _rename_dynamic_shapes_with_model_inputs?

Yes, it's a bug and the function will be deleted in 2.7. Because I didn't consider the mismatch input names between onnx graph and nn.Module, it causes KeyError to the matching. (That's why I set it to 2.7)

@justinchuby justinchuby marked this pull request as ready for review April 22, 2025 17:26
@justinchuby justinchuby requested a review from Copilot April 22, 2025 17:26
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the dynamo exporter flow to avoid redundant disk IO and updates the torch.onnx conversion passes while improving type annotations and generic typing.

  • Update test conditions to account for torch version 2.7.0 and dynamic shape handling.
  • Refactor type hints in IoConfig by marking several fields as Optional.
  • Enhance generic typing in config_utils with a type variable.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
test/unit_test/passes/onnx/test_conversion.py Adjusted test skip conditions for Windows and dynamic shapes based on torch version.
olive/model/config/io_config.py Updated type annotations to use Optional for various input/output fields.
olive/common/config_utils.py Modified the config union type to use a generic type variable T.
Comments suppressed due to low confidence (1)

olive/common/config_utils.py:317

  • Ensure that the type variable T is properly defined (e.g., via 'from typing import TypeVar' and 'T = TypeVar("T")') to support the generic type annotation.
config: Union[dict[str, Any], T, None],

@justinchuby justinchuby requested review from jambayk and Copilot April 22, 2025 20:13
Copilot

This comment was marked as outdated.

@justinchuby justinchuby enabled auto-merge (squash) April 23, 2025 00:23
@justinchuby justinchuby disabled auto-merge April 29, 2025 23:02
@titaiwangms
Copy link
Contributor

@justinchuby @jambayk Are we merging this soon? I am working on a PR to refactor the patch and dynamic_shapes to the latest version for better support on LLMs.

@justinchuby
Copy link
Contributor Author

I think the PR is ready. I would need some reviews

@justinchuby justinchuby requested a review from Copilot June 12, 2025 20:37
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the dynamo exporter and onnx conversion passes by reducing redundant disk IO and updating type annotations.

  • Updated test conditions for dynamo export based on torch versions
  • Changed type annotations in IoConfig to use Optional
  • Updated generic type hints in config_utils

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
test/unit_test/passes/onnx/test_conversion.py Adjusted test conditions with additional torch version checks
olive/model/config/io_config.py Updated type hints to use Optional for better type clarity and safety
olive/common/config_utils.py Modified function signature to use generic type T for configuration input
Comments suppressed due to low confidence (2)

test/unit_test/passes/onnx/test_conversion.py:36

  • [nitpick] Consider adding an inline comment to explain why skipping is conditioned on the torch version for Windows platforms, which can help future maintainers understand the rationale.
if platform.system() == "Windows" and use_dynamo_exporter and _torch_is_older_than("2.7.0"):

olive/model/config/io_config.py:37

  • [nitpick] Updating type annotations to Optional is a good improvement. It would be helpful to update the corresponding docstrings to reflect these changes.
input_shapes: Optional[list[list[int]]] = None

@justinchuby
Copy link
Contributor Author

=================================== FAILURES ===================================
____________________________ test_inc_quantization _____________________________
test/unit_test/passes/inc/test_inc_quantization.py:43: in test_inc_quantization
quantized_model = p.run(ov_model, output_folder)
/passes/_pass.py:242: in run
output_model = self._run_for_config(model, self.config, output_model_path)
***/passes/onnx/inc_quantization.py:536: in _run_for_config
if q_model.is_large_model:
E AttributeError: 'NoneType' object has no attribute 'is_large_model'

@jambayk
Copy link
Contributor

jambayk commented Aug 7, 2025

closing since it has become stale

@jambayk jambayk closed this Aug 7, 2025
@jambayk jambayk deleted the justinchu/dynamo-save branch August 7, 2025 21:03
@justinchuby justinchuby restored the justinchu/dynamo-save branch August 9, 2025 00:24
@justinchuby justinchuby reopened this Aug 9, 2025
@justinchuby
Copy link
Contributor Author

I will continue on this. Thanks

@justinchuby justinchuby marked this pull request as draft August 9, 2025 00:43
jambayk pushed a commit that referenced this pull request Aug 9, 2025
## Describe your changes

While working on #1765, I
realized that some typing annotations and error behavior can be
improved. I isolated the changes here as a separate PR.

## Checklist before requesting a review
- [ ] Add unit tests for this change.
- [ ] Make sure all tests can pass.
- [ ] Update documents if necessary.
- [x] Lint and apply fixes to your code by running `lintrunner -a`
- [ ] Is this a user-facing change? If yes, give a description of this
change to be included in the release notes.
- [ ] Is this PR including examples changes? If yes, please remember to
update [example
documentation](https://github.com/microsoft/Olive/blob/main/docs/source/examples.md)
in a follow-up PR.

## (Optional) Issue link

---------

Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby changed the title Refactor dynamo converter and ir.Model support [Replaced] Refactor dynamo converter and ir.Model support Aug 13, 2025
@justinchuby justinchuby changed the title [Replaced] Refactor dynamo converter and ir.Model support Refactor dynamo converter and ir.Model support Aug 13, 2025
commit 5450394
Merge: c7276ac 77f897e
Author: Justin Chu <[email protected]>
Date:   Fri Aug 15 13:10:27 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit c7276ac
Merge: ea436b6 6cdedb9
Author: Justin Chu <[email protected]>
Date:   Wed Aug 13 06:45:32 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit ea436b6
Author: Justin Chu <[email protected]>
Date:   Fri Aug 8 17:33:25 2025 -0700

    test

    Signed-off-by: Justin Chu <[email protected]>

commit dc90ff1
Merge: 11d49e0 c8718cd
Author: Justin Chu <[email protected]>
Date:   Fri Aug 8 17:31:04 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

    Signed-off-by: Justin Chu <[email protected]>

commit 11d49e0
Merge: 632b1cb 30aa1f9
Author: Xiaoyu <[email protected]>
Date:   Tue Jun 17 15:58:40 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 632b1cb
Author: Justin Chu <[email protected]>
Date:   Fri Jun 13 20:48:35 2025 -0700

    merge

commit 1ed33da
Author: Justin Chu <[email protected]>
Date:   Fri Jun 13 19:52:06 2025 -0700

    Update

commit d3a3ca1
Merge: 52504f5 8a847c2
Author: Justin Chu <[email protected]>
Date:   Fri Jun 13 19:43:24 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 52504f5
Merge: 5b500e8 d148eed
Author: Justin Chu <[email protected]>
Date:   Thu Jun 12 13:35:39 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 5b500e8
Author: Justin Chu <[email protected]>
Date:   Fri May 2 11:34:10 2025 -0700

    Update conversion.py

commit d8d0e94
Merge: 39e73e2 ebf8954
Author: Justin Chu <[email protected]>
Date:   Fri May 2 11:30:34 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 39e73e2
Merge: 11397fa fe0d4b5
Author: Justin Chu <[email protected]>
Date:   Wed Apr 30 15:54:01 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 11397fa
Merge: 0ac3491 245ef68
Author: Justin Chu <[email protected]>
Date:   Tue Apr 29 16:03:08 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 0ac3491
Merge: 28d4df0 1535aec
Author: Justin Chu <[email protected]>
Date:   Fri Apr 25 15:52:10 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 28d4df0
Author: Justin Chu <[email protected]>
Date:   Tue Apr 22 12:34:11 2025 -0700

    string_to_int_dim_params

commit ae1abf6
Author: Justin Chu <[email protected]>
Date:   Tue Apr 22 10:20:55 2025 -0700

    Implement string_to_int_dim_params

commit 5926cd4
Author: Justin Chu <[email protected]>
Date:   Tue Apr 22 10:09:39 2025 -0700

    Update tests

commit e018963
Merge: 2be4ad8 d9f73b5
Author: Justin Chu <[email protected]>
Date:   Mon Apr 21 19:44:36 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 2be4ad8
Author: Justin Chu <[email protected]>
Date:   Mon Apr 21 16:51:40 2025 -0700

    lint

commit 1f8372f
Merge: 2a1b6a0 86bd63e
Author: Justin Chu <[email protected]>
Date:   Mon Apr 21 16:48:48 2025 -0700

    Merge branch 'main' into justinchu/dynamo-save

commit 2a1b6a0
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 15:00:13 2025 -0700

    count size

commit fb4c19b
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 14:52:06 2025 -0700

    Update save_as_external_data

commit 3680472
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 14:17:31 2025 -0700

    Fix test

commit 2a87477
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 14:02:33 2025 -0700

    update

commit ce1824c
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 13:47:31 2025 -0700

    Fix external data handling

commit 3d5d42f
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 13:28:55 2025 -0700

    save model first

commit c4e6215
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 12:01:53 2025 -0700

    Refactor

commit 90052a1
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 11:53:55 2025 -0700

    Refactor

commit 405bca6
Author: Justin Chu <[email protected]>
Date:   Fri Apr 18 11:20:53 2025 -0700

    Update

commit f779277
Author: Justin Chu <[email protected]>
Date:   Thu Apr 17 17:56:46 2025 -0700

    Refactor ONNX model conversion return logic

commit 25ff541
Author: Justin Chu <[email protected]>
Date:   Thu Apr 17 17:55:13 2025 -0700

    Update olive/passes/onnx/conversion.py

commit 5017ad4
Author: Justin Chu <[email protected]>
Date:   Thu Apr 17 17:52:23 2025 -0700

    Rename `model_ir_to_olive_model` to `ir_model_to_olive_model`.

commit 73bdb33
Author: Justin Chu <[email protected]>
Date:   Thu Apr 17 17:50:09 2025 -0700

    Update OnnxScriptFusion

commit b885198
Author: Justin Chu <[email protected]>
Date:   Thu Apr 17 17:47:03 2025 -0700

    Implement model_ir_to_olive_model

commit 561f453
Author: Justin Chu <[email protected]>
Date:   Thu Apr 17 17:37:57 2025 -0700

    Refactor dynamo converter

Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby force-pushed the justinchu/dynamo-save branch from 5450394 to 2f9b942 Compare August 15, 2025 20:16
Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby marked this pull request as ready for review August 15, 2025 23:24
@justinchuby justinchuby requested a review from jambayk August 16, 2025 23:58
@justinchuby justinchuby merged commit 78f7586 into main Aug 18, 2025
19 checks passed
@justinchuby justinchuby deleted the justinchu/dynamo-save branch August 18, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants