Skip to content

Fix Slice and Split derive_properties for dynamic axes#244

Merged
vortex-captain merged 1 commit into
mainfrom
reny/split_fix
Apr 3, 2026
Merged

Fix Slice and Split derive_properties for dynamic axes#244
vortex-captain merged 1 commit into
mainfrom
reny/split_fix

Conversation

@vortex-captain

Copy link
Copy Markdown
Contributor

Summary

  • Fix Slice derive_properties crash when data_shape contains symbolic dimension names (e.g. "time" from dynamic axes). Filters to fixed-shape axes only when computing starts_equal_shape and slice_all.
  • Fix Split derive_properties to use n_outputs from caller when split_value and attr_num_outputs are both absent.

When data_shape contains symbolic dimension names (e.g. "time"),
Slice's derive_properties crashed with ValueError trying to cast
the string to int64. Filter to fixed-shape axes only when computing
starts_equal_shape and slice_all properties.

Also fix Split derive_properties to use n_outputs from caller when
split_value and attr_num_outputs are both absent.
@vortex-captain vortex-captain requested a review from a team as a code owner April 3, 2026 05:44
@vortex-captain vortex-captain enabled auto-merge (squash) April 3, 2026 05:50
@DingmaomaoBJTU

Copy link
Copy Markdown
Collaborator

Code review

Found 1 issue:

  1. Pattern path injects incorrect n_outputs for variadic-output operators like Split

The node path correctly uses len(node.output) (the actual number of output edges on the matched node). The pattern path uses len(pattern_match.skeleton_match_result.pattern.get_schema().outputs), which returns the number of schema-defined output parameters — not the number of actual output edges. For the Split operator, the ONNX schema defines a single variadic output parameter, so get_schema().outputs always has length 1 regardless of how many ways the tensor is split. Any Split node matched via a pattern will receive n_outputs = 1, causing SplitInputGenerator.derive_properties to derive num_outputs = 1 instead of the real value.

https://github.com/microsoft/ModelKit/blob/1114c0c3da63e6970a1880ddff273a6f1c6bfeee/src/winml/modelkit/analyze/core/runtime_checker_query.py#L820-L824

The node path (used in get_query_conditions_for_node) may be the right model here — something like the number of matched output edges rather than the schema parameter count.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@vortex-captain

Copy link
Copy Markdown
Contributor Author

Code review

Found 1 issue:

  1. Pattern path injects incorrect n_outputs for variadic-output operators like Split

The node path correctly uses len(node.output) (the actual number of output edges on the matched node). The pattern path uses len(pattern_match.skeleton_match_result.pattern.get_schema().outputs), which returns the number of schema-defined output parameters — not the number of actual output edges. For the Split operator, the ONNX schema defines a single variadic output parameter, so get_schema().outputs always has length 1 regardless of how many ways the tensor is split. Any Split node matched via a pattern will receive n_outputs = 1, causing SplitInputGenerator.derive_properties to derive num_outputs = 1 instead of the real value.

https://github.com/microsoft/ModelKit/blob/1114c0c3da63e6970a1880ddff273a6f1c6bfeee/src/winml/modelkit/analyze/core/runtime_checker_query.py#L820-L824

The node path (used in get_query_conditions_for_node) may be the right model here — something like the number of matched output edges rather than the schema parameter count.

🤖 Generated with Claude Code

  • If this code review was useful, please react with 👍. Otherwise, react with 👎.

Matching subgraph patterns with variadic outputs is out of scope of pattern matcher, as it complicates things tremendously and has no practical use cases for now. The current conditions["n_outputs"] works properly in the current scope.

@vortex-captain vortex-captain merged commit f6f9f82 into main Apr 3, 2026
9 checks passed
@vortex-captain vortex-captain deleted the reny/split_fix branch April 3, 2026 06:29
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.

2 participants