Skip incompatible dtypes in op_test using OpSchema | test(torchlib) #700

justinchuby · 2023-04-29T04:04:19Z

This requires ONNX 1.14. I am skipping the op test for ONNX<1.14 altogether.

I tested locally to make sure no float32 test is skipped with this mechanism, and that it skips properly according to the type annotation.

Currently no new dtypes are enabled because there are too many failures.

Requires

onnxscript/tests/function_libs/torch_lib/ops_test_common.py

BowenBao · 2023-05-01T22:14:07Z

onnxscript/tests/function_libs/torch_lib/ops_test_common.py

@@ -355,6 +355,41 @@ def _format_model_and_input_information(onnx_model, inputs):
    )


+TORCH_DTYPE_TO_ONNX_STRING = {
+    torch.bool: "tensor(bool)",


nit: Commented elsewhere before but don't recall precisely. Does it has to be plain string for onnx types?

Yes, it needs to be a plain string in onnx's type constraints

I was wondering if there is anything like str(onnx.BoolTensorType)/onnx.BoolTensorType.to_string()

I see. There probably is but we still need a mapping from torch types to onnx types. Any suggestions?

BowenBao · 2023-05-01T22:20:22Z

onnxscript/tests/function_libs/torch_lib/ops_test_common.py

+}
+
+
+def dtype_op_schema_compatible(dtype: torch.dtype, schema: onnx.defs.OpSchema) -> bool:


I think it's worth commenting to explain the context for dtype. It is the "dtype setting" introduced by opinfo tests, so not any average input dtype. Feels like it holds the assumption that to some extent, all arguments outputs should have the same dtype.

I think it helps also to brief what it means if a schema is compatible to a dtype. Do all arguments, outputs have to share that same type?

Added When a dtype is "compatible" with the schema, it means we can use the dtype to create sample inputs by OpInfo to test the ONNX function and expect outputs to match.

I'm very curious if there are edge cases when sample inputs are different type tensors and how opinfo handles it. Thanks for comments. They are very clear now.

onnxscript/tests/function_libs/torch_lib/ops_test_common.py

titaiwangms · 2023-05-02T01:06:54Z

onnxscript/tests/function_libs/torch_lib/ops_test.py

+        test_suite.skipTest(
+            f"dtype '{dtype}' is not supported by the op '{op.name}'. "
+            f"Type constraints: {onnx_function.op_schema.type_constraints}"
+        )


I have a concern on skipping tests with the manual annotated type constraints. If we wrongly annotate an Op inputs, it could skip the tests it shouldn't, which makes us miss the very first opportunity to catch it.

That's true. Any suggestions?

wonder if we can xfail

Based on what I found in pytorch/pytorch#100265, indeed, if the type constraints are from onnx spec, most likely it's xfailable. Not sure if it's an easy implementation though...

Sometimes it will xpass if we do xfail because ort can choose to implement the op for some types intentionally or unintentionally. So that's a little tricky

How about put the logic after runs? Just an initial thought, if it fails, then we check the dtype is supported or not.

Hmm but that’s still going to skip the tests where the annotation is incorrect?

I thought about this again. Maybe that’s ok for now since if we declare that something is not supported via the annotations, then the exporter will just need to bridge the types via type promotion logic. When we see unnecessary type promotion in the graph we can then come back and make changes to the annotations.

Merging for now. Feel free to share more thoughts!

BowenBao · 2023-05-02T01:07:53Z

onnxscript/tests/function_libs/torch_lib/ops_test_common.py

+}
+
+
+def dtype_op_schema_compatible(dtype: torch.dtype, schema: onnx.defs.OpSchema) -> bool:


I think this is good for a quick and general guard for tests and evolve from there.

What are your thoughts on more fine-grained type checking that validates type of each concrete input to the type of the matching input in schema?

I think we will need to build the same logic in the dispatcher. We can evolve that can use it here. For now I think it's good to use logic that is easy to understand and get right for running test cases. It is always better to say inputs are compatible than not so we don't miss test cases. We can always put an xfail on them.

Also this logic is for reducing the amount of tests we need to manually skip, so accuracy isn't super important here (as long as we don't skip more than we should)

Skip incompatible dtypes in op_test using OpSchema | test(torchlib)

b84ca6a

justinchuby requested review from xiaowuhu and fatcat-z April 29, 2023 04:04

justinchuby added module: torchlib Related to the torch/aten function lib in development topic: testing labels Apr 29, 2023

justinchuby linked an issue Apr 29, 2023 that may be closed by this pull request

Test functions on all supported dtypes #417

Closed

2 tasks

justinchuby mentioned this pull request Apr 29, 2023

Test functions on all supported dtypes #417

Closed

2 tasks

Skip

d042c34

justinchuby added the hold on merging Don't merge yet label Apr 29, 2023

Merge branch 'main' into justinchu/test-more-dtypes

46d893a

justinchuby removed the hold on merging Don't merge yet label Apr 29, 2023

titaiwangms self-requested a review May 1, 2023 16:22

justinchuby requested a review from BowenBao May 1, 2023 16:38

justinchuby added 3 commits May 1, 2023 14:40

Merge branch 'main' into justinchu/test-more-dtypes

c2b992b

Update ops_test.py

afb7cbf

Update ops_test.py

36fea8e

BowenBao reviewed May 1, 2023

View reviewed changes

onnxscript/tests/function_libs/torch_lib/ops_test_common.py Outdated Show resolved Hide resolved

BowenBao reviewed May 1, 2023

View reviewed changes

onnxscript/tests/function_libs/torch_lib/ops_test_common.py Outdated Show resolved Hide resolved

justinchuby added 3 commits May 2, 2023 00:20

Add docs

087e384

update names

c2fee91

More comments

605500f

titaiwangms reviewed May 2, 2023

View reviewed changes

BowenBao approved these changes May 2, 2023

View reviewed changes

justinchuby and others added 3 commits May 1, 2023 18:29

Merge branch 'main' into justinchu/test-more-dtypes

c5187f7

Remove torch version limit

b882e42

Merge branch 'main' into justinchu/test-more-dtypes

92f3ca1

justinchuby merged commit f141dae into main May 2, 2023

justinchuby deleted the justinchu/test-more-dtypes branch May 2, 2023 02:28

		}


		def dtype_op_schema_compatible(dtype: torch.dtype, schema: onnx.defs.OpSchema) -> bool:

Skip incompatible dtypes in op_test using OpSchema | test(torchlib) #700

Skip incompatible dtypes in op_test using OpSchema | test(torchlib) #700

Uh oh!

Conversation

justinchuby commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titaiwangms May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titaiwangms May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinchuby May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinchuby commented Apr 29, 2023 •

edited

Loading

titaiwangms May 2, 2023 •

edited

Loading

titaiwangms May 2, 2023 •

edited

Loading

justinchuby May 2, 2023 •

edited

Loading