-
Notifications
You must be signed in to change notification settings - Fork 33
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
Refactor pytest collection to anchor on test case files, not .mlir files. #282
Conversation
if self.path.name == TEST_DATA_FLAGFILE_NAME: | ||
test_cases.append( | ||
MlirFile.TestCase( | ||
name="test", | ||
runtime_flagfile=test_data_flagfile_name, | ||
MlirCompileRunTest.TestCase( | ||
name="", | ||
mlir_file=mlir_file, | ||
runtime_flagfile=TEST_DATA_FLAGFILE_NAME, | ||
enabled=have_lfs_files, | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could remove the need for this branch by generating boilerplate test_cases.json
files in the ONNX test suite:
{
"file_format": "test_cases_v0",
"test_cases": [
{
"name": "",
"runtime_flagfile": "test_data_flags.txt",
"remote_files": []
}
]
}
Trying to keep this conftest file somewhat simple and a bit concerned this is sliding the wrong way 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, would probably be best to have test_cases.json files for all the onnx tests too. That way all our tests can follow a unified format and the conftest won't get confusing. We should probably be anchoring on a test configuration file rather than the runtime flag file for onnx tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I can make that change too. How does a follow-up PR sound? That will require touching lots of files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also be able to replace test_cases.json
with tests.py
and then further simplify (or remove) the conftest.py
file... 🤔 Mainly trying to keep the large test suites autogenerated, concise, and directly compatible with iree-compile
-> iree-run-module
.
) See https://huggingface.co/amd-shark/sdxl-quant-models, which is now a source of truth for developers interfacing with this model. For now the files are pinned to a commit hash as changes are expected over the coming days. Some details: * I needed to update the regex in `download_remote_files.py` to support subdirectories. The `/` search was greedy, capturing e.g. `fe57fe12eeb6eac83f469793984f6ad4c06a478c/unet/fp16/export` and `sdxl_unet_fp16_dataset.irpa` instead of `fe57fe12eeb6eac83f469793984f6ad4c06a478c` and `unet/fp16/export/sdxl_unet_fp16_dataset.irpa`. * This benefits from #282, since the `.mlir` file that the test collector looks for is a remote file not downloaded with a regular Git [LFS] checkout. That PR changes the collector to look for `test_cases.json` instead of `.mlir` files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up is fine
) See https://huggingface.co/amd-shark/sdxl-quant-models, which is now a source of truth for developers interfacing with this model. For now the files are pinned to a commit hash as changes are expected over the coming days. Some details: * I needed to update the regex in `download_remote_files.py` to support subdirectories. The `/` search was greedy, capturing e.g. `fe57fe12eeb6eac83f469793984f6ad4c06a478c/unet/fp16/export` and `sdxl_unet_fp16_dataset.irpa` instead of `fe57fe12eeb6eac83f469793984f6ad4c06a478c` and `unet/fp16/export/sdxl_unet_fp16_dataset.irpa`. * This benefits from #282, since the `.mlir` file that the test collector looks for is a remote file not downloaded with a regular Git [LFS] checkout. That PR changes the collector to look for `test_cases.json` instead of `.mlir` files.
…les. (#282) This allows us to detect test cases that store their `.mlir` or `.mlirbc` file remotely, since the `test_data_flags.txt` or `test_cases.json` file will always exist locally. This also changes test case names in summaries from e.g. ``` PASSED onnx/node/generated/test_sub_uint8/model.mlir::cpu_llvm_sync_test PASSED onnx/node/generated/test_sub_example/model.mlir::cpu_llvm_sync_test PASSED onnx/node/generated/test_sub_bcast/model.mlir::cpu_llvm_sync_test XFAIL pytorch/models/opt-125M/opt-125M.mlirbc::cpu_llvm_task_splats - Expected compilation to fail (included in 'expected_compile_failures') ``` to e.g. ``` PASSED onnx/node/generated/test_sub_example/test_data_flags.txt::model.mlir::cpu_llvm_sync PASSED onnx/node/generated/test_sub_uint8/test_data_flags.txt::model.mlir::cpu_llvm_sync PASSED onnx/node/generated/test_sub_bcast/test_data_flags.txt::model.mlir::cpu_llvm_sync XFAIL pytorch/models/opt-125M/test_cases.json::opt-125M.mlirbc::cpu_llvm_task::splats - Expected compilation to fail (included in 'expected_compile_failures' ``` (anchored on the .txt or .json so added back the .mlir, also dropped the "test") This is a bit hacky either way. I took a look at https://docs.pytest.org/en/stable/example/customdirectory.html as an alternative to https://docs.pytest.org/en/stable/example/nonpython.html and that could help.
This allows us to detect test cases that store their
.mlir
or.mlirbc
file remotely, since thetest_data_flags.txt
ortest_cases.json
file will always exist locally. This also changes test case names in summaries from e.g.to e.g.
(anchored on the .txt or .json so added back the .mlir, also dropped the "test")
This is a bit hacky either way. I took a look at https://docs.pytest.org/en/stable/example/customdirectory.html as an alternative to https://docs.pytest.org/en/stable/example/nonpython.html and that could help.