Unify BuilderBase and GraphBuilder: shared call_op with feature flags

[gramalingam]

Summary

Extends the BuilderBase / TapeBuilder infrastructure (from PR #2905) so that GraphBuilder inherits from BuilderBase, sharing a single call_op() implementation gated by feature flags.

Changes

Class rename and API merge

• Renamed OpBuilderBase → BuilderBase (no backward-compat alias needed — not yet released)
• Merged the separate op() and _make_node() methods into a single op(op_type, *args, **kwargs) ergonomic API
• Promoted _create_op to BuilderBase.call_op() — the single core node-creation method

Feature flags (BuilderFeature enum)

• SCHEMA_PARTITION — partition positional args into inputs vs attributes using op schema
• CAST_INPUTS — promote scalars to tensors, match type variables
• CAST_ATTRIBUTES — cast attributes using schema info (stub for future)
• INFER_SHAPES — run shape inference after node creation
• CONSTANT_PROPAGATION — run constant propagation after node creation
• FULL = all of the above (used by GraphBuilder)
• NONE = no extra processing (used by TapeBuilder for rewriter/optimizer/version-converter)

GraphBuilder inherits from BuilderBase

• GraphBuilder(BuilderBase) with features=BuilderFeature.FULL
• call_op() is inherited directly — no override needed
• Default hook implementations (_get_schema, _partition_inputs_attributes, _cast_inputs, _cast_attributes) live in BuilderBase
• GraphBuilder only overrides hooks needing graph-specific state:
  • _input_to_ir_value — constant promotion with caching
  • _generate_node_name / _adapt_outputs — qualified naming
  • _post_create_node — constant propagation + shape inference
  • _annotate_node — scope metadata

Exports

• BuilderFeature exported from onnxscript package

Testing

• All 559 rewriter/optimizer/version-converter tests pass
• All 90 GraphBuilder tests pass
• All 15 TapeBuilder/context tests pass

[codecov]

Codecov Report

❌ Patch coverage is 88.54167% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.63%. Comparing base (029441f) to head (d3b52dd).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
onnxscript/_internal/tape_builder.py	86.80%	14 Missing and 5 partials ⚠️
onnxscript/_internal/builder.py	91.89%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2910      +/-   ##
==========================================
+ Coverage   72.61%   72.63%   +0.01%     
==========================================
  Files         259      259              
  Lines       31597    31651      +54     
  Branches     2973     2980       +7     
==========================================
+ Hits        22945    22990      +45     
- Misses       7643     7653      +10     
+ Partials     1009     1008       -1     

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

[Copilot]

Pull request overview

This PR unifies the op-building infrastructure by introducing a shared BuilderBase.call_op() pipeline with feature flags, and refactors GraphBuilder/TapeBuilder and call sites to use the new op(op_type, *args, **kwargs) API shape.

Changes:

• Renames OpBuilderBase → BuilderBase, adds BuilderFeature flags, and consolidates node creation into BuilderBase.call_op() in tape_builder.py.
• Refactors GraphBuilder to inherit from BuilderBase and implement the storage + graph-specific hooks while using feature-flagged post-processing.
• Updates rewriter/optimizer/version-converter call sites and exports (onnxscript.__init__) to use the new builder names and op() calling convention.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
onnxscript/version_converter/_version_converter.py	Updates VC context alias/import to BuilderBase.
onnxscript/rewriter/rules/common/_remove_optional_bias.py	Updates explicit op construction to new op.op(op_type, *inputs, **attrs) style.
onnxscript/rewriter/rules/common/_min_max_to_clip.py	Updates explicit op construction to pass inputs positionally.
onnxscript/rewriter/rules/common/_fuse_pad_into_conv.py	Updates explicit op construction to new positional inputs + _domain/_name reserved kwargs.
onnxscript/rewriter/rules/common/_fuse_batchnorm.py	Updates explicit op construction to positional inputs + expanded attributes mapping.
onnxscript/rewriter/_context.py	Re-exports/aliases updated to BuilderBase + TapeBuilder.
onnxscript/rewriter/_context_test.py	Updates tests to validate new op.op(...) calling convention (including **node.attributes).
onnxscript/optimizer/_constant_folding.py	Updates optimizer context alias/import to BuilderBase.
onnxscript/_internal/tape_builder.py	Introduces BuilderFeature, BuilderBase with shared call_op() and hook-based processing; TapeBuilder updated accordingly.
onnxscript/_internal/builder.py	Refactors GraphBuilder to inherit BuilderBase and provide graph-backed implementations/hooks; removes now-duplicated call_op implementation.
onnxscript/init.py	Public exports updated to include BuilderBase and BuilderFeature (and drop OpBuilderBase).

[titaiwangms]

Review

Synthesized from a multi-reviewer pass (correctness, adversarial, readability). In-scope findings only, sorted by severity.

�� Critical / Bug

1. GraphBuilder.op is a @property, shadowing BuilderBase.op() method — LSP violation
builder.py / tape_builder.py. BuilderBase.op() is a method (b.op("Relu", x)); GraphBuilder.op is a property returning an OpBuilder (b.op.Relu(x)). The calling convention silently flips between base and subclass. Any code typed against RewriterContext = BuilderBase and handed a GraphBuilder will fail at runtime. Rename the property (e.g. ops / default_op) or document the difference loudly in BuilderBase.

2. OpBuilder._call_op drops _name
builder.py. Pops _domain, _version, _outputs but not _name. So b.op.Add(x, y, _name="foo") either raises an unexpected-kwarg error (schema-aware ops) or creates a bogus attribute named _name (custom ops). Pop _name and forward as name= to builder.call_op(...).

🟠 Major

3. Silent breakage of the old op.op(inputs=..., attributes=..., domain=..., name=...) signature
tape_builder.py. The new signature ignores those kwargs and routes them into **kwargs as ONNX attributes — producing a wrong graph with no error. Internal rules were migrated, but third-party rewrite rule authors will silently lose. The PR body says "not yet released, no compat alias needed" — please double-check that's true for the public rewriter/version-converter extension API before merging. If there's any external surface, either add a compat shim (detect inputs=/attributes= and remap with a DeprecationWarning) or a CHANGELOG migration note.

4. OpBuilderBase removed with no alias
Same "not released" rationale. Trivial to keep OpBuilderBase = BuilderBase as a one-line transitional alias.

5. Missing tests for the new BuilderFeature matrix and multi-output contract
Only default TapeBuilder() behavior is exercised. Add focused tests for partial flag combinations (e.g. SCHEMA_PARTITION only, CAST_INPUTS only) and for the new multi-output return shape (op.op("Split", x, _outputs=3) returning Sequence[ir.Value] vs the old single-ir.Value contract).

🟡 Minor

6. Name-determinism shift. _cast_inputs now runs before _adapt_outputs / _generate_node_name, so promoted CastLike/Constant sub-nodes advance graph.num_nodes() before the outer node names itself. Old: Add_0 / Add_node_0. New: Add_1 / Add_node_1. Won't corrupt graphs but may break snapshot tests or debug-name expectations. Either snapshot the count up-front (pass it to both hooks) or accept-and-document the new contract.

7. Recursive CastLike loses opset context. _input_to_ir_value calls self.call_op("CastLike", ..., domain="", version=None) instead of going through the graph-bound op builder. For graphs targeting older opsets, schema lookup will miss. Suggest a _make_cast_like(value, like_type) hook so GraphBuilder can supply its ambient version.

8. _domain / _version / _outputs / _name underscore convention is undocumented. Add one sentence to op()'s docstring explaining the prefix exists to avoid clashing with ONNX attribute names passed via **kwargs. Non-obvious to a first-time rule author.

9. Duplicated helpers. _dtype_suffix and _constant_name are essentially identical in builder.py and tape_builder.py. Import from one source to avoid drift.

10. BuilderFeature.any_schema_feature is a redundant shadow of SCHEMA_AWARE. Single call site; inline features & SCHEMA_AWARE and delete the property to eliminate drift risk if a new schema flag is later added to SCHEMA_AWARE.

✅ Praise

The feature-flag pipeline is a good architectural direction — it centralizes schema/cast/inference/storage that was duplicated across builders, and the hook-based extension points are well-chosen. Section dividers in BuilderBase and the "Override in GraphBuilder to …" pattern in hook docstrings are excellent for onboarding. The 11-step numbered pipeline in call_op is easy to follow.

Follow-up suggestions (out of scope)

• Rename tape_builder.py since it now hosts BuilderBase as its primary export.
• Restore the BuilderBase class docstring to call out __getattr__ dispatch as a distinct creation path (the bundled "two operations" wording hides it).
• Consider inlining OpBuilder._call_op (3-line body, naming-collision with BuilderBase.call_op).

Recommendation

Request changes, focused on #1 and #2 (real bugs). Then re-discuss #3/#4 (compat policy) and #5 (tests). Rest can land in follow-ups.