Skip to content

Keep symbolic arithmetic inference scoped to dynamic XLA - #46

Merged
stevenvar merged 1 commit into
for-serving-2.20from
steven.fix-graph-properties-dynamic-tests
Aug 14, 2026
Merged

Keep symbolic arithmetic inference scoped to dynamic XLA#46
stevenvar merged 1 commit into
for-serving-2.20from
steven.fix-graph-properties-dynamic-tests

Conversation

@stevenvar

@stevenvar stevenvar commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Summary

TensorFlow-side dynamic value inference has a symbolic path for shape-like values produced by Add, Sub, Mul, and Div. That path previously ran even when dynamic value inference was disabled, and it returned early when symbolic propagation could not handle the available operands. This could prevent GraphProperties from continuing through its normal value-inference logic.

This change:

  • runs symbolic arithmetic propagation only when dynamic value inference is enabled;
  • returns early only after symbolic propagation succeeds;
  • otherwise falls through to TensorFlow's existing inference path.

Ordinary GraphProperties behavior is therefore preserved when dynamic value inference is disabled, while dynamic XLA inference can still propagate compatible symbolic shape values.

Tests

  • //tensorflow/core/grappler/costs:graph_properties_test
  • all 63 tests pass

@stevenvar
stevenvar force-pushed the steven.fix-graph-properties-dynamic-tests branch 2 times, most recently from 5df6c3d to 95e9f58 Compare August 13, 2026 09:43
@stevenvar stevenvar changed the title Fix GraphProperties symbolic inference regressions Keep symbolic arithmetic inference scoped to dynamic XLA Aug 13, 2026
@stevenvar
stevenvar force-pushed the steven.fix-graph-properties-dynamic-tests branch from 95e9f58 to b05772f Compare August 13, 2026 10:16
@stevenvar
stevenvar force-pushed the steven.fix-graph-properties-dynamic-tests branch 2 times, most recently from 59d953d to bba27c3 Compare August 14, 2026 08:59
@stevenvar
stevenvar force-pushed the steven.fix-graph-properties-dynamic-tests branch 3 times, most recently from 73775f2 to 911d97f Compare August 14, 2026 10:17
Comment on lines +1770 to +1778
if (op == "Sub")
TF_RETURN_IF_ERROR(ic->Subtract(da, db, &r));
else if (op == "Add")
TF_RETURN_IF_ERROR(ic->Add(da, db, &r));
else if (op == "Mul")
TF_RETURN_IF_ERROR(ic->Multiply(da, db, &r));
else
TF_RETURN_IF_ERROR(
ic->Divide(da, db, /*evenly_divisible=*/false, &r));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This still does not fall back when symbolic arithmetic fails 😒. Shape-value tensors can validly produce -1, for example, [1] - [2], but InferenceContext::Subtract interprets these values as dimensions and returns InvalidArgument. Because TF_RETURN_IF_ERROR propagates that error, GraphProperties fails instead of reaching normal value inference, contrary to the new fallback behaviour. Please treat arithmetic failure as “symbolic propagation does not apply” (or use tensor-value semantics) and add a regression test covering negative results and unknown-shape sentinels.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Thank you for your review. As you correctly said, -1 is invalid as a concrete dimension but remains a perfectly valid integer tensor value. The symbolic path now steps aside instead of failing GraphProperties, and regression tests ensure this distinction should not require another reminder.

@stevenvar
stevenvar force-pushed the steven.fix-graph-properties-dynamic-tests branch from 911d97f to 6aa420b Compare August 14, 2026 10:24

@Guillermo-Callaghan Guillermo-Callaghan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@stevenvar
stevenvar merged commit d415a8f into for-serving-2.20 Aug 14, 2026
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