-
Notifications
You must be signed in to change notification settings - Fork 97
Update SplitToSequence in constant folding #2542
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
Conversation
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.
Pull Request Overview
This PR updates the SplitToSequence function in constant folding to handle cases where the Split input has no constant value but has shape information that can be used to determine the number of outputs. It also refactors the constant folding pass to improve its API and reduce verbosity in logging.
Key changes:
- Enhanced
split_to_sequenceto use shape information when constant values aren't available - Replaced
should_foldcallback parameter withalways_fold_opscollection for better API design - Streamlined logging and code structure in the constant folding pass
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| onnxscript/optimizer/_constant_folding.py | Updated SplitToSequence logic, refactored FoldConstantsPass API, improved logging |
| onnxscript/function_libs/torch_lib/ops/core.py | Added trace_only flag to aten_split_with_sizes function |
| always_fold_ops: Collection of op types that should always be folded. | ||
| For ops from the default opset, only op_type is neede (e.g. "Transpose"), |
Copilot
AI
Sep 4, 2025
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.
There's a typo in the docstring: 'neede' should be 'needed'.
| always_fold_ops: Collection of op types that should always be folded. | |
| For ops from the default opset, only op_type is neede (e.g. "Transpose"), | |
| For ops from the default opset, only op_type is needed (e.g. "Transpose"), |
| always_fold_ops: A collection of op types that should always be folded, | ||
| regardless of their input or output sizes. For ops from the default opset, | ||
| only op_type is neede (e.g. "Transpose"), otherwise specify the domain |
Copilot
AI
Sep 4, 2025
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.
There's a typo in the docstring: 'neede' should be 'needed'.
| always_fold_ops: A collection of op types that should always be folded, | |
| regardless of their input or output sizes. For ops from the default opset, | |
| only op_type is neede (e.g. "Transpose"), otherwise specify the domain | |
| only op_type is needed (e.g. "Transpose"), otherwise specify the domain |
| if logger.isEnabledFor(logging.DEBUG): | ||
| logger.debug( | ||
| "Skipping constant folding for node %s because it has non-constant inputs", |
Copilot
AI
Sep 4, 2025
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.
The debug message format string expects one placeholder (%s) but receives two arguments: the node and the list of input names. Either add another %s placeholder or combine the arguments.
| if logger.isEnabledFor(logging.DEBUG): | |
| logger.debug( | |
| "Skipping constant folding for node %s because it has non-constant inputs", | |
| "Skipping constant folding for node %s because it has non-constant inputs: %s", |
| shape_inference=onnx_shape_inference, | ||
| input_size_limit=input_size_limit, | ||
| output_size_limit=output_size_limit, | ||
| should_fold=should_fold, |
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.
This should be reverted
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2542 +/- ##
==========================================
- Coverage 69.99% 63.26% -6.73%
==========================================
Files 216 216
Lines 26074 26073 -1
Branches 2618 2619 +1
==========================================
- Hits 18250 16496 -1754
- Misses 6921 8745 +1824
+ Partials 903 832 -71 ☔ View full report in Codecov by Sentry. |
Splitinput (SymbolicTensor) could have no const_value, but with shape that gives us information of how many outputs an op.Split should return.