Add an option to not inline a function when building the graph by justinchuby · Pull Request #2851 · microsoft/onnxscript

justinchuby · 2026-03-13T16:54:33Z

This pull request refactors the function call mechanism in the ONNX Script builder, clearly distinguishing between creating a single function call node (call) and inlining a function's body into the graph (call_inline). It also introduces a function registry for tracking functions used in a graph and updates tests to reflect these changes. The main improvements are in API clarity, function registration, and test coverage.

Function call API refactor and registration:

Introduced a clear distinction between call (creates a single function call node and registers the function) and call_inline (inlines the function body without registering the function) in GraphBuilder and OpBuilder, updating their signatures and documentation for clarity. [1] [2] [3]
Added a _functions dictionary to GraphBuilder to register called functions and a functions property for access. [1] [2]

Function call implementation changes:

Refactored the internal logic of call_op and related methods to support the new API, including explicit handling of domains, versions, output naming, and function registration. [1] [2] [3] [4] [5] [6]

Minor improvements:

Added a domain property to the OpBuilder class for easier access to the operator domain.

These changes improve the clarity, maintainability, and correctness of the ONNX Script builder's function call API.

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>

Copilot

Pull request overview

Adds support in the internal GraphBuilder to optionally not inline an ONNXScript function call, emitting a single function-call node and tracking the referenced function definitions for later export/attachment.

Changes:

Add Op.domain convenience property to expose an op’s opset domain.
Extend GraphBuilder.call/OpBuilder.call with _inline flag; when _inline=False, emit a single call node and register the callee in GraphBuilder.functions.
Add unit tests covering _inline=False behavior (single node emission, output renaming, prefix handling, function registration).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
onnxscript/_internal/values.py	Adds `Op.domain` property forwarding to the bound opset domain.
onnxscript/_internal/builder.py	Introduces function registry and `_inline` option to either inline or emit a function-call node.
onnxscript/_internal/builder_test.py	Adds tests verifying non-inlined call behavior and registration.

onnxscript/_internal/builder.py

onnxscript/_internal/builder_test.py

codecov · 2026-03-13T17:03:42Z

Codecov Report

❌ Patch coverage is 83.70370% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.90%. Comparing base (19e5284) to head (0a3efad).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
onnxscript/_internal/builder_test.py	84.15%	16 Missing ⚠️
onnxscript/_internal/builder.py	83.87%	3 Missing and 2 partials ⚠️
onnxscript/_internal/values.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2851      +/-   ##
==========================================
+ Coverage   71.86%   71.90%   +0.03%     
==========================================
  Files         239      239              
  Lines       29138    29252     +114     
  Branches     2875     2876       +1     
==========================================
+ Hits        20941    21033      +92     
- Misses       7219     7239      +20     
- Partials      978      980       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

onnxscript/_internal/builder.py

gramalingam · 2026-03-14T00:57:06Z

Not sure if it is better to have a separate function or an option (as in this PR). For now, this seems fine.

onnxscript/_internal/builder.py

            graph = function.graph
        elif isinstance(function, onnxscript.OnnxFunction):
            graph = function.graph()
+            function = function.function_ir


In general, to fix an unused local variable, either remove the assignment if the value isn’t needed, or, if the assignment is required for its side effects, keep the right‑hand expression and drop or rename the left‑hand variable to an “unused” name (for documentation).

Here, function = function.function_ir merely rebinds function to function.function_ir. That has no side effects and the new value is never used. The best behavior‑preserving fix is to delete this line in call_inline, unlike in call where function.function_ir is later required. We only change onnxscript/_internal/builder.py in the shown region: remove line 600 (function = function.function_ir) and keep everything else the same.

No new imports, methods, or definitions are required.

…args OpBuilder._call_op was inserting _domain, _version into the kwargs dict, but GraphBuilder.call_op expects domain, version, outputs as separate keyword arguments. This caused them to be treated as node attributes, breaking custom domain handling, schema lookup, type inference, shape inference, and output naming. Changes: - OpBuilder._call_op: pop _domain, _version, _outputs from kwargs and pass as separate keyword args to call_op - Remove _prefix from GraphBuilder.call and OpBuilder.call (only call_inline needs it) - Update test to use push_module/pop_module instead of _prefix on call

Copilot

Pull request overview

This PR enhances the ONNXScript IR builder’s function-call behavior by supporting both (a) inlining a function body into the current graph and (b) emitting a single function-call node while tracking the called function for downstream export/inspection.

Changes:

Added function tracking/registration in GraphBuilder and a new non-inlining function-call path.
Introduced a separate call_inline(...) path and updated/added tests to cover both behaviors.
Added a domain accessor on Op (covering OnnxFunction as a subclass) for easier domain access.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`onnxscript/_internal/builder.py`	Adds function registry, introduces non-inlined function call node creation, and refactors op-call argument handling.
`onnxscript/_internal/builder_test.py`	Updates existing call tests to `call_inline` and adds new tests for single-node function calls + registration.
`onnxscript/_internal/values.py`	Adds `domain` property (via `Op`) to expose opset domain.

Copilot · 2026-03-24T17:18:52Z

onnxscript/_internal/builder.py

    def call(
        self,
-        function,
+        function: ir.Function | onnxscript.OnnxFunction,
+        *args,
+        _outputs: int | Sequence[str | ir.Value] | None = None,
+        **kwargs,
+    ):
+        """Call a function as a single function node."""
+        if isinstance(function, ir.Function):
+            graph = function.graph


PR description says call/OpBuilder.call gained an _inline option with default inlining, but the implementation changes call() to non-inlining semantics and introduces a separate call_inline() API. This is a behavior-breaking change for existing users of call() and doesn’t match the described API. Either add the _inline flag (defaulting to current inlining behavior) or keep call() as inline and introduce a new explicit non-inlining method (or flag) for function-call nodes.

Copilot · 2026-03-24T17:18:52Z

onnxscript/_internal/builder.py


    def initializer(self, tensor: ir.TensorProtocol, name: str | None = None) -> ir.Value:
        return self._builder.initializer(tensor, name)



OpBuilder.functions is defined as a plain method, but GraphBuilder.functions is a @property and the PR description says the property is exposed on both builders. This is an API inconsistency that will surprise users (they’d have to call op.functions() instead of accessing op.functions). Consider making this a @property for parity with GraphBuilder.

Suggested change

@property

Copilot · 2026-03-24T17:18:53Z

onnxscript/_internal/builder.py

        inputs: Sequence[ir.Value | ir.TensorProtocol],
-        kwargs: dict[str, Any],
+        kwargs: dict[str, ir.Value | ir.TensorProtocol],
+        /,
+        domain: str = "",
+        version: int | None = None,
+        outputs: int | Sequence[str | ir.Value] = 1,


The call_op signature now annotates kwargs as dict[str, ir.Value | ir.TensorProtocol], but kwargs is used as a generic argument/attribute bag (passed into _partition_inputs_attributes / _cast_attributes) and can include non-value attribute types (ints, floats, strings, sequences, etc.). This type annotation is too narrow and will cause incorrect type-checking; it should stay as dict[str, Any] (or a dedicated attribute type alias) to match actual usage.

Copilot · 2026-03-24T17:18:53Z

onnxscript/_internal/builder.py

+        node = ir.node(
+            op_type=function.name,
+            inputs=args,


GraphBuilder.call() passes inputs=args directly into ir.node(...) without adapting inputs the way call_op does (e.g., promoting python scalars / TensorProtocol into initializers and ensuring all inputs are ir.Value). This will break callers that pass constants or tensors (which call_op explicitly supports). Consider running the same input-adaptation path used by call_op (at least _input_to_ir_value for each arg) before constructing the node.

Suggested change

node = ir.node(

op_type=function.name,

inputs=args,

# Adapt inputs similarly to call_op: promote constants/tensors to ir.Value.

adapted_args = [self._input_to_ir_value(arg) for arg in args]

node = ir.node(

op_type=function.name,

inputs=adapted_args,

Copilot · 2026-03-24T17:18:53Z

onnxscript/_internal/builder.py

+        node = ir.node(
+            op_type=function.name,
+            inputs=args,
+            attributes=kwargs or None,
+            outputs=output_values,
+            domain=function.domain,
+            name=self._qualify_node_name(function.name),


GraphBuilder.call() uses name=self._qualify_node_name(function.name), which will produce identical node names for repeated calls to the same function in the same scope (unlike call_op, which appends a monotonic counter). If node names are expected to be stable/unique for debugging or downstream tooling, incorporate the node index (e.g., ..._node_{count}) similarly to call_op.

Suggested change

node = ir.node(

op_type=function.name,

inputs=args,

attributes=kwargs or None,

outputs=output_values,

domain=function.domain,

name=self._qualify_node_name(function.name),

call_index = getattr(self, "_call_node_index", 0)

base_name = f"{function.name}_node_{call_index}"

self._call_node_index = call_index + 1

node = ir.node(

op_type=function.name,

inputs=args,

attributes=kwargs or None,

outputs=output_values,

domain=function.domain,

name=self._qualify_node_name(base_name),

xadupre · 2026-03-24T17:31:10Z

onnxscript/_internal/builder.py

+        kwargs: dict[str, ir.Value | ir.TensorProtocol],
+        /,
+        domain: str = "",
+        version: int | None = None,


do we expect different versions of the same opset?

We this is supported when building an ONNX IR model, even though we don't do this in practice

xadupre · 2026-03-24T17:32:16Z

onnxscript/_internal/builder.py

    def initializer(self, tensor: ir.TensorProtocol, name: str | None = None) -> ir.Value:
        return self._builder.initializer(tensor, name)

+    def functions(self) -> dict[ir.OperatorIdentifier, ir.Function]:


no property ?

Good catch thanks

xadupre · 2026-03-24T17:37:34Z

onnxscript/_internal/builder.py

+        node.metadata_props["pkg.onnxscript.name_scopes"] = repr(self._scope_names())
+
+        self.add_node(node)
+        self._functions[function.identifier()] = function


the same function can be called multiple times, so the

xadupre · 2026-03-24T17:38:17Z

onnxscript/_internal/builder.py

+        node.metadata_props["pkg.onnxscript.name_scopes"] = repr(self._scope_names())
+
+        self.add_node(node)
+        self._functions[function.identifier()] = function


I guess two different functions cannot have the same identifier.

justinchuby added 3 commits March 13, 2026 09:38

Allow functions to not be inlined

3cf4f55

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>

Add functions

ebbf30f

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>

Lint

b195105

Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>

github-project-automation bot added this to ONNX Script Review Board Mar 13, 2026

github-project-automation bot moved this to Todo in ONNX Script Review Board Mar 13, 2026

justinchuby added topic: Graph Builder topic: api labels Mar 13, 2026

justinchuby requested a review from Copilot March 13, 2026 16:54

Copilot started reviewing on behalf of justinchuby March 13, 2026 16:55 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

Potential fix for pull request finding

72639b1

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

gramalingam reviewed Mar 14, 2026

View reviewed changes

onnxscript/_internal/builder.py Outdated Show resolved Hide resolved

Justin Chu added 2 commits March 23, 2026 16:29

Merge branch 'main' into justinchu/function-inline

50322f9

Update impl

5a9e00b

github-code-quality bot found potential problems Mar 24, 2026

View reviewed changes

onnxscript/_internal/builder.py Dismissed Show dismissed Hide dismissed

github-advanced-security bot found potential problems Mar 24, 2026

View reviewed changes

justinchuby requested a review from Copilot March 24, 2026 17:13

Copilot started reviewing on behalf of justinchuby March 24, 2026 17:14 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

xadupre reviewed Mar 24, 2026

View reviewed changes


		def initializer(self, tensor: ir.TensorProtocol, name: str \| None = None) -> ir.Value:
		return self._builder.initializer(tensor, name)

Conversation

justinchuby commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

gramalingam commented Mar 14, 2026

Uh oh!

Uh oh!

Check notice

Copilot Autofix

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xadupre Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

justinchuby Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xadupre Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

justinchuby Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xadupre Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

xadupre Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinchuby commented Mar 13, 2026 •

edited

Loading

codecov bot commented Mar 13, 2026 •

edited

Loading