-
Notifications
You must be signed in to change notification settings - Fork 93
[Do Not Merge]Migrate onnxscript converter to use onnx ir #2706
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2706 +/- ##
==========================================
+ Coverage 70.04% 70.10% +0.06%
==========================================
Files 226 226
Lines 27177 27132 -45
Branches 2734 2739 +5
==========================================
- Hits 19036 19022 -14
+ Misses 7197 7165 -32
- Partials 944 945 +1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
Signed-off-by: Ganesan Ramalingam <[email protected]>
| pass | ||
| value.meta["typeinfo"] = typeinfo | ||
|
|
||
|
|
Check notice
Code scanning / CodeQL
Empty except Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
To fix this issue, the empty except AttributeError: block should be replaced with exception handling that does something useful. The most appropriate fix is to log a warning (using the module's logger) indicating that an AttributeError was encountered while setting the type info, which helps diagnose and debug the cause without terminating program execution. This must be done in the function set_type_info in onnxscript/converter.py. No new methods or imports are required, as the logger logger is already defined and available in the module. The fix is local: replace pass with a logging call in the specified region.
-
Copy modified lines R122-R125
| @@ -119,8 +119,10 @@ | ||
| type_and_shape = ir.from_proto(typeinfo.to_type_proto()) | ||
| value.type = type_and_shape.type | ||
| value.shape = type_and_shape.shape | ||
| except AttributeError: | ||
| pass | ||
| except AttributeError as ex: | ||
| logger.warning( | ||
| "Failed to set type and shape info due to AttributeError in set_type_info: %s", ex | ||
| ) | ||
| value.meta["typeinfo"] = typeinfo | ||
|
|
||
|
|
| type_and_shape = ir.from_proto(typeinfo.to_type_proto()) | ||
| value.type = type_and_shape.type | ||
| value.shape = type_and_shape.shape | ||
| except AttributeError: |
Check notice
Code scanning / CodeQL
Empty except Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
The best fix is to properly handle the exception by notifying the user or logging the error, so that missing or erroneous type information does not go unnoticed. At minimum, add a logging statement at the appropriate level (e.g., warning or error), describing what happened and including useful contextual information. This way, if an AttributeError is raised, it is recorded and is much less likely to go undetected.
Specifically, edit the file onnxscript/irbuilder.py, in the function set_type_info, replacing the empty except AttributeError: block (lines 130–131) with an except block that logs the exception with a helpful message, including details about the value and typeinfo involved.
No new imports are needed since logging is already imported, and the logger onnxscript is already defined as logger.
-
Copy modified lines R130-R136
| @@ -127,8 +127,13 @@ | ||
| type_and_shape = ir.from_proto(typeinfo.to_type_proto()) | ||
| value.type = type_and_shape.type | ||
| value.shape = type_and_shape.shape | ||
| except AttributeError: | ||
| pass | ||
| except AttributeError as e: | ||
| logger.warning( | ||
| "Failed to set type and shape on IR value '%s' with typeinfo '%s': %s", | ||
| getattr(value, "name", "<unknown>"), | ||
| repr(typeinfo), | ||
| e, | ||
| ) | ||
| value.meta["typeinfo"] = typeinfo | ||
|
|
||
|
|
Signed-off-by: Ganesan Ramalingam <[email protected]>
|
|
||
|
|
||
| def select_ir_version(version: int, domain: str = "") -> int: | ||
| """Selects a suitable ONNX ir_version for a given opset version.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably default the minimal version to a more recent one, like v10, such that the generated model is compatible with newer tool chains (metadata)? This can also be a good default to push for adoption
| """Selects a suitable ONNX ir_version for a given opset version.""" | ||
| if domain == "": | ||
| domain = "ai.onnx" | ||
| if (domain, version) not in onnx.helper.OP_SET_ID_VERSION_MAP: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider adding inline noqas for TID251 so we allow the helper module in a case by case basis. There is also a possibility that it is not needed.
| onnx.helper.make_opsetid(domain, version) for domain, version in opsets.items() | ||
| ] | ||
|
|
||
| return onnx.helper.make_model( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why we do this instead of converting from an IR Model?
Migrate onnxscript converter to use onnx ir.
PLEASE DO NOT MERGE YET. May want to wait for release 0.6