Refactor pytest collection to anchor on test case files, not .mlir files. #282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ScottTodd
commented
Jul 8, 2024
Comment on lines
+225
to
233
| 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.
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.
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.
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.
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.
ScottTodd
added a commit
that referenced
this pull request
Jul 9, 2024
) 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.
renxida
pushed a commit
that referenced
this pull request
Jul 18, 2024
) 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.
renxida
pushed a commit
that referenced
this pull request
Jul 18, 2024
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows us to detect test cases that store their
.mliror.mlirbcfile remotely, since thetest_data_flags.txtortest_cases.jsonfile 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.