[TVMC] add cl support in tvmc runner #6831
Conversation
There was a problem hiding this comment.
Hi @euntaik, thanks for the patch. Adding support to CL is something we are looking forward.
Could you also add some tests, to make sure that support doesn't break in the future without us noticing? There are two kinds tests I think we need here: some for compilation targeting cl, as well as runner tests.
|
cc @comaniac |
tests/python/driver/tvmc/conftest.py
Outdated
| @@ -70,6 +89,19 @@ def tflite_mobilenet_v1_1_quant(tmpdir_factory): | |||
| return model_file | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="session") | |||
| def tflite_mobilenet_v1_0_25_128(tmpdir_factory): | |||
There was a problem hiding this comment.
I think it is OK to add more models here, just want to check whether there is anything special with this model, that prevents using the existing mobilenet, already here?
There was a problem hiding this comment.
I wanted to use the smallest possible mobilenet without the quantization.
There was a problem hiding this comment.
Same opinion. I don't see a strong reason of adding an end-to-end model test. Please be aware that running e2e network in the unit test is relatively time-consuming, so we should avoid unnecessary test as possible.
There was a problem hiding this comment.
Yeah, this could be the first one to check only API calls, using a mock, rather than downloading a file and exercising the whole e2e path.
python/tvm/driver/tvmc/runner.py
Outdated
| @@ -50,7 +50,7 @@ def add_run_parser(subparsers): | |||
| # like 'cl', 'webgpu', etc (@leandron) | |||
There was a problem hiding this comment.
| # like 'cl', 'webgpu', etc (@leandron) | |
| # like 'webgpu', etc (@leandron) |
There was a problem hiding this comment.
IMHO, unit tests should be isolated and only test the target components. Specifically, the unit tests for TVMC should not test the entire compilation and execution flow but just need to make sure the inputs to other TVM components are correct. Whether the codegen or runtime are successes or not should not be covered in TVMC unit tests (that means, this PR should not enable OpenCL in the CI).
@leandron I realized this issue also applies to some existing tests. Could we review the TVMC unit tests again later and make them more precise and concise? If we found that the reason to have such tests here is that this is never covered else where, it is totally fine to add more unit tests to the right place (e.g., codegen, runtime).
tests/python/driver/tvmc/conftest.py
Outdated
| @@ -70,6 +89,19 @@ def tflite_mobilenet_v1_1_quant(tmpdir_factory): | |||
| return model_file | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="session") | |||
| def tflite_mobilenet_v1_0_25_128(tmpdir_factory): | |||
There was a problem hiding this comment.
Same opinion. I don't see a strong reason of adding an end-to-end model test. Please be aware that running e2e network in the unit test is relatively time-consuming, so we should avoid unnecessary test as possible.
|
created a new PR for changes in compiler.py |
I agree we don't need to run all tests as end-to-end, and I can remove some overlap, by creating some mocks and checking that calls are made to the internal APIs with the right parameters (as you mentioned), but I'd like a few to stay - I'll see to get it done, as soon as I can finish a PR I'm working on. |
It makes sense to keep a few for reasons and we can discuss on the PR. Thanks :) |
comaniac
added
status: accepted
and removed
status: need update
* [TVMC] add cl support in tvmc runner * Cleanup comment and asssert device type in else case
* [TVMC] add cl support in tvmc runner * Cleanup comment and asssert device type in else case
* [TVMC] add cl support in tvmc runner * Cleanup comment and asssert device type in else case
add cl support in tvmc runner