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 support for the MLF to 'compile' command #8086

Merged
merged 2 commits into from
May 25, 2021

Conversation

gromero
Copy link
Contributor

@gromero gromero commented May 20, 2021

Hi,

Could I get reviews for the following change, please?

It adds support for the Model Library Format (MLF) to 'tvmc' so users can
output compilation artifacts to a MLF archive passing the new flag
'--output-format mlf'. For instance:

$ python3 -m tvm.driver.tvmc compile ./sine_model.tflite --target="c" --output sine.tar --output-format mlf

will generate a sine.tar archive that is serialized accordingly to the
MLF.

Since the MLF is currently meant to be used only on micro targets, an
error is generated if one tries to run a MLF outside a micro context:

$ python3 -m tvm.driver.tvmc run ./sine.tar
Error: You're trying to run a model saved using the Model Library Format (MLF). MLF can only be used to run micro targets (µTVM).

The micro context does not exist yet but will be later introduced as
part of the [RFC] "TVMC: Add support for µTVM".

Signed-off-by: Gustavo Romero gustavo.romero@linaro.org

@gromero
Copy link
Contributor Author

gromero commented May 20, 2021

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.

LGTM, thanks for doing this work @gromero.

As we spoke, you'll be adding some tests, which is great, plus, I got a couple minor comments. Feel free to use them as suggestions.

python/tvm/driver/tvmc/compiler.py Outdated Show resolved Hide resolved
python/tvm/driver/tvmc/model.py Show resolved Hide resolved
@leandron
Copy link
Contributor

Also just to report, I tested it locally and it works.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @gromero , looks great! just a small comment


if output_format in ["so", "tar"]:
return self.export_classic_format(executor_factory, package_path, cross, output_format)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

could we be explicit here (elif output_format == "mlf") rather than as a fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! done.

@gromero
Copy link
Contributor Author

gromero commented May 21, 2021

@leandron @areusch Thanks a lot for the reviews. @leandron I'm addressing the test case and I'll push it soon, thanks for pointing it out.

Add support for the Model Library Format (MLF) to 'tvmc' so users can
output compilation artifacts to a MLF archive passing the new flag
'--output-format mlf'. For instance:

$ python3 -m tvm.driver.tvmc compile ./sine_model.tflite --target="c" --output sine.tar --output-format mlf

will generate a sine.tar archive that is serialized accordingly to the
MLF.

Since the MLF is currently meant to be used only on micro targets, an
error is generated if one tries to run a MLF outside a micro context.

The micro context does not exist yet but will be later introduced as
part of the [RFC] "TVMC: Add support for µTVM".

That commit also adds 3 pytest tests to test tvmc + MLF.

Finally, it also fixes some missing periods in the 'compile' command
help sections and renames export_format to output_format so there is
no confusion with flag '--dump-code', which contains "formats to export"
in its help section.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
@gromero
Copy link
Contributor Author

gromero commented May 24, 2021

Hi @areusch and @leandron PTAL. In that v2 I addressed your review comments, added 3 pytest tests to check tvmc + MLF, and rebased the code resolving the conflicts with recent changes (like those related to the addition of --desired-layout and --cross-options flags). Thanks!

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.

LGTM just pending a test pending the fix for one of the tests.

assert str(exp.value) == expected_reason, on_error


def test_tvmc_import_package_mlf(tflite_compiled_model_mlf):
Copy link
Contributor

@leandron leandron May 24, 2021

Choose a reason for hiding this comment

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

Suggested change
def test_tvmc_import_package_mlf(tflite_compiled_model_mlf):
def test_tvmc_import_package_mlf(tflite_compiled_model_mlf):
pytest.importorskip("tflite")

As this test is compiled and executed in the same machine, it will need to be protected with a pytest.importorskip("tflite") as well, so that it will be skipped in the CI i386 machine, like the other in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

Fix missing importorskip() in the import_package test allowing the
test in question to be skipped when 'tflite' is not installed in the
test environment, otherwise the test will fail with:

[...]
>       archive_path = exported_tvmc_package.package_path
E       AttributeError: 'str' object has no attribute 'package_path'
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.

LGTM just pending CI. I tested it locally and I'm happy with this PR.

@areusch do you want to have another look?

@gromero
Copy link
Contributor Author

gromero commented May 25, 2021

@tmoreau89 Hello Thierry. Could you please merge it?

@leandron leandron merged commit 7da97b9 into apache:main May 25, 2021
@leandron
Copy link
Contributor

This is merged now. thanks @areusch @gromero!

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 17, 2021
* [TVMC] Add support for the MLF to 'compile' command

Add support for the Model Library Format (MLF) to 'tvmc' so users can
output compilation artifacts to a MLF archive passing the new flag
'--output-format mlf'. For instance:

$ python3 -m tvm.driver.tvmc compile ./sine_model.tflite --target="c" --output sine.tar --output-format mlf

will generate a sine.tar archive that is serialized accordingly to the
MLF.

Since the MLF is currently meant to be used only on micro targets, an
error is generated if one tries to run a MLF outside a micro context.

The micro context does not exist yet but will be later introduced as
part of the [RFC] "TVMC: Add support for µTVM".

That commit also adds 3 pytest tests to test tvmc + MLF.

Finally, it also fixes some missing periods in the 'compile' command
help sections and renames export_format to output_format so there is
no confusion with flag '--dump-code', which contains "formats to export"
in its help section.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* Fix missing importorskip in the import_package test

Fix missing importorskip() in the import_package test allowing the
test in question to be skipped when 'tflite' is not installed in the
test environment, otherwise the test will fail with:

[...]
>       archive_path = exported_tvmc_package.package_path
E       AttributeError: 'str' object has no attribute 'package_path'
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 17, 2021
* [TVMC] Add support for the MLF to 'compile' command

Add support for the Model Library Format (MLF) to 'tvmc' so users can
output compilation artifacts to a MLF archive passing the new flag
'--output-format mlf'. For instance:

$ python3 -m tvm.driver.tvmc compile ./sine_model.tflite --target="c" --output sine.tar --output-format mlf

will generate a sine.tar archive that is serialized accordingly to the
MLF.

Since the MLF is currently meant to be used only on micro targets, an
error is generated if one tries to run a MLF outside a micro context.

The micro context does not exist yet but will be later introduced as
part of the [RFC] "TVMC: Add support for µTVM".

That commit also adds 3 pytest tests to test tvmc + MLF.

Finally, it also fixes some missing periods in the 'compile' command
help sections and renames export_format to output_format so there is
no confusion with flag '--dump-code', which contains "formats to export"
in its help section.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* Fix missing importorskip in the import_package test

Fix missing importorskip() in the import_package test allowing the
test in question to be skipped when 'tflite' is not installed in the
test environment, otherwise the test will fail with:

[...]
>       archive_path = exported_tvmc_package.package_path
E       AttributeError: 'str' object has no attribute 'package_path'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants