Replace op_schema with op_signature

[justinchuby]

This pull request refactors how operator schemas are handled throughout the autocast, converter, and evaluator modules. The main change is replacing direct usage of OpSchema with a new _schemas.OpSignature abstraction, leading to more consistent and modular code when dealing with operator signatures, especially for input casting and evaluation. Several related methods are renamed and refactored for clarity and encapsulation.

Important changes

• The Evaluator interface now defines eval_op on onnx ops. The old eval was removed in favor of a more flexible eval_op. The exporter's eval will continue to function with a compatible logic in class Op
• op_schema properties from Functions are removed

Operator signature abstraction and autocast refactor:

• Replaced usage of OpSchema with _schemas.OpSignature in onnxscript/_internal/autocast.py, updating all relevant function signatures and internal logic to use the new abstraction. This includes changing how input parameters are filtered and type constraints are accessed.

AST Converter integration:

• Updated the converter (onnxscript/_internal/converter.py) to pass op_signature instead of op_schema to autocast functions, ensuring compatibility with the new signature abstraction.

Evaluator refactor and encapsulation:

• Refactored the evaluator (onnxscript/_internal/evaluator.py) to use _adapt_inputs, _adapt_attributes, and _adapt_outputs methods, encapsulating the logic for adapting inputs/outputs and removing unused or redundant methods. Now, operator signatures are consistently adapted from OpSchema using _schemas.OpSignature.

Addtionally:

• Clean up typing annotation utilities
• Now supply IR attrs directly when creating attributes to avoid proto serialization and loss of subgraph references

[codecov]

Codecov Report

❌ Patch coverage is 86.11111% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.37%. Comparing base (1080d6a) to head (e4fb629).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
onnxscript/_internal/values.py	71.11%	11 Missing and 2 partials ⚠️
onnxscript/_internal/converter.py	90.90%	2 Missing and 1 partial ⚠️
onnxscript/_internal/evaluator.py	93.10%	1 Missing and 1 partial ⚠️
onnxscript/_internal/autocast.py	92.30%	0 Missing and 1 partial ⚠️
onnxscript/ir/_schemas.py	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2771      +/-   ##
==========================================
- Coverage   70.45%   70.37%   -0.08%     
==========================================
  Files         228      228              
  Lines       27257    27150     -107     
  Branches     2760     2736      -24     
==========================================
- Hits        19203    19108      -95     
- Misses       7102     7103       +1     
+ Partials      952      939      -13     

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

[Copilot]

Pull request overview

This PR refactors how operator schemas are handled by replacing direct usage of onnx.defs.OpSchema with a new _schemas.OpSignature abstraction throughout the autocast, converter, and evaluator modules. This change improves modularity and encapsulation when dealing with operator signatures.

Changes:

• Replaced OpSchema with OpSignature in autocast functions for input type casting
• Updated converter to pass op_signature instead of op_schema to autocast functions
• Refactored evaluator methods to be private (_adapt_inputs, _adapt_attributes, _adapt_outputs) and removed the use_graph_attribute method along with its associated conditional logic

Reviewed changes

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

File	Description
onnxscript/_internal/autocast.py	Updated to use OpSignature instead of OpSchema, including parameter filtering and type constraint access changes
onnxscript/_internal/converter.py	Changed to pass op_signature instead of op_schema to autocast functions
onnxscript/_internal/evaluator.py	Made adapter methods private, removed use_graph_attribute method, and simplified _adapt_attributes to always use graph attributes

Comments suppressed due to low confidence (1)

onnxscript/_internal/evaluator.py:548

• The use_graph_attribute method in ORTMixedEvaluator is now dead code since it's no longer called after removing the conditional logic in _adapt_attributes. This method should be removed from the ORTMixedEvaluator class.

    def use_graph_attribute(self, schema: onnx.defs.OpSchema) -> bool:
        return _schema_id(schema) not in self._python_ops

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

onnxscript/_internal/values.py:364

• The docstring still mentions a 'functions' parameter that was removed from the method signature. This outdated documentation should be removed.

            functions: A list of functions to include in the model.
                By default, all functions called at least once are included.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

[gramalingam]

minor suggestion: I guess OpSignature could expose an inputs property that returns this value.

[justinchuby]

Done

[gramalingam]

Does this have a body (definition)? Looks like the next function eval_function comes right after this comment, unless the formatting is confusing me

[justinchuby]

The docstring is the body. This is a protocol so there is no other logic in the method

[gramalingam]

Are these unused? (I have a vague memory of something like this also being used in the so called "exporter" that converts an onnx model back to a string representation of an onnxscript program that produces that onnx model ... but looking at this function, this might not be the one.)

[justinchuby]

Yes. They are unused.

[gramalingam]

I wonder if this is necessary. I guess this is being used to handle the functions down below better. But, in practice, I don't think I have seen uses where functions are passed in, but we do call to_model_proto commonly.

Aren't we making the common case more expensive (cloning the graph before serializaing it instead of serializing it) in order to handle a usage I have not seen?

I guess two other options are: (a) Handle the special-case where no functions are specified more efficiently, or (b) Break compatibility and even drop the functions parameter or design a better API.

[justinchuby]

to_model_proto is called very rarely, so I don't think this will be costly. I cloned because the graph was subsequently modified (with updated opset imports etc.)

I do agree we should do (b). But that can be independent of copying.

[justinchuby]

@gramalingam all tests passed