Allow tvmc to compile models with AOT executor in MLF #8331

Mousius · 2021-06-24T15:38:49Z

The tflite_compiled_model fixture was getting duplicated a few times so I've added a parameterized fixture tflite_tvmc_compiler which combines tmpdir_factory setup with compile_model

Nested target params broke a basic string split, so in cases where we use nested target params I replaced the string split with shlex split

Mousius · 2021-06-24T15:43:03Z

@leandron @gromero

gromero

Hi @Mousius . Thanks for adding these AOT bits to tvmc.

Please see my comments inline

I really liked your commit message and I get happy when I see that format instead of the "bullet format", like adding the comment about shlex etc. Thank you. The only thing I missed is that you actually added an important change in TVMCPackage.import_package() which is the ability to distinguish Graph from AOT runtime based on graph, so it would be nice to mention it also in the commit message, I think :)

gromero · 2021-06-24T21:39:25Z

python/tvm/driver/tvmc/model.py

+            if "graph" in metadata["runtimes"]:
+                graph = temp.relpath("runtime-config/graph/graph.json")
+            else:
+                graph = None


Nice. I just think a hint about AOT should exist here. How about adding a comment above graph = None, like "AOT runtime"?

I'll use the code as docs rather than a comment and pop the condition in a variable:

is_graph_runtime = "graph" in metadata["runtimes"] graph = temp.relpath("runtime-config/graph/graph.json") if is_graph_runtime else None

gromero · 2021-06-24T21:45:57Z

tests/python/driver/tvmc/conftest.py

-@pytest.fixture(scope="session")
-def tflite_compiled_model_mlf(tmpdir_factory):
+@pytest.fixture
+def tflite_tvmc_compiler(tmpdir_factory):


How about renaming the tflite_tvmc_compiler fixture to tflite_compile_model, just because I don't think that tvmc in there is informative. But no strong feelings about it. Feel free also to get more input from Leandro before sending a v2.

Also, this is in the tvmc conftest so context makes more sense 😸 good call!

gromero · 2021-06-24T21:47:16Z

tests/python/driver/tvmc/test_mlf.py



-def test_tvmc_import_package_mlf(tflite_compiled_model_mlf):
+def test_tvmc_import_package_mlf(tflite_mobilenet_v1_1_quant, tflite_tvmc_compiler):


I think we can rename test_tvmc_import_package_mlf to test_tvmc_import_package_mlf_graph.

gromero · 2021-06-24T21:49:13Z

tests/python/driver/tvmc/test_mlf.py


    assert tvmc_package.lib_name is None, ".lib_name must not be set in the MLF archive."
    assert tvmc_package.lib_path is None, ".lib_path must not be set in the MLF archive."
    assert tvmc_package.graph is not None, ".graph must be set in the MLF archive."


The error message must be adapted here, adding "[...] if not AOT". Maybe:
`".graph must be set in the MLF archive if not AOT runtime"

Good call, I'll say "if Graph" to make it clear where the distinction is

leandron

LGTM, agree with @gromero's suggestions.

The tflite_compiled_model fixture was getting duplicated a few times so I've added a parameterized fixture tflite_tvmc_compiler which combines tmpdir_factory setup with compile_model Nested targets broke a basic string split, so in cases where we use nested targets I replaced the string split with shlex split

Plus other clean ups

leandron · 2021-06-30T08:44:40Z

This is is failing in the latest version of this PR. I'm pasting the error below, to save a trip to Jenkins:

==================================== ERRORS ====================================

____________ ERROR collecting tests/python/driver/tvmc/test_mlf.py _____________
/usr/local/lib/python3.6/dist-packages/pluggy/hooks.py:286: in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
/usr/local/lib/python3.6/dist-packages/pluggy/manager.py:93: in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
/usr/local/lib/python3.6/dist-packages/pluggy/manager.py:87: in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
/usr/local/lib/python3.6/dist-packages/_pytest/python.py:249: in pytest_pycollect_makeitem
    res = list(collector._genfunctions(name, obj))
/usr/local/lib/python3.6/dist-packages/_pytest/python.py:470: in _genfunctions
    self.ihook.pytest_generate_tests.call_extra(methods, dict(metafunc=metafunc))
/usr/local/lib/python3.6/dist-packages/pluggy/hooks.py:324: in call_extra
    return self(**kwargs)
/usr/local/lib/python3.6/dist-packages/pluggy/hooks.py:286: in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
/usr/local/lib/python3.6/dist-packages/pluggy/manager.py:93: in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
/usr/local/lib/python3.6/dist-packages/pluggy/manager.py:87: in <lambda>
    firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
conftest.py:35: in pytest_generate_tests
    tvm.testing._auto_parametrize_target(metafunc)
python/tvm/testing.py:800: in _auto_parametrize_target
    for mark in metafunc.definition.iter_markers("parametrize")
python/tvm/testing.py:801: in <listcomp>
    for arg in mark.args[0].split(",")
E   AttributeError: 'list' object has no attribute 'split'
=============================== warnings summary ===============================
../usr/local/lib/python3.6/dist-packages/tensorflow/python/autograph/utils/testing.py:21
  /usr/local/lib/python3.6/dist-packages/tensorflow/python/autograph/utils/testing.py:21: DeprecationWarning: the imp module is eprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

- Docs: https://docs.pytest.org/en/stable/warnings.html
- generated xml file: /workspace/build/pytest-results/python-integration-driver-ctypes.xml -
=========================== short test summary info ============================
ERROR tests/python/driver/tvmc/test_mlf.py - AttributeError: 'list' object ha...
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
========================= 1 warning, 1 error in 0.97s ==========================

Apart from that, LGTM. Happy to review/merge once we get CI working.

Mousius · 2021-06-30T16:39:46Z

Hopefully that should do it, thanks for your feedback @gromero 😸

leandron

LGTM

leandron · 2021-07-01T09:37:26Z

This is now merged, thanks @Mousius and @gromero

* Allow tvmc to compile models with AOT executor The tflite_compiled_model fixture was getting duplicated a few times so I've added a parameterized fixture tflite_tvmc_compiler which combines tmpdir_factory setup with compile_model Nested targets broke a basic string split, so in cases where we use nested targets I replaced the string split with shlex split * Clarify that graph JSON is required only for graph executor Plus other clean ups * Change parametrize fixture to use string instead of list

gromero requested changes Jun 24, 2021

View reviewed changes

leandron reviewed Jun 25, 2021

View reviewed changes

Mousius added 3 commits June 27, 2021 21:35

Clarify that graph JSON is required only for graph executor

79c57be

Plus other clean ups

Change parametrize fixture to use string instead of list

d7893a4

Mousius force-pushed the aot-exec-tvmc branch from b76aae7 to d7893a4 Compare June 30, 2021 15:59

leandron approved these changes Jul 1, 2021

View reviewed changes

leandron merged commit 9112b6e into apache:main Jul 1, 2021

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed



		def test_tvmc_import_package_mlf(tflite_compiled_model_mlf):
		def test_tvmc_import_package_mlf(tflite_mobilenet_v1_1_quant, tflite_tvmc_compiler):

Allow tvmc to compile models with AOT executor in MLF #8331

Allow tvmc to compile models with AOT executor in MLF #8331

Uh oh!

Conversation

Mousius commented Jun 24, 2021

Uh oh!

Mousius commented Jun 24, 2021

Uh oh!

gromero left a comment

Choose a reason for hiding this comment

Uh oh!

gromero Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

Mousius Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

gromero Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mousius Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

gromero Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

gromero Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

Mousius Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

leandron left a comment

Choose a reason for hiding this comment

Uh oh!

leandron commented Jun 30, 2021

Uh oh!

Mousius commented Jun 30, 2021

Uh oh!

leandron left a comment

Choose a reason for hiding this comment

Uh oh!

leandron commented Jul 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gromero Jun 24, 2021 •

edited

Loading