Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If node adaptation fails when node outside subgraph is referenced. #145

Open
adityagoel4512 opened this issue Mar 11, 2024 · 3 comments
Open
Assignees

Comments

@adityagoel4512
Copy link
Member

adityagoel4512 commented Mar 11, 2024

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 adityagoel4512 changed the title Node adaptation fails when composing inlined model with node with subgraph. If node adaptation fails when node outside subgraph is referenced. Mar 11, 2024
@aivanoved aivanoved self-assigned this May 14, 2024
aivanoved added a commit that referenced this issue May 29, 2024
@aivanoved
Copy link
Contributor

#155 adds test which stand in contrast to this issue

I cannot reproduce this issue anymore, unfortunately I can't prove that the promotion will always happen as required either

@adityagoel4512
Copy link
Member Author

It currently bails node adaptation - see #146.

@aivanoved
Copy link
Contributor

The question of why the node gets adapted correctly still stands, though

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

No branches or pull requests

2 participants