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

Addition of 4 pytorch onnx-export models. #278

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

saienduri
Copy link
Contributor

@saienduri saienduri commented Jul 3, 2024

This commit adds testing for 4 models that we have noticed to be unstable and want to keep passing through CI:

mit-b0
mobilebert-uncased
t5-base
t5-large

These models are run through the onnx mode and pytorch framework where the pytorch model is exported to onnx, imported to torch IR using the onnx importer, and then fed to IREE to lower to linalg and run the module.

We currently don't support the use of external parameters through the onnx flow thus the lack of parameter files and splat testing

Also added the --iree-input-demote-i64-to-i32 flag to the cpu models config file so that we are in line with what we run in e2eshark for any cpu code generation

@saienduri saienduri requested a review from ScottTodd July 3, 2024 13:23
Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

Thanks! Generally looks great. A few ideas for improvement, but could proceed without all addressed.

Comment on lines 3 to 7
"iree_compile_flags" : [
"--iree-hal-target-backends=llvm-cpu",
"--iree-llvmcpu-target-cpu-features=host"
"--iree-llvmcpu-target-cpu-features=host",
"--iree-input-demote-i64-to-i32"
],
Copy link
Member

Choose a reason for hiding this comment

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

Also added the --iree-input-demote-i64-to-i32 flag to the cpu models config file so that we are in line with what we run in e2eshark for any cpu code generation

I'm not sure I agree with this. I'm worried about the number of flags we have developers using, and I'd like to have more visibility into which models require which flags and why. One way we can do that is by having users of the test suite test with default flags.

In this case, the --iree-input- flags are sort of special in that they are generic across backends so I do see a case for using some of them... maybe in a _compatibility_flags test suite config, then we could diff the XFAILs between that and the _default_flags config.

Copy link
Member

Choose a reason for hiding this comment

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

These models are run through the onnx mode and pytorch framework where the pytorch model is exported to onnx, imported to torch IR using the onnx importer, and then fed to IREE to lower to linalg and run the module.

That seems rather roundabout... I guess ONNX there is used as a serialization format?

IMO if we're adding tests for these models we should add both the direct and onnx-export versions.

Comment on lines +17 to +19
"expected_run_failures": [
"pytorch/models/onnx-export/mobilebert-uncased",
]
Copy link
Member

Choose a reason for hiding this comment

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

This commit adds testing for 4 models that we have noticed to be unstable and want to keep passing through CI

Can we add models that we think are stable too? :) I'd like to prefetch all models from e2eshark and get them running here if possible, rather than cherrypick a few after we notice issues.

Comment on lines 15 to 16
// error: 'builtin.module' op failed to run transform dialect passes
// (might need to drop the iree-codegen-transform-dialect-library flag)
Copy link
Member

Choose a reason for hiding this comment

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

Can drop this comment since the attention_and_matmul_spec is removed.
I also saw error: a handle passed as operand #0 and consumed by this operation points to a payload entity more than once and updated the comment in my draft PR: #277

How about we just leave the reason comments off? Also on my PR there, if we run with -rA (instead of -rpfE, see https://docs.pytest.org/en/stable/how-to/output.html), we'll get full error output for even XFAIL'd tests in CI logs, so having an possibly outdated comment in the source isn't as useful.

Comment on lines +57 to +60
"https://sharkpublic.blob.core.windows.net/sharkpublic/shark-test-suite/iree-tests/pytorch/models/onnx-export/t5-base/inference_output.46.bin",
"https://sharkpublic.blob.core.windows.net/sharkpublic/shark-test-suite/iree-tests/pytorch/models/onnx-export/t5-base/inference_output.47.bin",
"https://sharkpublic.blob.core.windows.net/sharkpublic/shark-test-suite/iree-tests/pytorch/models/onnx-export/t5-base/inference_output.48.bin",
"https://sharkpublic.blob.core.windows.net/sharkpublic/shark-test-suite/iree-tests/pytorch/models/onnx-export/t5-base/inference_output.49.bin",
Copy link
Member

Choose a reason for hiding this comment

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

o_o that's a lot of outputs. Fine for now, but let's file an issue to use archive files (.zip or .tar.gz). If we use something like Hugging Face repositories, we could also lean on their download API to fetch an entire repo

Comment on lines +106 to +108
"https://sharkpublic.blob.core.windows.net/sharkpublic/shark-test-suite/iree-tests/pytorch/models/onnx-export/t5-large/inference_output.95.bin",
"https://sharkpublic.blob.core.windows.net/sharkpublic/shark-test-suite/iree-tests/pytorch/models/onnx-export/t5-large/inference_output.96.bin",
"https://sharkpublic.blob.core.windows.net/sharkpublic/shark-test-suite/iree-tests/pytorch/models/onnx-export/t5-large/inference_output.97.bin",
Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's even more outputs :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants