spox icon indicating copy to clipboard operation
spox copied to clipboard

If node adaptation fails when node outside subgraph is referenced.

Open adityagoel4512 opened this issue 11 months ago • 3 comments

The described error can be reproduced with the following example:

import spox.opset.ai.onnx.v19 as op19
import spox.opset.ai.onnx.v18 as op18
from spox import Tensor, Var, argument, build
import onnx
import numpy
import pytest

def test_if_adaptation_const():
    sq = op19.const(1.1453, dtype=numpy.float32)
    b = argument(Tensor(numpy.float32, ("N",)))
    cond = op18.equal(sq, b)
    out = op18.if_(cond,
        then_branch=lambda: [sq],
        else_branch=lambda: [sq])
    build({"b": b}, {"out": out[0]})

Spox (correctly) attempts to update the opset of the if node from 18 to 19, but in creating a singleton model fails to add the output of the const as an input of the singleton since this is not an "input" of the If node.

tests/test_adapt.py:131:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
src/spox/_public.py:117: in build
    return graph.to_onnx_model()
src/spox/_graph.py:412: in to_onnx_model
    self.to_onnx(concrete=concrete),
src/spox/_graph.py:343: in to_onnx
    node_protos = itertools.chain.from_iterable(self.get_adapted_nodes().values())
src/spox/_graph.py:281: in get_adapted_nodes
    best_effort = adapt_best_effort(
src/spox/_adapt.py:142: in adapt_best_effort
    adapted = adapt_node(
src/spox/_adapt.py:60: in adapt_node
    onnx.checker.check_model(source_model, full_check=True)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

model = ir_version: 9
graph {
  node {
    input: "Equal_0_C"
    output: "If_0_outputs_0"
    name: "If_0"
    op_type: "If"
..._type {
        elem_type: 1
        shape {
        }
      }
    }
  }
}
opset_import {
  domain: ""
  version: 16
}

full_check = True, skip_opset_compatibility_check = False

    def check_model(
        model: ModelProto | str | bytes | os.PathLike,
        full_check: bool = False,
        skip_opset_compatibility_check: bool = False,
    ) -> None:
        """Check the consistency of a model. An exception is raised if the test fails.

        Args:
            model: Model to check.
            full_check: If True, the function also checks for shapes that can be inferred.
            skip_opset_compatibility_check: If True, the function skips the check for
                opset compatibility.
        """
        # If model is a path instead of ModelProto
        if isinstance(model, (str, os.PathLike)):
            C.check_model_path(os.fspath(model), full_check, skip_opset_compatibility_check)
        else:
            protobuf_string = (
                model if isinstance(model, bytes) else model.SerializeToString()
            )
            # If the protobuf is larger than 2GB,
            # remind users should use the model path to check
            if sys.getsizeof(protobuf_string) > MAXIMUM_PROTOBUF:
                raise ValueError(
                    "This protobuf of onnx model is too large (>2GB). Call check_model with model path instead."
                )
>           C.check_model(protobuf_string, full_check, skip_opset_compatibility_check)
E           RuntimeError: Nodes in a graph must be topologically sorted, however input 'Constant_0_output' of node:
E           name: If_0_else_branch__Introduce_0_id0 OpType: Identity
E            is not output of any previous nodes.
E
E           ==> Context: Bad node spec for node. Name: If_0 OpType: If

../../micromamba/envs/spox/lib/python3.12/site-packages/onnx/checker.py:148: RuntimeError

Spox should either bail gracefully when it cannot adapt a node (this is a best effort thing anyway) or should support this behaviour.

adityagoel4512 avatar Mar 11 '24 17:03 adityagoel4512