[RUNTIME] Unify load params interface #7559
Conversation
|
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 |
|
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. |
|
@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. |
|
My understanding is that if we really want a slim runtime, we should go with utvm @areusch |
|
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 ? |
|
I left the map migration as a separate commit. I'll cherry-pick and submit |
|
@tkonolige Feel free to submit a separate PR and let's merge it in quickly |
|
@junrushao1994 @tqchen thanks! this helps support some refactors I want to do as well. |
src/runtime/file_utils.h
Outdated
| * \return Map of parameter name to parameter value. | ||
| */ | ||
| Map<String, NDArray> LoadParams(dmlc::Stream* strm); | ||
| // /*! |
src/runtime/container.cc
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
I think it's weird to have the term "node" in the runtime
There was a problem hiding this comment.
addressed in #7570
tkonolige
force-pushed
the
runtime_params_serialize
branch
from
002fe4d to
512f088
Compare
|
@areusch @junrushao1994 Map is now in runtime. Can you re-review? |
This allows the VM to load parameters just using the runtime.
tkonolige
force-pushed
the
runtime_params_serialize
branch
from
9cc0087 to
97d194b
Compare
| 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); |
There was a problem hiding this comment.
possible to do this zero-copy?
There was a problem hiding this comment.
That's not possible. The data has to be loaded onto the cpu and then copied to the device.
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