[Hexagon] Fix RPC session close by adding shutdown PackedFunc#12960
[Hexagon] Fix RPC session close by adding shutdown PackedFunc#12960mehrdadh merged 2 commits intoapache:mainfrom
Conversation
mehrdadh
force-pushed
the
hexagon/fix_rpc_shutdown_issue
branch
3 times, most recently
from
d81753f to
8b05730
Compare
mehrdadh
changed the title
mehrdadh
force-pushed
the
hexagon/fix_rpc_shutdown_issue
branch
from
8b05730 to
4acfafd
Compare
mehrdadh
force-pushed
the
hexagon/fix_rpc_shutdown_issue
branch
from
4acfafd to
701823c
Compare
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Overall, looks good, just a couple of changes to naming/documentation.
src/runtime/rpc/rpc_module.cc
Outdated
| const char* type_key() const final { return "rpc"; } | ||
|
|
||
| PackedFunc GetFunction(const std::string& name, const ObjectPtr<Object>& sptr_to_self) final { | ||
| if (name == "Shutdown") { |
There was a problem hiding this comment.
As I'm reading it, I think we should rename the exposed function to something more descriptive, such as CloseRPCConnection. I know we initially named it Shutdown because that's what the RPCEndpoint function is called, but it could cause confusion at this level. (e.g. "Shutdown" could also mean to power off the remote device.)
src/runtime/rpc/rpc_endpoint.cc
Outdated
| RPCCode RPCEndpoint::HandleUntilReturnEvent(bool client_mode, RPCSession::FEncodeReturn setreturn) { | ||
| RPCCode code = RPCCode::kCallFunc; | ||
|
|
||
| ICHECK(channel_ != nullptr) << "channel is already closed"; |
There was a problem hiding this comment.
Since this would be the first error message seen by a user/developer who runs into it, we should include more context. Also, since it could result from usage rather than being an internal error, we should use CHECK instead of ICHECK.
CHECK(channel_) << "Expected connection to server " << name_ << " to be active, but the connection was previously closed";| /*! | ||
| * \brief The server shutdown cleanup function. | ||
| */ | ||
| void Shutdown(); |
There was a problem hiding this comment.
We should have a more descriptive docstring here, and in the destructor. For the destructor, we should say that it closes the connection, if the connection hasn't already been closed. For the Shutdown function, we should
- Shutdown has no effect if the connection has already been shut down.
- Shutdown will wait for all output currently queued from the RPC connection (i.e. The user doesn't need to wait for completion before calling Shutdown.)
- Any further use of objects that depended on the endpoint (e.g. A
tvm.nd.arrayallocated on the remote RPC session) may throw an exception when used.
| run_pytest ctypes python-contrib-hexagon tests/python/contrib/test_hexagon | ||
| else | ||
| run_pytest ctypes python-contrib-hexagon tests/python/contrib/test_hexagon --tx $num_of_devices*popen --dist=load | ||
| run_pytest ctypes python-contrib-hexagon tests/python/contrib/test_hexagon -n=$num_of_devices |
There was a problem hiding this comment.
This looks like an unrelated change. Should it be part of this PR?
There was a problem hiding this comment.
yeah, this will unblock the physical device (HDK) CI.
| */ | ||
| static std::shared_ptr<RPCSession> Get(int table_index); | ||
|
|
||
| virtual void Shutdown() {} |
mehrdadh
force-pushed
the
hexagon/fix_rpc_shutdown_issue
branch
from
698658a to
501d503
Compare
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Thank you for the changes, and looks good!
|
fixed #12779 |
…#12960) * Add shutdown function to RPC module * address comments
2 participants
This PR adds a shutdown packed function to RPC module so we can force close the RPC session from python side.
cc @areusch @Lunderberg