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

[TVMC] add cl support in tvmc runner #6831

Merged
merged 2 commits into from
Nov 10, 2020
Merged

[TVMC] add cl support in tvmc runner #6831

merged 2 commits into from
Nov 10, 2020

Conversation

euntaik
Copy link
Contributor

@euntaik euntaik commented Nov 3, 2020

add cl support in tvmc runner

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

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.

@leandron
Copy link
Contributor

leandron commented Nov 3, 2020

cc @comaniac

@comaniac comaniac self-assigned this Nov 3, 2020
@comaniac comaniac added the status: need update need update based on feedbacks label Nov 3, 2020
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Please address the comments from @leandron

@@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to use the smallest possible mobilenet without the quantization.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

tests/python/driver/tvmc/conftest.py Outdated Show resolved Hide resolved
@@ -50,7 +50,7 @@ def add_run_parser(subparsers):
# like 'cl', 'webgpu', etc (@leandron)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# like 'cl', 'webgpu', etc (@leandron)
# like 'webgpu', etc (@leandron)

python/tvm/driver/tvmc/compiler.py Outdated Show resolved Hide resolved
tests/python/driver/tvmc/test_runner.py Outdated Show resolved Hide resolved
tests/scripts/task_config_build_cpu.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

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).

python/tvm/driver/tvmc/compiler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/runner.py Show resolved Hide resolved
@@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@euntaik
Copy link
Contributor Author

euntaik commented Nov 5, 2020

created a new PR for changes in compiler.py
#6855

@leandron leandron mentioned this pull request Nov 5, 2020
7 tasks
@leandron
Copy link
Contributor

leandron commented Nov 5, 2020

@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).

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.

@comaniac
Copy link
Contributor

comaniac commented Nov 5, 2020

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 comaniac merged commit 80ca598 into apache:main Nov 10, 2020
@comaniac
Copy link
Contributor

Thanks @euntaik @leandron. We can work on adding more tests along with other TVMC TODOs.

@comaniac comaniac added status: accepted and removed status: need update need update based on feedbacks labels Nov 10, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
* [TVMC] add cl support in tvmc runner

* Cleanup comment and asssert device type in else case
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
* [TVMC] add cl support in tvmc runner

* Cleanup comment and asssert device type in else case
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
* [TVMC] add cl support in tvmc runner

* Cleanup comment and asssert device type in else case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants