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

[IR] Improve from_proto/to_proto typing with overloads #1992

Merged
merged 8 commits into from
Dec 31, 2024

Conversation

justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Dec 31, 2024

  • Use typing.overload to annotate the from_proto method for accurate type hinting. With this change we can recommend users to use ir.from/to_proto over the ir.serde.(de)serialize* methods and still keep mypy happy. This simplifies the serialization apis for users.
  • Create deserialize_tensor_shape to deserialize tensor shapes.

onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Dec 31, 2024

❌ 25 Tests Failed:

Tests completed Failed Passed Skipped
16846 25 16821 3780
View the top 1 failed tests by shortest run time
::onnxscript.rewriter.onnxruntime.xformers._test_models
Stack Traces | 0s run time
No failure message available
View the full list of 2 ❄️ flaky tests
tests.eager_mode_test.TestEagerModeArguments_0_reference_runtime::test_function_input_and_attribute_by_kwargs_out_of_order

Flake rate in main: 39.28% (Passed 12602 times, Failed 8152 times)

Stack Traces | 0.002s run time
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:91: in run
    res = self._run(x, y)
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:139: in _run
    res = (convert_from_ml_dtypes(res[0]),)
..../test_onnx_weekly/lib/python3.11.../onnx/reference/custom_element_types.py:50: in convert_from_ml_dtypes
    return array.view(dtype=dtype)
E   ValueError: Changing the dtype of a 0d array is only supported if the itemsize is unchanged

The above exception was the direct cause of the following exception:
tests/eager_mode_test.py:115: in test_function_input_and_attribute_by_kwargs_out_of_order
    self.assertEqual(add_with_alpha(alpha=3.0, other=2.0, this=1.0), 7.0)
onnxscript/values.py:576: in __call__
    return evaluator.default().eval_function(self, args, kwargs)
onnxscript/evaluator.py:307: in eval_function
    result = function.function(*adapted_args, **adapted_kwargs)
tests/eager_mode_test.py:59: in add_with_alpha
    other = op.Mul(other, alpha)
.../onnx_opset/_impl/opset14.py:696: in Mul
    return op(*self._prepare_inputs(schema, A, B))
onnxscript/values.py:304: in __call__
    return evaluator.default().eval(schema, args, kwargs)
onnxscript/evaluator.py:194: in eval
    outputs = self._eval(schema, inputs, attributes, closure)
onnxscript/evaluator.py:524: in _eval
    result = session.run(None, session_run_input)
..../test_onnx_weekly/lib/python3.11.../onnx/reference/reference_evaluator.py:593: in run
    outputs = node.run(*inputs, **linked_attributes)
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:114: in run
    res = OpRunBinary.run(self, x, y)
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:93: in run
    raise TypeError(
E   TypeError: Issues with types <class 'numpy.ndarray'>, <class 'numpy.ndarray'> (binary operator 'Mul').
tests.eager_mode_test.TestEagerModeArguments_0_reference_runtime::test_function_some_input_by_kwargs

Flake rate in main: 39.28% (Passed 12602 times, Failed 8152 times)

Stack Traces | 0.002s run time
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:91: in run
    res = self._run(x, y)
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:139: in _run
    res = (convert_from_ml_dtypes(res[0]),)
..../test_onnx_weekly/lib/python3.11.../onnx/reference/custom_element_types.py:50: in convert_from_ml_dtypes
    return array.view(dtype=dtype)
E   ValueError: Changing the dtype of a 0d array is only supported if the itemsize is unchanged

The above exception was the direct cause of the following exception:
tests/eager_mode_test.py:106: in test_function_some_input_by_kwargs
    self.assertEqual(add_with_alpha(1.0, other=2.0), 3.0)
onnxscript/values.py:576: in __call__
    return evaluator.default().eval_function(self, args, kwargs)
onnxscript/evaluator.py:307: in eval_function
    result = function.function(*adapted_args, **adapted_kwargs)
tests/eager_mode_test.py:59: in add_with_alpha
    other = op.Mul(other, alpha)
.../onnx_opset/_impl/opset14.py:696: in Mul
    return op(*self._prepare_inputs(schema, A, B))
onnxscript/values.py:304: in __call__
    return evaluator.default().eval(schema, args, kwargs)
onnxscript/evaluator.py:194: in eval
    outputs = self._eval(schema, inputs, attributes, closure)
onnxscript/evaluator.py:524: in _eval
    result = session.run(None, session_run_input)
..../test_onnx_weekly/lib/python3.11.../onnx/reference/reference_evaluator.py:593: in run
    outputs = node.run(*inputs, **linked_attributes)
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:114: in run
    res = OpRunBinary.run(self, x, y)
..../test_onnx_weekly/lib/python3.11.../reference/ops/_op.py:93: in run
    raise TypeError(
E   TypeError: Issues with types <class 'numpy.ndarray'>, <class 'numpy.ndarray'> (binary operator 'Mul').

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

onnxscript/ir/serde.py Fixed Show fixed Hide fixed
| onnx.FunctionProto,
) -> Any:
proto: onnx.TensorShapeProto.Dimension,
) -> tuple[int | _core.SymbolicDim, str | None]: ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
@justinchuby justinchuby changed the title [IR] Fix from_proto typing with overloads [IR] Improve from_proto typing with overloads Dec 31, 2024
@justinchuby justinchuby added the topic: IR Intermediate representation label Dec 31, 2024
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
@justinchuby justinchuby changed the title [IR] Improve from_proto typing with overloads [IR] Improve from_proto/to_proto typing with overloads Dec 31, 2024
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Fixed Show fixed Hide fixed
onnxscript/ir/serde.py Outdated Show resolved Hide resolved
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@justinchuby justinchuby enabled auto-merge (squash) December 31, 2024 17:37
@justinchuby justinchuby merged commit 854b5d9 into main Dec 31, 2024
21 of 41 checks passed
@justinchuby justinchuby deleted the justinchu/from_proto-type branch December 31, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: IR Intermediate representation
Projects
Development

Successfully merging this pull request may close these issues.

2 participants