[TVMC] A simplified TVMC API for python scripting (Part 1). #7823
Conversation
|
@leandron, @areusch, @tkonolige, @CircleSpin can you guys take a look at this PR and let me know what you think? |
|
Thanks for the proposal. I like the OOP approach in general since it clearly represents the available APIs and relationship between objects. I'll spend sometime on reviewing this PR in this week. Meanwhile, for the example, it seems to me that the following design is more OOP style: tvmc_model = tvmc.load(onnx_resnet50)
tvmc_model.tune(target="llvm")
tvmc_package = tvmc_model.compile(target="llvm")
result = tvmc_package.run(device="cpu")In this way, we can clearly see |
|
@comaniac, the OOP approach was something we discussed and are on the fence about implementing. The reason we went with this functional API is because we want this API to be catered to new users trying to understand TVM. These users will often not be familiar with ML deployment and breaking the process down into 4 concrete steps (load, tune, compile, and run) can help crystallize exactly what TVM is doing. I feel that making it object oriented instead somewhat obscures the steps. That said, I do agree that there are benefits to OOP such as discoverability and would be fine adding class methods if you feel strongly that its better. |
@jwfromm I got your point, which is also fair enough. Although I personally prefer the class method approach still, I don't feel it is strongly better than the proposed one, so I'm fine with it if others agree. |
There was a problem hiding this comment.
Given that this is intended to be a high level api for TVM, I think it should be moved out of the TVMC namespace and into the top level. New users (which a target for this), will be confused at the difference between TVM and TVMC.
I also think some tutorials using this api would be good, but those can wait for a later PR.
There was a problem hiding this comment.
I like the idea of making TVMC's API even more accessible. At the same time, this is quite a big refactor in many places.
I was thinking that we could at least split the self-contained change of moving to use Model Library Format as a separate PR, whereas the other changes could stay here?
also cc @u99127 @gromero @manupa-arm @giuseros
| @@ -112,8 +112,10 @@ def drive_run(args): | |||
| except IOError as ex: | |||
| raise TVMCException("Error loading inputs file: %s" % ex) | |||
|
|
|||
| outputs, times = run_module( | |||
| args.FILE, | |||
| tvmc_package = TVMCPackage(package_path=args.FILE) | |||
There was a problem hiding this comment.
I see this as a breaking change with the existing output format. I'm flagging this here to see what others think on whether we should maintain compatibility with our existing custom format or we might as well do the move and use the official APIs and abandon the custom implementation.
There was a problem hiding this comment.
You're right that this change means that packages created by tvmc before this PR will no longer be usable. After talking to @areusch, I'm of the understanding that the plan is to standardize model packaging using export_model_library_format across TVM soon, so I think this is a pain point that will have to be introduced into TVMC eventually. I thought it made sense to bite the bullet now rather than later. However, it would be pretty straight-forward to convert a legacy package to the new format. I'd be happy to provide a helper script that does that if you'd like.
There was a problem hiding this comment.
oh uhh, that's an idea I want to propose, but it hasn't been adopted by RFC or anything. apologies if I miscommunicated that. there are some things we need to work through there around BYOC I think before we can propose it, but I think we can probably accommodate a .so file in Model Library Format with a version bump.
There was a problem hiding this comment.
@areusch, I added .so exporting in this PR. Would you prefer i avoid using export_model_library_format for now?
There was a problem hiding this comment.
well I think this thread was originally about the format produced by --output (@leandron, correct me if I miss something here).
I think it would be to use the metadata and parameter export formats from model_library_format here. I see three areas of tension with doing this:
codegenis intended to be an exploded copy of the.so, so by adding the.so, it creates some confusion as to what exactly is expected to be here. it seems like we should be able to accommodate both source and binary forms in Model Library Format, but it seems like we need to think through things like: a) how should code consuming this format know whether it should expect a.soor source? b) what is the method by which you build the source incodegeninto a binary? in havingcodegensource in Model Library Format, it would almost seem as if you should be able to build it with e.g. amakecommand. But if that's so, also having a binary pre-built seems weird without some additional explanation of what each thing is for, and unit tests that verify those stories.- doing this will expose Model Library Format to a lot more format revisions (which are different from the breaking change identified by @leandron here iiuc). Model Library Format revisions would impact downstream codegen using the microTVM Project API.
- the one I mentioned, which is that Model Library Format doesn't support non-DSO-exportable Modules e.g. CUDA, Vulkan, etc.
i'm not sure what our policy is on maintaining tvmc output formats. I think adding more to the output of tvmc makes sense, but we may need to choose a command-line flag reflective of the format. I think also, perhaps given the tensions with Model Library Format, we should either
a) implement something a bit different here, so there is no confusion
b) do an RFC to resolve at least issue 1, perhaps we can come to consensus on a way to place the .so in Model Library Format
maybe since it seems like we should preface this PR with an RFC explaining the API, option b isn't so bad? wdyt?
There was a problem hiding this comment.
yeah that's fair. apologies for the miscommunication here.
one question though: what documentation will we require for the custom packing format? as this is tvmc, the bar should be higher than just reading the code, imo.
There was a problem hiding this comment.
My goal is that the user doesn't need to acknowledge or understand the packing format at all. TVMCModel and TVMCPackage completely handle it.
There was a problem hiding this comment.
@leandron, I've reverted this PR to using the existing custom packaging format in TVMC. Existing packages should now be fully compatible after this refactor.
There was a problem hiding this comment.
Thanks @jwfromm I'll have a look.
(the comment got a bit long, but I thought to put the idea here and see what you think, as we probably got everybody in the PR anyway).
@areusch I think we have the opportunity to make Model Library Format (MLF) to be the way we serialize TVM outputs. Obviously, due to the nature of the outputs we produce from different backends (object files, shared libraries, sources), it can contain various artifacts within a tarball, and for that I think we could make better use of the metadata descriptor file, to describe what is in there in a way that we programmatically understand, for a given MLF package.
This would open the door to, for example, on Project API, we check whether a given tarball is created from a "micro" target and can be used to integrate with an RTOS. In TVMC, we can define whether the package aims, cpu, cuda, cl, etc. and whether it includes the shared library, etc.
With an abstraction like TVMCPackage (maybe renamed) we could make those features to be easily available for target applications already using TVM, to make use of the packages.
the one I mentioned, which is that Model Library Format doesn't support non-DSO-exportable Modules e.g. CUDA, Vulkan, etc.
I think I saw you mentioning this limitation in the RFC as well, but I'm curious to understand a bit more on where is the limitation? is that on TVM side, or on the targets, such as Vulkan, that we can't export them?
There was a problem hiding this comment.
@leandron It's a bit of a long explanation, I will see if I can summarize. Currently a codegen backend is expected to produce runtime::Module. When either the load mechanism (if an existing e.g. CSourceModule could possibly be used) or the format (when producing e.g. not C) differs from one checked-in, a new Module must be defined. For this reason, many codegen produce aggregate modules containing multiple pieces of data. See CUDA for an example.
Worse yet, Modules are expected to implement their own load() and save(), and it is possible (though not likely now) that you could produce a Module in codegen which, after save() and load(), don't behave the same. It's possible we are not entirely unit testing now what we deploy.
Finally, Modules aren't labelled--so while it kind of makes sense how we've populated Model Library Format given target_host-targeted LLVM and C modules, it's difficult to implement any sort of generic organizational scheme.
My opinion is that the solution to this problem is generating a set of POD types (e.g. structs) from the compiler with labels and overhauling the module load process to better align it with the implied load process we are asking users to implement in microTVM. Specifically, it should be easy to document a set of steps that the user needs to implement on their own in order to arrive at an e.g. GraphExecutor from a given SoC after device reset.
There was a problem hiding this comment.
@jwfromm I did a pass here with an eye towards ensuring clarity in the API. there are a lot of inputs to TVMC, so I think it would be good, if we are introducing top-level data structures, to consider some organization now rather than letting things occupy the same top-level namespace. I also think the bar for docs should be a bit higher than normal here, since it's intended to be user-facing.
| @@ -112,8 +112,10 @@ def drive_run(args): | |||
| except IOError as ex: | |||
| raise TVMCException("Error loading inputs file: %s" % ex) | |||
|
|
|||
| outputs, times = run_module( | |||
| args.FILE, | |||
| tvmc_package = TVMCPackage(package_path=args.FILE) | |||
There was a problem hiding this comment.
yeah that's fair. apologies for the miscommunication here.
one question though: what documentation will we require for the custom packing format? as this is tvmc, the bar should be higher than just reading the code, imo.
|
@areusch I've addressed or asked questions about all your feedback. Could you take another look and let me know what you think? With regards to the custom packaging format, I don't think there's a big need for detailed documentation. The main motivation for introducing TVMCModel and TVMCPackage is to make it so users don't have to acknowledge the packing format or even the packaged files if they dont want to. Only advanced users, who should be comfortable reading the code, would ever have any need to dig into the packaging. |
| Compilation target as plain string, inline JSON or path to a JSON file. | ||
| tuning_records: str, optional | ||
| The path to a file that tuning results will be saved to. If not specified, | ||
| a temporary file will be used. |
There was a problem hiding this comment.
Given that tuning record is the most important result of runing .tune, it is even worthwhile to enforce this argument to be specified by users, IMHO.
Meanwhile, it might be better to just eliminate prior_records. When tuning_records has prior records, we could directly use them to hot-start the tuning. It's a bit confusing to have these two arguments especially both autotvm and auto-scheduler support tuning resuming and append new records to the file without overriding existing records.
python/tvm/driver/tvmc/autotuner.py
Outdated
| ) | ||
|
|
||
| # If not specified, choose a number of trials likely to produce good results. | ||
| if trials is None: | ||
| trials = 10000 |
There was a problem hiding this comment.
I agree that we should not hide a constant here. Since trials is an argument of this function and it could even be popogated from the CLI. We could just set the default value of the function and CLI argument.
In general, any value we set in this function should have a corresponding message to let users know what value they are using. For example, the logging level of showing min_repeat_ms value should be INFO instead of DEBUG IMHO. A more formal approach is displaying a table of configuration with all values being used before tuning, so that users can double check if they miss anything.
jwfromm
changed the title
|
@leandron please take a look when you have a minute |
|
Thanks @jwfromm @tkonolige @leandron @areusch |
As noted in many of @CircleSpin's recent PRs, TVMC has an excellent set of utilities to make python scripting in TVM a more approachable and intuitive experience for new users. This PR introduces an object oriented approach that we believe will be a much smoother introduction to working in TVM from Python. Before jumping into the design discussion, I want to note that we were careful to make sure that the command line TVMC workflow was minimally effected by this change.
In short, the goal this PR is to reduce a full TVM workflow into a small set of intuitive function calls, one example of which can be seen in
test_model.py, which I'll include here for discussion.The above code uses three new classes defined in
tvmc/model.py:TVMCModel,TVMCPackage, andTVMCResult.A
TVMCModelis a wrapper around a precompiled model in TVMC. It can be generated either by callingtvmc.loador by pointing it to a previously saved TVMCModel. The class itself is very straightforward and most of its functionality is for saving and loading itself from disk and producing TVMCPackages. Being able to save and load TVMCModel's directly is convenient when users dont want to have to run a relay importer everytime they start up a new python instance.Calling
tvmc.compileon a TVMCModel now will produce a package usingexport_model_library_formatwhich was introduced in PR #7533. This format is actually very similar to what TVMC was previously using and will likely become the standard way for saving compiled artifacts going forward. The produced package is then used to create a TVMCPackage object which contains all the information needed to run the model.The TVMCPackage is passed to
tvmc.runwhich now returns aTVMCResult, a very simple wrapper around the runtime measurements and produced results.I'd love to hear what you guys think of this flow and design and am happy to discuss any decisions in more detail. Assuming that this work is well received, we're planning on writing a tutorial for new users based on it.
Also included in this PR are a change to
auto_scheduler.HardwareParamssuch that all arguments are optional. When an argument is provided asNone, it will now extract the default value for the current target. I've also changed the default values for hardware parameters in TVMC to be None to reflect this change. This should make the default behavior of tvmc more consistent with other workflows.The goal of this first PR is to introduce the core Python API while minimally effecting the TVMC command-line experience. In follow-up PRs, we'll work to improve both the Python API and command line API with more targeted changes.