You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/03/02 01:02:06 UTC
[GitHub] [tvm] tkonolige opened a new pull request #7559: [RUNTIME] Unify load params interface
tkonolige opened a new pull request #7559:
URL: https://github.com/apache/tvm/pull/7559
This allows the VM to load parameters just using the runtime. This way we can run a model in the runtime the same way we can with the graph runtime.
I had to move Map into the runtime, so the diff is large.
@jroesch @areusch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#discussion_r589691387
##########
File path: src/runtime/graph/graph_runtime.cc
##########
@@ -196,31 +198,10 @@ void GraphRuntime::LoadParams(const std::string& param_blob) {
}
void GraphRuntime::LoadParams(dmlc::Stream* strm) {
- uint64_t header, reserved;
- ICHECK(strm->Read(&header)) << "Invalid parameters file format";
- ICHECK(header == kTVMNDArrayListMagic) << "Invalid parameters file format";
- ICHECK(strm->Read(&reserved)) << "Invalid parameters file format";
-
- std::vector<std::string> names;
- ICHECK(strm->Read(&names)) << "Invalid parameters file format";
- uint64_t sz;
- strm->Read(&sz);
- size_t size = static_cast<size_t>(sz);
- ICHECK(size == names.size()) << "Invalid parameters file format";
- for (size_t i = 0; i < size; ++i) {
- int in_idx = GetInputIndex(names[i]);
- if (in_idx < 0) {
- NDArray temp;
- temp.Load(strm);
- continue;
- }
- uint32_t eid = this->entry_id(input_nodes_[in_idx], 0);
- ICHECK_LT(eid, data_entry_.size());
-
- // The data_entry is allocated on device, NDArray.load always load the array into CPU.
- NDArray temp;
- temp.Load(strm);
- data_entry_[eid].CopyFrom(temp);
+ Map<String, NDArray> params = ::tvm::runtime::LoadParams(strm);
+ for (auto& p : params) {
+ uint32_t eid = this->entry_id(input_nodes_[GetInputIndex(p.first)], 0);
+ data_entry_[eid].CopyFrom(p.second);
Review comment:
That's not possible. The data has to be loaded onto the cpu and then copied to the device.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#discussion_r585981227
##########
File path: src/runtime/container.cc
##########
@@ -79,5 +79,100 @@ TVM_REGISTER_OBJECT_TYPE(ADTObj);
TVM_REGISTER_OBJECT_TYPE(StringObj);
TVM_REGISTER_OBJECT_TYPE(ClosureObj);
+TVM_REGISTER_OBJECT_TYPE(ArrayNode);
+
+TVM_REGISTER_GLOBAL("node.Array").set_body([](TVMArgs args, TVMRetValue* ret) {
Review comment:
addressed in #7570
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789250613
@junrushao1994 I think prematurely optimizing for the runtime library size is a mistake, there are other things like error handling, etc which we want to pull into the runtime which will require Map. My understanding was main push on slim runtime was from FB folks who no longer are actively contributing. We should add it to the runtime unless there is a really compelling reason to not have it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789275067
@junrushao1994 @tqchen thanks! this helps support some refactors I want to do as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789253983
OK, I think perhaps it makes sense to bring Map into runtime given the the goal of slim runtime is covered by uTVM CRT. Let us open another PR for the Map migration though. @junrushao1994 can you do that given you wrote the original map ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789253404
My understand is that if we really want a slim runtime, we should go with utvm @areusch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#discussion_r585972034
##########
File path: src/runtime/file_utils.h
##########
@@ -92,6 +94,44 @@ void LoadMetaDataFromFile(const std::string& file_name,
* \param file_name The file name.
*/
void RemoveFile(const std::string& file_name);
+
+constexpr uint64_t kTVMNDArrayListMagic = 0xF7E58D4F05049CB7;
+/*!
+ * \brief Load parameters from a string.
+ * \param param_blob Serialized string of parameters.
+ * \return Map of parameter name to parameter value.
+ */
+Map<String, NDArray> LoadParams(const std::string& param_blob);
+/*!
+ * \brief Load parameters from a stream.
+ * \param strm Stream to load parameters from.
+ * \return Map of parameter name to parameter value.
+ */
+Map<String, NDArray> LoadParams(dmlc::Stream* strm);
+// /*!
Review comment:
intentional?
##########
File path: src/runtime/container.cc
##########
@@ -79,5 +79,100 @@ TVM_REGISTER_OBJECT_TYPE(ADTObj);
TVM_REGISTER_OBJECT_TYPE(StringObj);
TVM_REGISTER_OBJECT_TYPE(ClosureObj);
+TVM_REGISTER_OBJECT_TYPE(ArrayNode);
+
+TVM_REGISTER_GLOBAL("node.Array").set_body([](TVMArgs args, TVMRetValue* ret) {
Review comment:
I think it's weird to have the term "node" in the runtime
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#discussion_r589686703
##########
File path: src/runtime/graph/graph_runtime.cc
##########
@@ -196,31 +198,10 @@ void GraphRuntime::LoadParams(const std::string& param_blob) {
}
void GraphRuntime::LoadParams(dmlc::Stream* strm) {
- uint64_t header, reserved;
- ICHECK(strm->Read(&header)) << "Invalid parameters file format";
- ICHECK(header == kTVMNDArrayListMagic) << "Invalid parameters file format";
- ICHECK(strm->Read(&reserved)) << "Invalid parameters file format";
-
- std::vector<std::string> names;
- ICHECK(strm->Read(&names)) << "Invalid parameters file format";
- uint64_t sz;
- strm->Read(&sz);
- size_t size = static_cast<size_t>(sz);
- ICHECK(size == names.size()) << "Invalid parameters file format";
- for (size_t i = 0; i < size; ++i) {
- int in_idx = GetInputIndex(names[i]);
- if (in_idx < 0) {
- NDArray temp;
- temp.Load(strm);
- continue;
- }
- uint32_t eid = this->entry_id(input_nodes_[in_idx], 0);
- ICHECK_LT(eid, data_entry_.size());
-
- // The data_entry is allocated on device, NDArray.load always load the array into CPU.
- NDArray temp;
- temp.Load(strm);
- data_entry_[eid].CopyFrom(temp);
+ Map<String, NDArray> params = ::tvm::runtime::LoadParams(strm);
+ for (auto& p : params) {
+ uint32_t eid = this->entry_id(input_nodes_[GetInputIndex(p.first)], 0);
+ data_entry_[eid].CopyFrom(p.second);
Review comment:
possible to do this zero-copy?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a change in pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
tkonolige commented on a change in pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#discussion_r585978336
##########
File path: src/runtime/file_utils.h
##########
@@ -92,6 +94,44 @@ void LoadMetaDataFromFile(const std::string& file_name,
* \param file_name The file name.
*/
void RemoveFile(const std::string& file_name);
+
+constexpr uint64_t kTVMNDArrayListMagic = 0xF7E58D4F05049CB7;
+/*!
+ * \brief Load parameters from a string.
+ * \param param_blob Serialized string of parameters.
+ * \return Map of parameter name to parameter value.
+ */
+Map<String, NDArray> LoadParams(const std::string& param_blob);
+/*!
+ * \brief Load parameters from a stream.
+ * \param strm Stream to load parameters from.
+ * \return Map of parameter name to parameter value.
+ */
+Map<String, NDArray> LoadParams(dmlc::Stream* strm);
+// /*!
Review comment:
whoops
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-790822382
@areusch @junrushao1994 Map is now in runtime. Can you re-review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789254640
I left the map migration as a separate commit. I'll cherry-pick and submit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 merged pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #7559:
URL: https://github.com/apache/tvm/pull/7559
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-788678473
Thanks for the contribution Tristan! Would you like to elaborate why we need to move the Map to runtime? If we decide to do it, probably we need a separate PR to make things more reviewable
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#discussion_r589720788
##########
File path: src/runtime/graph/graph_runtime.cc
##########
@@ -196,31 +198,10 @@ void GraphRuntime::LoadParams(const std::string& param_blob) {
}
void GraphRuntime::LoadParams(dmlc::Stream* strm) {
- uint64_t header, reserved;
- ICHECK(strm->Read(&header)) << "Invalid parameters file format";
- ICHECK(header == kTVMNDArrayListMagic) << "Invalid parameters file format";
- ICHECK(strm->Read(&reserved)) << "Invalid parameters file format";
-
- std::vector<std::string> names;
- ICHECK(strm->Read(&names)) << "Invalid parameters file format";
- uint64_t sz;
- strm->Read(&sz);
- size_t size = static_cast<size_t>(sz);
- ICHECK(size == names.size()) << "Invalid parameters file format";
- for (size_t i = 0; i < size; ++i) {
- int in_idx = GetInputIndex(names[i]);
- if (in_idx < 0) {
- NDArray temp;
- temp.Load(strm);
- continue;
- }
- uint32_t eid = this->entry_id(input_nodes_[in_idx], 0);
- ICHECK_LT(eid, data_entry_.size());
-
- // The data_entry is allocated on device, NDArray.load always load the array into CPU.
- NDArray temp;
- temp.Load(strm);
- data_entry_[eid].CopyFrom(temp);
+ Map<String, NDArray> params = ::tvm::runtime::LoadParams(strm);
+ for (auto& p : params) {
+ uint32_t eid = this->entry_id(input_nodes_[GetInputIndex(p.first)], 0);
+ data_entry_[eid].CopyFrom(p.second);
Review comment:
ok nvm, you're right
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 edited a comment on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
junrushao1994 edited a comment on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789253404
My understanding is that if we really want a slim runtime, we should go with utvm @areusch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789258221
@tkonolige Feel free to submit a separate PR and let's merge it in quickly
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #7559: [RUNTIME] Unify load params interface
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #7559:
URL: https://github.com/apache/tvm/pull/7559#issuecomment-789031004
LoadParams is a free standing function that we would like to use at runtime with the VM (because there is no clear way to directly load parameters into the VM). It makes the most sense that this function returns a Map from parameter name to NDArray.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org