-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Runtime] Serialization/Deserialization of runtime module #15244
[Runtime] Serialization/Deserialization of runtime module #15244
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 |
src/node/structural_hash.cc
Outdated
const auto* rtmod = static_cast<const runtime::ModuleNode*>(n); | ||
std::string blob; | ||
dmlc::MemoryStringStream mstrm(&blob); | ||
support::Base64OutStream b64strm(&mstrm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not sufficient, we will need to collect the import tree, as in normal export, so not only module itself get exported, but also the imported modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch! This is addressed by using https://github.com/apache/tvm/blob/main/src/runtime/dso_library.cc#L152 which also collects import tree.
Thanks @sunggg. The main comment is to include import tree information so we can recover modules that imports each other(e.g. a host module that imports cuda). We can try to refactor and bring out logic in https://github.com/apache/tvm/blob/main/src/target/codegen.cc#L63 to achieve that goal. Also we need to check if the modules are all serializable and report error accordingly if they are not |
Thanks for the valuable feedback, @tqchen! To check if the modules are all exportable, it leverages the check introduced by #14406. AssertionError: Module RelayGraphRuntimeCodegenModule should be either dso exportable or binary serializable. |
@tvm-bot rerun |
Thank you, @tqchen for the suggestion. By reflecting your feedback, the latest commit cleans-up the APIs as follows: // support full roundtrip of `runtime::Module` if the root and its all submodules are binary serializable
// These are used for structural hash of `runtime::Module` and thus also used for `SaveJson` and `LoadJson`
// Please note that `SerializeModuleToBytes` supports `include_dso` to keep the existing usage (e.g., `PackImportsToC`).
// When `include_dso` is disabled and it encounters `DSOModules`, error will be raised.
std::string SerializeModuleToBytes(const runtime::Module& mod, bool include_dso = true);
runtime::Module DeserializeModuleFromBytes(std::string blob);
// supports export/import of `runtime::Module` if the root and all submodules are either binary serializable or dso exportable
// this is currently used for pytorch integration: https://github.com/apache/tvm/pull/12232
std::string ExportModuleToBase64(tvm::runtime::Module module);
runtime::Module ImportModuleFromBase64(std::string base64str); Whenever each API faces module/submodule that is not expected, the error will be triggered. Also, |
6477301
to
f52ef3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sunggg some followup comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final nits and we are good to go
src/support/base64.h
Outdated
@@ -293,6 +293,40 @@ class Base64OutStream : public dmlc::Stream { | |||
} | |||
} | |||
}; | |||
|
|||
inline size_t b64strlen(const std::string b64str) { | |||
ICHECK(b64str.size() % 4 == 0) << "invalid base64 encoding"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these two functions to callers, since they are not necessarily the optimal way of decoding b64. The b64Stream is the preferred way. So avoid making them in support(and gradually we move the contrib impl over, or keep them self-contained to a sub module scope) is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I moved them to contrib side (src/contrib/torch/tvm_module_wrapper/RuntimeModuleWrapperTVM.cc
) since it is the only user currently.
Hey just wanted to chime in a little bit - are we on the right track to get it in? Seems an important PR |
Thanks for the ping, this is ready and merged |
This PR implements serialization and deserialization of such runtime module by registering
.set_creator
and.set_repr_bytes
.If runtime module is binary serializable (see #14406), its roundtrip is possible.
When the runtime module is not binary serializable, such as csource_module, it will raise an error.
cc. @tqchen