Copilot commented on code in PR #19527:
URL: https://github.com/apache/tvm/pull/19527#discussion_r3213352364
##########
tests/python/relax/test_frontend_onnx.py:
##########
@@ -961,6 +961,80 @@ def test_scatter(axis: int, name: str, opset: int):
check_correctness(model, inputs={"indices": indices}, opset=opset)
[email protected](
+ "reduction, opset, data, indices, updates",
+ [
+ (
+ None,
+ 11,
+ np.array([[1, 2, 3], [4, 5, 6]], dtype="float32"),
+ np.array([[2, 0, 1], [1, 2, 0]], dtype="int64"),
+ np.array([[30, 10, 20], [50, 60, 40]], dtype="float32"),
+ ),
+ (
+ "none",
+ 18,
+ np.array([[1, 2, 3], [4, 5, 6]], dtype="float32"),
+ np.array([[2, 0, 1], [1, 2, 0]], dtype="int64"),
+ np.array([[30, 10, 20], [50, 60, 40]], dtype="float32"),
+ ),
Review Comment:
The new tests cover opset 11 default behavior and opset 16 add/mul, but they
don't exercise the opset 16 code path when the `reduction` attribute is omitted
(which should default to ONNX "none" and be normalized to Relax "update").
Adding an opset 16 case with `reduction=None` would guard the `_impl_v16`
defaulting behavior against regressions.
##########
python/tvm/relax/frontend/onnx/onnx_frontend.py:
##########
@@ -1140,6 +1140,19 @@ def _impl_v11(cls, bb, inputs, attr, params):
raise ValueError("Scatter is deprecated in ONNX 11")
+def _get_onnx_reduction(attr, valid_reductions: list[str]):
+ reduction = attr.get("reduction", None)
+ reduction = reduction or b"update"
+ if isinstance(reduction, bytes):
+ reduction = reduction.decode("utf-8")
+ reduction = "update" if reduction == "none" else reduction
+ assert reduction in valid_reductions, (
+ f"Only {valid_reductions} reductions are supported, but {reduction} is
gotten"
+ )
Review Comment:
_get_onnx_reduction uses an `assert` to validate a user-provided ONNX
attribute. Asserts can be disabled with Python optimizations (-O), which would
skip validation and potentially surface a harder-to-debug error deeper in
Relax. Prefer raising a ValueError (or TVMError) with a clear message (also
consider rewording "is gotten" to "got").
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
