[TVMC][UMA] Support using UMA with TVMC #14165
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment. Generated by tvm-bot |
python/tvm/driver/tvmc/compiler.py
Outdated
| """Include parser for 'compile' subcommand""" | ||
|
|
||
| parser = subparsers.add_parser("compile", help="compile a model.") | ||
| parser = subparsers.add_parser("compile", help="compile a model.", add_help=False) |
There was a problem hiding this comment.
Curious, why do we need to remove help option?
There was a problem hiding this comment.
I encountered an issue where the TVMCSuppressedArgumentParser still caused a SystemExit exception. However, I cannot seem to reproduce the issue right now. So I'll remove this again.
There was a problem hiding this comment.
Alright, I found the input again: "tvmc compile --help". When removing the help option from the subparser, this is resolved. The help option still works because it is enabled through the main parser.
There was a problem hiding this comment.
I tried this out locally but see some difference in the help prompt when executing tvmc compile --help. Before the patch we get something like:
$ tvmc compile --help
usage: tvmc compile [-h] [--cross-compiler CROSS_COMPILER]
[--cross-compiler-options CROSS_COMPILER_OPTIONS]
[--desired-layout {NCHW,NHWC}] [--dump-code FORMAT]
[--model-format {keras,onnx,pb,tflite,pytorch,paddle,relay}]
[-o OUTPUT] [-f {so,mlf}] [--pass-config name=value]
[--target TARGET]
...
positional arguments:
FILE path to the input model file.
optional arguments:
-h, --help show this help message and exit
--cross-compiler CROSS_COMPILER
the cross compiler to generate target libraries, e.g.
'aarch64-linux-gnu-gcc'.
--cross-compiler-options CROSS_COMPILER_OPTIONS
the cross compiler options to generate target
libraries, e.g. '-mfpu=neon-vfpv4'.
--desired-layout {NCHW,NHWC}
change the data layout of the whole graph.
While after seems to lead to:
$ tvmc compile --help
usage: tvmc compile [--experimental-tvm-extension EXPERIMENTAL_TVM_EXTENSION]
[--cross-compiler CROSS_COMPILER]
[--cross-compiler-options CROSS_COMPILER_OPTIONS]
[--desired-layout {NCHW,NHWC}] [--dump-code FORMAT]
[--model-format {keras,onnx,pb,tflite,pytorch,paddle,relay}]
[-o OUTPUT] [-f {so,mlf}] [--pass-config name=value]
[--target TARGET]
...
tvmc compile: error: the following arguments are required: FILE
Could you confirm if you see the same behaviour? I think it would be really helpful if we can keep the argument descriptions as before
There was a problem hiding this comment.
Alright, I can confirm this issue. The underlying issue is that the TVMCSuppressedArgumentParser still throws SystemExit when given such a command line. We also cannot override error of ArgumentParser, because the failing parser is the subparser, which is not a TVMCSuppressedArgumentParser.
Before this patch, this problem already occurs. For example with tvmc run --help, SystemExit is thrown and a partial help message is printed (missing --project-option). So my suggested fix is to move the parse_known_args as far down as possible to yield a mostly complete help message.
There was a problem hiding this comment.
Thanks for investigating! This looks much better, so I won't block on it. Perhaps we can create a separate issue to track this bug?
| for _name, obj in inspect.getmembers(loaded_mod): | ||
| if _is_concrete_extension_type(obj): | ||
| scanned_extensions.append(obj) | ||
| except ImportError as err: |
There was a problem hiding this comment.
would be good to add a test for this case
There was a problem hiding this comment.
I tried adding this, but encountered an issue with race conditions. The way the sys.path is currently modified by extensions.py is problematic when more than one test is run in parallel. Is there already an approach for serializing tests in TVM? There doesn't seem to be an upstream solution: pytest-dev/pytest-xdist#18
There was a problem hiding this comment.
While such an approach should work for single source files, I'm not sure how that would work for packages or namespace packages.
There was a problem hiding this comment.
Apologies for the delay, I had a think about this but wasn't able to come up with a better solution myself, so I won't block on this
|
@tvm-bot rerun |
|
Sorry for the late update. The outstanding test failure is likely because of the outstanding issue that the sys.path is modified in order to support all module loading. This probably inteferres with running the tests in parallel. I'm not sure how to resolve this as I did not find another method to achieve the same kind of module loading. Do we need to compromise here and settle for a simple module load? I unresolved the relevant discussion with @lhutton1. |
…ument handling; lint
PhilippvK
force-pushed
the
patch_uma_tvmc
branch
from
257a26e to
1284f25
Compare
This change introduces:
TVMExtensioninterface to extend TVMC with custom code. Currently its only use is to addUMABackends.--experimental-tvm-extension <dir>to point to extension modules.$TVM_EXTENSION_DIRfor the same purpose.See discussion thread: https://discuss.tvm.apache.org/t/tvmc-uma-integration/14309
@Mousius @cgerum @MichaelJKlaiber @PhilippvK