Skip to content

Commit 613c355

Browse files
authored
[onnx_importer] Disambiguate empty string: optional none vs tensor name (#4551)
Fixes #4550 The NodeImporter cached torch.constant.none under _nv_map[""], matching ONNX's convention that an empty string in Node.input denotes an omitted optional input. Some producers (e.g. Microsoft SkipSimplifiedLayerNormalization) also bind real intermediate results to outputs whose names are the empty string. Each such output overwrote _nv_map[""], so later nodes that use "" for omitted optionals (e.g. GroupQueryAttention's trailing inputs) incorrectly received those tensor SSA values instead of torch.constant.none. Behavior changes: - Cache the shared none value under _OPTIONAL_NONE_CACHE_KEY instead of "". - When resolving node inputs, treat input_name == "" as omitted optional: append get_none() and an empty onnx.TypeProto without indexing _nv_map[""]. - Register outputs named "" under unique keys __torch_mlir_onnx_importer_anon_<n> so multiple anonymous outputs do not overwrite each other. Adds test/python/onnx_importer/test_empty_string_optional_inputs.py: minimal Identity -> custom op graph where optional inputs are "" and must import as %none operands, not tensor values stored under "". Symptom fixed: GroupQueryAttention previously showed duplicated operands such as (%10#2, %10#2, %10#2) instead of (%none, %none, %none) for optional slots.
1 parent c852e84 commit 613c355

2 files changed

Lines changed: 83 additions & 4 deletions

File tree

python/torch_mlir/extras/onnx_importer.py

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,7 @@ class NodeImporter:
239239
"_p",
240240
"_b",
241241
"_nv_map",
242+
"_none_value",
242243
]
243244

244245
def __init__(
@@ -259,6 +260,7 @@ def __init__(
259260
self._p = parent_op
260261
self._b = block
261262
self._nv_map: Dict[str, Value] = {}
263+
self._none_value: Optional[Value] = None
262264

263265
@classmethod
264266
def define_function(
@@ -366,8 +368,8 @@ def import_all(self, func=True):
366368
Operation.create(name="torch.operator_terminator", operands=outputs)
367369

368370
def get_none(self):
369-
if "" in self._nv_map:
370-
return self._nv_map[""]
371+
if self._none_value is not None:
372+
return self._none_value
371373

372374
with InsertionPoint(self._b), Location.name("onnx_importer.none"):
373375
nne = Operation.create(
@@ -376,7 +378,7 @@ def get_none(self):
376378
operands=[],
377379
attributes={},
378380
).results[0]
379-
self._nv_map[""] = nne
381+
self._none_value = nne
380382
return nne
381383

382384
def import_node(self, node: onnx.NodeProto):
@@ -396,6 +398,12 @@ def import_node(self, node: onnx.NodeProto):
396398
input_values = []
397399
input_type_protos = []
398400
for input_name in node.input:
401+
# ONNX uses the empty string for omitted optional inputs; it must not
402+
# be confused with _nv_map[""], which may hold a real tensor named "".
403+
if input_name == "":
404+
input_values.append(self.get_none())
405+
input_type_protos.append(onnx.TypeProto())
406+
continue
399407
try:
400408
input_values.append(self._nv_map[input_name])
401409
# Missing optional arguments will have empty types
@@ -447,7 +455,8 @@ def import_node(self, node: onnx.NodeProto):
447455
self.import_regions(node.attribute, custom_op)
448456

449457
for output_name, output_value in zip(output_names, custom_op.results):
450-
self._nv_map[output_name] = output_value
458+
if output_name != "":
459+
self._nv_map[output_name] = output_value
451460

452461
def import_attributes(self, onnx_attrs: List[onnx.AttributeProto]):
453462
attrs = {}
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
2+
# See https://llvm.org/LICENSE.txt for license information.
3+
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
4+
5+
# RUN: %PYTHON %s
6+
7+
"""Regression for NodeImporter: ONNX input name '' means omitted optional.
8+
9+
The importer must not conflate that with _nv_map[""] when an earlier node binds
10+
a real tensor to the empty-string output name (see onnx_importer empty-string
11+
collision fix).
12+
"""
13+
14+
import unittest
15+
16+
import onnx
17+
from onnx import TensorProto, helper
18+
19+
from _torch_mlir_config import configure_context, ir, onnx_importer
20+
21+
22+
def _minimal_collision_model() -> onnx.ModelProto:
23+
"""Identity writes to output ""; second node lists '', '' as omitted inputs."""
24+
inp = helper.make_tensor_value_info("x", TensorProto.FLOAT, [1, 2])
25+
out = helper.make_tensor_value_info("y", TensorProto.FLOAT, [1, 2])
26+
n1 = helper.make_node("Identity", ["x"], [""])
27+
n2 = helper.make_node(
28+
"ReproEmptyStringCollision",
29+
["x", "", ""],
30+
["y"],
31+
domain="zmc.repro",
32+
)
33+
graph = helper.make_graph([n1, n2], "g", [inp], [out])
34+
return helper.make_model(
35+
graph,
36+
opset_imports=[
37+
helper.make_opsetid("", 21),
38+
helper.make_opsetid("zmc.repro", 1),
39+
],
40+
)
41+
42+
43+
class EmptyStringOptionalInputsTest(unittest.TestCase):
44+
def test_optional_slots_use_constant_none_not_prior_tensor(self):
45+
model = _minimal_collision_model()
46+
ctx = ir.Context()
47+
configure_context(ctx)
48+
mi = onnx_importer.ModelInfo(model)
49+
m = mi.create_module(context=ctx).operation
50+
onnx_importer.NodeImporter.define_function(mi.main_graph, m).import_all()
51+
asm = m.get_asm()
52+
lines = [
53+
ln.strip() for ln in asm.splitlines() if "ReproEmptyStringCollision" in ln
54+
]
55+
self.assertEqual(
56+
len(lines),
57+
1,
58+
msg="expected exactly one onnx.ReproEmptyStringCollision operator line",
59+
)
60+
line = lines[0]
61+
# Correct: trailing optionals are torch.constant.none uses (printed as %none, %none).
62+
self.assertGreaterEqual(
63+
line.count("%none"),
64+
2,
65+
msg=f"expected at least two %none operands for omitted inputs, got:\n{line}",
66+
)
67+
68+
69+
if __name__ == "__main__":
70+
unittest.main()

0 commit comments

Comments
 (0)