Skip to content

Support symbolic shape inference #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Support symbolic shape inference #82

wants to merge 1 commit into from

Conversation

inisis
Copy link

@inisis inisis commented Jun 15, 2025

Closes #57

  1. Migrate symbolic shape inference code from onnxruntime;
  2. Prefer symbolic shape inference over onnx built-in shape inference.

@inisis inisis requested review from titaiwangms and a team as code owners June 15, 2025 17:04
@justinchuby justinchuby self-assigned this Jun 15, 2025
Copy link

codecov bot commented Jun 15, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 57.07%. Comparing base (24096c6) to head (bd48c7f).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
src/onnx_ir/passes/common/shape_inference.py 60.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (24096c6) and HEAD (bd48c7f). Click for more details.

HEAD has 9 uploads less than BASE
Flag BASE (24096c6) HEAD (bd48c7f)
18 9
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #82       +/-   ##
===========================================
- Coverage   73.48%   57.07%   -16.41%     
===========================================
  Files          37       38        +1     
  Lines        4507     6272     +1765     
  Branches      906     1299      +393     
===========================================
+ Hits         3312     3580      +268     
- Misses        865     2317     +1452     
- Partials      330      375       +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -15,6 +15,7 @@

import onnx_ir as ir
from onnx_ir.passes.common import _c_api_utils
from .symbolic_shape_infer import SymbolicShapeInference

Check warning

Code scanning / lintrunner

RUFF/TID252 Warning

Prefer absolute imports over relative imports.
See https://docs.astral.sh/ruff/rules/relative-imports
@inisis inisis marked this pull request as draft June 16, 2025 08:06
@justinchuby
Copy link
Member

justinchuby commented Jun 16, 2025

Thank you! A few suggestions:

  1. We would like the src/onnx_ir/passes/common/symbolic_shape_infer.py module to be refactored and use only onnx_ir methods for graph manipulation. This would be the most important and time consuming part. But it's a good opportunity to do the necessary refactorings. You may start by removing imports from onnx and see how we can do things with onnx_ir.
    a. We can consider using the onnx test data for validating the new logic. It can be done in a separate PR.
  2. The inference methods need to be robust and tested. Is there a way to explicitly specify in each method the supported opset versions? It would be preferable if relevant information are written in a declarative manner.
  3. Because the module is big, it may be helpful if you create a root directory _symbolic_shape_inference to host the code and break the logic into smaller files. When the logic matures we can expose the symbolic_shape_inference module publically.
    a. It's preferable if you can modularize the original big chunk of logic so that they are individually testable and maintainable.
  4. For the pass we would like to keep the old behavior. I suggest creating a new in-place SymbolicShapeInferencePass that will call the new logic, so users have a choice.

@justinchuby
Copy link
Member

Checking in - were you able to make some progress? Thanks!

@inisis
Copy link
Author

inisis commented Jun 27, 2025

@justinchuby Sorry for the late reply, I'll finish it by end of this week.

@inisis
Copy link
Author

inisis commented Jun 29, 2025

Hi @justinchuby, I may not have enough bandwidth to do the refactoring, It's really time consuming😔.

@justinchuby
Copy link
Member

Thanks for letting me know! I am creating #117 to continue.

@inisis inisis closed this Jun 30, 2025
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.

Integrate symbolic shape inference
2 participants