[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419739620 ## File path: .gitignore ## @@ -2,9 +2,10 @@ __pycache__/ *.py[cod] *$py.class - +*.S Review comment: try to ignore assembly files generated in commands. 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419704899 ## File path: python/tvm/_ffi/base.py ## @@ -48,8 +48,11 @@ def _load_lib(): """Load libary by searching possible path.""" lib_path = libinfo.find_lib_path() lib = ctypes.CDLL(lib_path[0], ctypes.RTLD_GLOBAL) -# DMatrix functions lib.TVMGetLastError.restype = ctypes.c_char_p +# Put the libpath to LD_LIBRARY_PATH +# will be useful for pipe session to find libtvm +os.environ["LD_LIBRARY_PATH"] = "%s:%s" % ( Review comment: Note that the change to LD_LIBRARY_PATH won't affert the execution after the python script exits. I still think it makes sense to make the libary path in the installation to be available for sub-processes 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419702687 ## File path: python/tvm/contrib/cc.py ## @@ -90,7 +90,8 @@ def get_target_triple(): def cross_compiler(compile_func, options=None, output_format=None, - get_target_triple=None): + get_target_triple=None, + add_files=None): Review comment: I think your understanding is correct 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419702240
##
File path: python/tvm/rpc/client.py
##
@@ -183,28 +198,37 @@ class LocalSession(RPCSession):
need to be ran both locally and remotely.
"""
def __init__(self):
-# pylint: disable=super-init-not-called
-self.context = nd.context
-self.get_function = tvm._ffi.get_global_func
-self._temp = util.tempdir()
+self._temp = server._server_env([])
+RPCSession.__init__(self, _ffi_api.LocalSession())
-def upload(self, data, target=None):
-if isinstance(data, bytearray):
-if not target:
-raise ValueError("target must present when file is a
bytearray")
-blob = data
-else:
-blob = bytearray(open(data, "rb").read())
-if not target:
-target = os.path.basename(data)
-with open(self._temp.relpath(target), "wb") as f:
-f.write(blob)
-def download(self, path):
-return bytearray(open(self._temp.relpath(path), "rb").read())
+@tvm._ffi.register_func("rpc.PopenSession")
+def _popen_session(binary):
+temp = util.tempdir()
-def load_module(self, path):
-return _load_module(self._temp.relpath(path))
+if isinstance(binary, (bytes, bytearray)):
+path_exec = temp.relpath("server.minrpc")
+with open(path_exec, "wb") as outfile:
+outfile.write(binary)
+os.chmod(path_exec, stat.S_IXUSR)
+path_exec = os.path.abspath(path_exec)
+else:
+path_exec = os.path.abspath(binary)
Review comment:
You are right that there is none atm, we could certainly skip this part.
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419701703
##
File path: src/runtime/rpc/minrpc/minrpc_server.h
##
@@ -0,0 +1,598 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file minrpc_server.h
+ * \brief Minimum RPC server implementation,
+ *redirects all the calls to C runtime API.
+ *
+ * \note This file do not depend on c++ std or c std,
+ * and only depends on TVM's C runtime API.
+ */
+#ifndef TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+#define TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+
+#include
+#include
+#include "../rpc_protocol.h"
+#include "../../../support/arena.h"
+
+/*! \brief Whether or not to enable glog style DLOG */
+#ifndef TVM_MINRPC_ENABLE_LOGGING
+#define TVM_MINRPC_ENABLE_LOGGING 0
+#endif
+
+#ifndef MINRPC_CHECK
+#define MINRPC_CHECK(cond) \
+ if (!(cond)) this->ThrowError(RPCServerStatus::kCheckError);
+#endif
+
+#if TVM_MINRPC_ENABLE_LOGGING
+#include
+#endif
+
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief A minimum RPC server that only depends on the tvm C runtime..
+ *
+ * All the dependencies are provided by the io arguments.
+ *
+ * \tparam TIOHandler IO provider to provide io handling.
+ * An IOHandler needs to provide the following functions:
+ * - PosixWrite, PosixRead, Close: posix style, read, write, close API.
+ * - Exit: exit with status code.
+ */
+template
+class MinRPCServer {
+ public:
+ /*!
+ * \brief Constructor.
+ * \param io The IO handler.
+ */
+ explicit MinRPCServer(TIOHandler io)
+ : io_(io), arena_(PageAllocator(io)) {}
+
+ /*! \brief Run the server loop until shutdown signal is received. */
+ void ServerLoop() {
+RPCCode code;
+uint64_t packet_len;
+
+while (true) {
+ arena_.RecycleAll();
+ allow_clean_shutdown_ = true;
+
+ this->Read(_len);
+ if (packet_len == 0) continue;
+ this->Read();
+
+ allow_clean_shutdown_ = false;
+
+ if (code >= RPCCode::kSyscallCodeStart) {
+this->HandleSyscallFunc(code);
+ } else {
+switch (code) {
+ case RPCCode::kCallFunc: {
+HandleNormalCallFunc();
+break;
+ }
+ case RPCCode::kInitServer: {
+HandleInitServer();
+break;
+ }
+ case RPCCode::kCopyFromRemote: {
+HandleCopyFromRemote();
+break;
+ }
+ case RPCCode::kCopyToRemote: {
+HandleCopyToRemote();
+break;
+ }
+ case RPCCode::kShutdown: {
+this->Shutdown();
+return;
+ }
+ default: {
+this->ThrowError(RPCServerStatus::kUnknownRPCCode);
+break;
+ }
+}
+ }
+}
+ }
+
+ void Shutdown() {
+arena_.FreeAll();
+io_.Close();
+ }
+
+ void HandleNormalCallFunc() {
+uint64_t call_handle;
+TVMValue* values;
+int* tcodes;
+int num_args;
+TVMValue ret_value[3];
+int ret_tcode[3];
+
+this->Read(_handle);
+RecvPackedSeq(, , _args);
+
+int call_ecode = TVMFuncCall(
+reinterpret_cast(call_handle),
+values, tcodes, num_args,
+&(ret_value[1]), &(ret_tcode[1]));
+
+if (call_ecode == 0) {
+ // Return value encoding as in LocalSession
+ int rv_tcode = ret_tcode[1];
+ ret_tcode[0] = kDLInt;
+ ret_value[0].v_int64 = rv_tcode;
+ if (rv_tcode == kTVMNDArrayHandle) {
+ret_tcode[1] = kTVMDLTensorHandle;
+ret_value[2].v_handle = ret_value[1].v_handle;
+ret_tcode[2] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 3);
+ } else if (rv_tcode == kTVMPackedFuncHandle ||
+ rv_tcode == kTVMModuleHandle) {
+ret_tcode[1] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ } else {
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ }
+} else {
+ this->ReturnLastTVMError();
+}
+ }
+
+ void HandleCopyFromRemote() {
+uint64_t handle, offset, num_bytes;
+TVMContext ctx;
+
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419691019
##
File path: src/runtime/rpc/rpc_endpoint.cc
##
@@ -0,0 +1,1059 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file rpc_session.cc
+ * \brief RPC session for remote function call.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "rpc_endpoint.h"
+#include "rpc_local_session.h"
+#include "../object_internal.h"
+#include "../../support/ring_buffer.h"
+#include "../../support/arena.h"
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * Event-driven state-machine based handlers for RPCEndpoint.
+ *
+ * Key functions:
+ *
+ * - SendPackedSeq: send the arguments over to the peer
+ * - HandleNextEvent: handle the next request from the peer(RPCCode followed
by per code protocol).
+ */
+class RPCEndpoint::EventHandler : public dmlc::Stream {
+ public:
+ EventHandler(support::RingBuffer* reader,
+ support::RingBuffer* writer,
+ std::string name,
+ std::string* remote_key)
+ : reader_(reader),
+writer_(writer),
+name_(name),
+remote_key_(remote_key) {
+this->Clear();
+
+if (*remote_key == "%toinit") {
+ state_ = kInitHeader;
+ remote_key_->resize(0);
+ pending_request_bytes_ = sizeof(int32_t);
+}
+ }
+
+ /*!
+ * \brief Bytes needed to fulfill current request
+ */
+ size_t BytesNeeded() const {
+if (reader_->bytes_available() < pending_request_bytes_) {
+ return pending_request_bytes_ - reader_->bytes_available();
+} else {
+ return 0;
+}
+ }
+
+ /*!
+ * \brief Request number of bytes from the reader.
+ * \param nbytes The number of bytes
+ */
+ void RequestBytes(size_t nbytes) {
+pending_request_bytes_ += nbytes;
+reader_->Reserve(pending_request_bytes_);
+ }
+
+ /*! \return Whether we are ready to handle next request. */
+ bool Ready() const {
+return reader_->bytes_available() >= pending_request_bytes_;
+ }
+
+ /*! \return Whether we can perform a clean shutdown */
+ bool CanCleanShutdown() const {
+return state_ == kRecvPacketNumBytes;
+ }
+
+ /*! \brief Finish the copy ack stage. */
+ void FinishCopyAck() {
+this->SwitchToState(kRecvPacketNumBytes);
+ }
+
+ /*!
+ * \brief Enter the io loop until the next event.
+ * \param client_mode Whether we are in the client.
+ * \param setreturn The function to set the return value encoding.
+ * \return The function to set return values when there is a return event.
+ */
+ RPCCode HandleNextEvent(bool client_mode, RPCSession::FEncodeReturn
setreturn) {
+std::swap(client_mode_, client_mode);
+
+while (this->Ready()) {
+ switch (state_) {
+case kInitHeader: HandleInitHeader(); break;
+case kRecvPacketNumBytes: {
+ uint64_t packet_nbytes;
+ CHECK(this->Read(_nbytes));
+ if (packet_nbytes != 0) {
Review comment:
there is no max limit for now.
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419688468
##
File path: src/runtime/rpc/minrpc/minrpc_server.h
##
@@ -0,0 +1,598 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file minrpc_server.h
+ * \brief Minimum RPC server implementation,
+ *redirects all the calls to C runtime API.
+ *
+ * \note This file do not depend on c++ std or c std,
+ * and only depends on TVM's C runtime API.
+ */
+#ifndef TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+#define TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+
+#include
+#include
+#include "../rpc_protocol.h"
+#include "../../../support/arena.h"
+
+/*! \brief Whether or not to enable glog style DLOG */
+#ifndef TVM_MINRPC_ENABLE_LOGGING
+#define TVM_MINRPC_ENABLE_LOGGING 0
+#endif
+
+#ifndef MINRPC_CHECK
+#define MINRPC_CHECK(cond) \
+ if (!(cond)) this->ThrowError(RPCServerStatus::kCheckError);
+#endif
+
+#if TVM_MINRPC_ENABLE_LOGGING
+#include
+#endif
+
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief A minimum RPC server that only depends on the tvm C runtime..
+ *
+ * All the dependencies are provided by the io arguments.
+ *
+ * \tparam TIOHandler IO provider to provide io handling.
+ * An IOHandler needs to provide the following functions:
+ * - PosixWrite, PosixRead, Close: posix style, read, write, close API.
+ * - Exit: exit with status code.
+ */
+template
+class MinRPCServer {
+ public:
+ /*!
+ * \brief Constructor.
+ * \param io The IO handler.
+ */
+ explicit MinRPCServer(TIOHandler io)
+ : io_(io), arena_(PageAllocator(io)) {}
+
+ /*! \brief Run the server loop until shutdown signal is received. */
+ void ServerLoop() {
+RPCCode code;
+uint64_t packet_len;
+
+while (true) {
+ arena_.RecycleAll();
+ allow_clean_shutdown_ = true;
+
+ this->Read(_len);
+ if (packet_len == 0) continue;
+ this->Read();
+
+ allow_clean_shutdown_ = false;
+
+ if (code >= RPCCode::kSyscallCodeStart) {
+this->HandleSyscallFunc(code);
+ } else {
+switch (code) {
+ case RPCCode::kCallFunc: {
+HandleNormalCallFunc();
+break;
+ }
+ case RPCCode::kInitServer: {
+HandleInitServer();
+break;
+ }
+ case RPCCode::kCopyFromRemote: {
+HandleCopyFromRemote();
+break;
+ }
+ case RPCCode::kCopyToRemote: {
+HandleCopyToRemote();
+break;
+ }
+ case RPCCode::kShutdown: {
+this->Shutdown();
+return;
+ }
+ default: {
+this->ThrowError(RPCServerStatus::kUnknownRPCCode);
+break;
+ }
+}
+ }
+}
+ }
+
+ void Shutdown() {
+arena_.FreeAll();
+io_.Close();
+ }
+
+ void HandleNormalCallFunc() {
+uint64_t call_handle;
+TVMValue* values;
+int* tcodes;
+int num_args;
+TVMValue ret_value[3];
+int ret_tcode[3];
+
+this->Read(_handle);
+RecvPackedSeq(, , _args);
+
+int call_ecode = TVMFuncCall(
+reinterpret_cast(call_handle),
+values, tcodes, num_args,
+&(ret_value[1]), &(ret_tcode[1]));
+
+if (call_ecode == 0) {
+ // Return value encoding as in LocalSession
+ int rv_tcode = ret_tcode[1];
+ ret_tcode[0] = kDLInt;
+ ret_value[0].v_int64 = rv_tcode;
+ if (rv_tcode == kTVMNDArrayHandle) {
+ret_tcode[1] = kTVMDLTensorHandle;
+ret_value[2].v_handle = ret_value[1].v_handle;
+ret_tcode[2] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 3);
+ } else if (rv_tcode == kTVMPackedFuncHandle ||
+ rv_tcode == kTVMModuleHandle) {
+ret_tcode[1] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ } else {
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ }
+} else {
+ this->ReturnLastTVMError();
+}
+ }
+
+ void HandleCopyFromRemote() {
+uint64_t handle, offset, num_bytes;
+TVMContext ctx;
+
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419687967
##
File path: src/runtime/rpc/minrpc/posix_popen_server.cc
##
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Disable constructor to bring minimum dep on c++ABI.
+#define TVM_ARENA_HAS_DESTRUCTOR 0
+
+#include
+#include
+#include "minrpc_server.h"
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief IOHandler based on posix API.
+ */
+class PosixIOHandler {
Review comment:
Good point, perhaps we can add that as a followup.
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419687452
##
File path: src/runtime/rpc/rpc_pipe_impl.cc
##
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file rpc_pipe_impl.cc
+ * \brief Pipe-based RPC channel.
+ */
+// Linux only for now, as linux is the most common usecase.
+#if defined(__linux__) || defined(__ANDROID__)
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "rpc_endpoint.h"
+#include "rpc_local_session.h"
+#include "../../support/pipe.h"
+
+namespace tvm {
+namespace runtime {
+
+class PipeChannel final : public RPCChannel {
+ public:
+ explicit PipeChannel(int readfd, int writefd, pid_t child_pid)
+ : readfd_(readfd), writefd_(writefd), child_pid_(child_pid) {
+ }
+
+ ~PipeChannel() {
+Close();
+ }
+
+ size_t Send(const void* data, size_t size) final {
+ssize_t n = write(writefd_, data, size);
+if (n == -1) {
+ LOG(FATAL) << "Pipe write error";
+}
+return static_cast(n);
+ }
+
+ size_t Recv(void* data, size_t size) final {
+ssize_t n = read(readfd_, data, size);
+if (n == -1) {
+ LOG(FATAL) << "Pipe read error";
+}
+return static_cast(n);
+ }
+
+ void Close() {
+close(readfd_);
+close(writefd_);
+kill(child_pid_, SIGKILL);
+ }
+
+ private:
+ int readfd_;
+ int writefd_;
+ pid_t child_pid_;
+};
+
+
+Module CreatePipeClient(std::vector cmd) {
+ int parent2child[2];
+ int child2parent[2];
+ CHECK_EQ(pipe(parent2child), 0);
+ CHECK_EQ(pipe(child2parent), 0);
+
+ int parent_read = child2parent[0];
+ int parent_write = parent2child[1];
+ int child_read = parent2child[0];
+ int child_write = child2parent[1];
+
+ pid_t pid = fork();
+ if (pid == 0) {
+// child process
+close(parent_read);
+close(parent_write);
+std::string sread_pipe = std::to_string(child_read);
Review comment:
do you mean dup2 to stdio? My only worry is that what if the runtime do
something like print to stdout. For now most of the runtime only logs to stderr
so we might be fine. But passing in pipe number might be more robust in this
case
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419685914
##
File path: src/runtime/rpc/minrpc/minrpc_server.h
##
@@ -0,0 +1,598 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file minrpc_server.h
+ * \brief Minimum RPC server implementation,
+ *redirects all the calls to C runtime API.
+ *
+ * \note This file do not depend on c++ std or c std,
+ * and only depends on TVM's C runtime API.
+ */
+#ifndef TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+#define TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+
+#include
+#include
+#include "../rpc_protocol.h"
+#include "../../../support/arena.h"
+
+/*! \brief Whether or not to enable glog style DLOG */
+#ifndef TVM_MINRPC_ENABLE_LOGGING
+#define TVM_MINRPC_ENABLE_LOGGING 0
+#endif
+
+#ifndef MINRPC_CHECK
+#define MINRPC_CHECK(cond) \
+ if (!(cond)) this->ThrowError(RPCServerStatus::kCheckError);
+#endif
+
+#if TVM_MINRPC_ENABLE_LOGGING
+#include
+#endif
+
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief A minimum RPC server that only depends on the tvm C runtime..
+ *
+ * All the dependencies are provided by the io arguments.
+ *
+ * \tparam TIOHandler IO provider to provide io handling.
+ * An IOHandler needs to provide the following functions:
+ * - PosixWrite, PosixRead, Close: posix style, read, write, close API.
+ * - Exit: exit with status code.
+ */
+template
+class MinRPCServer {
+ public:
+ /*!
+ * \brief Constructor.
+ * \param io The IO handler.
+ */
+ explicit MinRPCServer(TIOHandler io)
+ : io_(io), arena_(PageAllocator(io)) {}
+
+ /*! \brief Run the server loop until shutdown signal is received. */
+ void ServerLoop() {
+RPCCode code;
+uint64_t packet_len;
+
+while (true) {
+ arena_.RecycleAll();
+ allow_clean_shutdown_ = true;
+
+ this->Read(_len);
+ if (packet_len == 0) continue;
+ this->Read();
+
+ allow_clean_shutdown_ = false;
+
+ if (code >= RPCCode::kSyscallCodeStart) {
+this->HandleSyscallFunc(code);
+ } else {
+switch (code) {
+ case RPCCode::kCallFunc: {
+HandleNormalCallFunc();
+break;
+ }
+ case RPCCode::kInitServer: {
+HandleInitServer();
+break;
+ }
+ case RPCCode::kCopyFromRemote: {
+HandleCopyFromRemote();
+break;
+ }
+ case RPCCode::kCopyToRemote: {
+HandleCopyToRemote();
+break;
+ }
+ case RPCCode::kShutdown: {
+this->Shutdown();
+return;
+ }
+ default: {
+this->ThrowError(RPCServerStatus::kUnknownRPCCode);
+break;
+ }
+}
+ }
+}
+ }
+
+ void Shutdown() {
+arena_.FreeAll();
+io_.Close();
+ }
+
+ void HandleNormalCallFunc() {
+uint64_t call_handle;
+TVMValue* values;
+int* tcodes;
+int num_args;
+TVMValue ret_value[3];
+int ret_tcode[3];
+
+this->Read(_handle);
+RecvPackedSeq(, , _args);
+
+int call_ecode = TVMFuncCall(
+reinterpret_cast(call_handle),
+values, tcodes, num_args,
+&(ret_value[1]), &(ret_tcode[1]));
+
+if (call_ecode == 0) {
+ // Return value encoding as in LocalSession
+ int rv_tcode = ret_tcode[1];
+ ret_tcode[0] = kDLInt;
+ ret_value[0].v_int64 = rv_tcode;
+ if (rv_tcode == kTVMNDArrayHandle) {
+ret_tcode[1] = kTVMDLTensorHandle;
+ret_value[2].v_handle = ret_value[1].v_handle;
+ret_tcode[2] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 3);
+ } else if (rv_tcode == kTVMPackedFuncHandle ||
+ rv_tcode == kTVMModuleHandle) {
+ret_tcode[1] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ } else {
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ }
+} else {
+ this->ReturnLastTVMError();
+}
+ }
+
+ void HandleCopyFromRemote() {
+uint64_t handle, offset, num_bytes;
+TVMContext ctx;
+
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419685186
##
File path: src/runtime/rpc/minrpc/minrpc_server.h
##
@@ -0,0 +1,598 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file minrpc_server.h
+ * \brief Minimum RPC server implementation,
+ *redirects all the calls to C runtime API.
+ *
+ * \note This file do not depend on c++ std or c std,
+ * and only depends on TVM's C runtime API.
+ */
+#ifndef TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+#define TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+
+#include
+#include
+#include "../rpc_protocol.h"
+#include "../../../support/arena.h"
+
+/*! \brief Whether or not to enable glog style DLOG */
+#ifndef TVM_MINRPC_ENABLE_LOGGING
+#define TVM_MINRPC_ENABLE_LOGGING 0
+#endif
+
+#ifndef MINRPC_CHECK
+#define MINRPC_CHECK(cond) \
+ if (!(cond)) this->ThrowError(RPCServerStatus::kCheckError);
+#endif
+
+#if TVM_MINRPC_ENABLE_LOGGING
+#include
+#endif
+
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief A minimum RPC server that only depends on the tvm C runtime..
+ *
+ * All the dependencies are provided by the io arguments.
+ *
+ * \tparam TIOHandler IO provider to provide io handling.
+ * An IOHandler needs to provide the following functions:
+ * - PosixWrite, PosixRead, Close: posix style, read, write, close API.
+ * - Exit: exit with status code.
+ */
+template
+class MinRPCServer {
+ public:
+ /*!
+ * \brief Constructor.
+ * \param io The IO handler.
+ */
+ explicit MinRPCServer(TIOHandler io)
+ : io_(io), arena_(PageAllocator(io)) {}
+
+ /*! \brief Run the server loop until shutdown signal is received. */
+ void ServerLoop() {
+RPCCode code;
+uint64_t packet_len;
+
+while (true) {
+ arena_.RecycleAll();
+ allow_clean_shutdown_ = true;
+
+ this->Read(_len);
+ if (packet_len == 0) continue;
Review comment:
right now there is no maximum len limit
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419684714
##
File path: src/runtime/rpc/minrpc/minrpc_server.h
##
@@ -0,0 +1,598 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file minrpc_server.h
+ * \brief Minimum RPC server implementation,
+ *redirects all the calls to C runtime API.
+ *
+ * \note This file do not depend on c++ std or c std,
+ * and only depends on TVM's C runtime API.
+ */
+#ifndef TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+#define TVM_RUNTIME_RPC_MINRPC_MINRPC_SERVER_H_
+
+#include
+#include
+#include "../rpc_protocol.h"
+#include "../../../support/arena.h"
+
+/*! \brief Whether or not to enable glog style DLOG */
+#ifndef TVM_MINRPC_ENABLE_LOGGING
+#define TVM_MINRPC_ENABLE_LOGGING 0
+#endif
+
+#ifndef MINRPC_CHECK
+#define MINRPC_CHECK(cond) \
+ if (!(cond)) this->ThrowError(RPCServerStatus::kCheckError);
+#endif
+
+#if TVM_MINRPC_ENABLE_LOGGING
+#include
+#endif
+
+
+namespace tvm {
+namespace runtime {
+
+/*!
+ * \brief A minimum RPC server that only depends on the tvm C runtime..
+ *
+ * All the dependencies are provided by the io arguments.
+ *
+ * \tparam TIOHandler IO provider to provide io handling.
+ * An IOHandler needs to provide the following functions:
+ * - PosixWrite, PosixRead, Close: posix style, read, write, close API.
+ * - Exit: exit with status code.
+ */
+template
+class MinRPCServer {
+ public:
+ /*!
+ * \brief Constructor.
+ * \param io The IO handler.
+ */
+ explicit MinRPCServer(TIOHandler io)
+ : io_(io), arena_(PageAllocator(io)) {}
+
+ /*! \brief Run the server loop until shutdown signal is received. */
+ void ServerLoop() {
+RPCCode code;
+uint64_t packet_len;
+
+while (true) {
+ arena_.RecycleAll();
+ allow_clean_shutdown_ = true;
+
+ this->Read(_len);
+ if (packet_len == 0) continue;
+ this->Read();
+
+ allow_clean_shutdown_ = false;
+
+ if (code >= RPCCode::kSyscallCodeStart) {
+this->HandleSyscallFunc(code);
+ } else {
+switch (code) {
+ case RPCCode::kCallFunc: {
+HandleNormalCallFunc();
+break;
+ }
+ case RPCCode::kInitServer: {
+HandleInitServer();
+break;
+ }
+ case RPCCode::kCopyFromRemote: {
+HandleCopyFromRemote();
+break;
+ }
+ case RPCCode::kCopyToRemote: {
+HandleCopyToRemote();
+break;
+ }
+ case RPCCode::kShutdown: {
+this->Shutdown();
+return;
+ }
+ default: {
+this->ThrowError(RPCServerStatus::kUnknownRPCCode);
+break;
+ }
+}
+ }
+}
+ }
+
+ void Shutdown() {
+arena_.FreeAll();
+io_.Close();
+ }
+
+ void HandleNormalCallFunc() {
+uint64_t call_handle;
+TVMValue* values;
+int* tcodes;
+int num_args;
+TVMValue ret_value[3];
+int ret_tcode[3];
+
+this->Read(_handle);
+RecvPackedSeq(, , _args);
+
+int call_ecode = TVMFuncCall(
+reinterpret_cast(call_handle),
+values, tcodes, num_args,
+&(ret_value[1]), &(ret_tcode[1]));
+
+if (call_ecode == 0) {
+ // Return value encoding as in LocalSession
+ int rv_tcode = ret_tcode[1];
+ ret_tcode[0] = kDLInt;
+ ret_value[0].v_int64 = rv_tcode;
+ if (rv_tcode == kTVMNDArrayHandle) {
+ret_tcode[1] = kTVMDLTensorHandle;
+ret_value[2].v_handle = ret_value[1].v_handle;
+ret_tcode[2] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 3);
+ } else if (rv_tcode == kTVMPackedFuncHandle ||
+ rv_tcode == kTVMModuleHandle) {
+ret_tcode[1] = kTVMOpaqueHandle;
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ } else {
+this->ReturnPackedSeq(ret_value, ret_tcode, 2);
+ }
+} else {
+ this->ReturnLastTVMError();
+}
+ }
+
+ void HandleCopyFromRemote() {
+uint64_t handle, offset, num_bytes;
+TVMContext ctx;
+
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419684443 ## File path: python/tvm/rpc/minrpc.py ## @@ -0,0 +1,79 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Utils to path.""" +import os +from tvm._ffi import libinfo +from tvm.contrib import cc + + +def find_minrpc_server_libpath(server="posix_popen_server"): +"""Get the path of minrpc server libary. + +Parameters +-- +server : str +The kind of built in minrpc server. + +Returns +--- +path : str +The path to the min server library. +""" +curr_dir = os.path.dirname(os.path.realpath(os.path.expanduser(__file__))) +source_dir = os.path.abspath(os.path.join(curr_dir, "..", "..", "..")) Review comment: This is temporary hack. Ideally we should ship min_server to a location (e.g. build) that get installed when we do `setup.py install` instead of relying on the availablity of the source. Or we just ship the source with the installation. When we figure that out(possibly in micro tvm link), we can have a consolidated library as in the libinfo.find_library() 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419681078
##
File path: python/tvm/rpc/client.py
##
@@ -183,28 +198,37 @@ class LocalSession(RPCSession):
need to be ran both locally and remotely.
"""
def __init__(self):
-# pylint: disable=super-init-not-called
-self.context = nd.context
-self.get_function = tvm._ffi.get_global_func
-self._temp = util.tempdir()
+self._temp = server._server_env([])
+RPCSession.__init__(self, _ffi_api.LocalSession())
-def upload(self, data, target=None):
-if isinstance(data, bytearray):
-if not target:
-raise ValueError("target must present when file is a
bytearray")
-blob = data
-else:
-blob = bytearray(open(data, "rb").read())
-if not target:
-target = os.path.basename(data)
-with open(self._temp.relpath(target), "wb") as f:
-f.write(blob)
-def download(self, path):
-return bytearray(open(self._temp.relpath(path), "rb").read())
+@tvm._ffi.register_func("rpc.PopenSession")
+def _popen_session(binary):
+temp = util.tempdir()
-def load_module(self, path):
-return _load_module(self._temp.relpath(path))
+if isinstance(binary, (bytes, bytearray)):
+path_exec = temp.relpath("server.minrpc")
+with open(path_exec, "wb") as outfile:
+outfile.write(binary)
+os.chmod(path_exec, stat.S_IXUSR)
+path_exec = os.path.abspath(path_exec)
+else:
+path_exec = os.path.abspath(binary)
Review comment:
good pt about existence check. abspath makes the execution more
consistent, e.g. if we need to chpwd in the subsequent functions
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419676108 ## File path: python/tvm/contrib/cc.py ## @@ -90,7 +90,8 @@ def get_target_triple(): def cross_compiler(compile_func, options=None, output_format=None, - get_target_triple=None): + get_target_triple=None, + add_files=None): Review comment: I think it is mainly a difference in API choices. - A0 pushing more flags to export_library - A1 using functional style programming decorator, to decorate a existing compiler by adding ldflags implicitly. Pass in fcompile variants, or use fcompile->fcompile functors to decorate new compilers. My take is that A1 is as powerful as A0, and the complexity of compiler customization goes to the construction process of fcompile. It also have the advantage of moving customization to a single place (fcompile) so that we don't have to worry about customizing the export_library step in autotvm and downstream toolchains that uses export_library. We can certainly de-couple the APIs in A1. Right now compilation and linking are coupled, and a more powerful way to construct A1 might be as follows, and we can work toward that direction. ```python fcompile = attach_linker_opts(get_compiler()) ``` 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419676108 ## File path: python/tvm/contrib/cc.py ## @@ -90,7 +90,8 @@ def get_target_triple(): def cross_compiler(compile_func, options=None, output_format=None, - get_target_triple=None): + get_target_triple=None, + add_files=None): Review comment: I think it is mainly a difference in API choices. - A0 pushing more flags to export_library - A1 using functional style programming decorator, to decorate a existing compiler by adding ldflags implicitly. Pass in fcompile variants, or use fcompile->fcompile functors to decorate new compilers. My take is that A1 is as powerful as A0, and the complexity of compiler customization goes to the construction process of fcompile. It also have the advantage of moving customization to a single point (fcompile) so that we don't have to worry about customizing the export_library step in autotvm and downstream toolchains that uses export_library. We can certainly de-couple the APIs in A1. Right now compilation and linking are coupled, and a more powerful way to construct A1 might be as follows, and we can work toward that direction. ```python fcompile = attach_linker_opts(get_compiler()) ``` 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419676108 ## File path: python/tvm/contrib/cc.py ## @@ -90,7 +90,8 @@ def get_target_triple(): def cross_compiler(compile_func, options=None, output_format=None, - get_target_triple=None): + get_target_triple=None, + add_files=None): Review comment: I think it is mainly a difference in API: - A0 pushing more flags to export_library - A1 using functional style programming decorator, to decorate a existing compiler by adding ldflags implicitly. Pass in fcompile variants, or use fcompile->fcompile functors to decorate new compilers. My take is that A1 is as powerful as A0, and the complexity of compiler customization goes to the construction process of fcompile We can certainly de-couple the APIs in A1. Right now compilation and linking are coupled, and a more powerful way to construct A1 might be as follows, and we can work toward that direction. ```python fcompile = attach_linker_opts(get_compiler()) ``` 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419676108 ## File path: python/tvm/contrib/cc.py ## @@ -90,7 +90,8 @@ def get_target_triple(): def cross_compiler(compile_func, options=None, output_format=None, - get_target_triple=None): + get_target_triple=None, + add_files=None): Review comment: I think it is mainly a difference in API: - A0 pushing more flags to export_library - A1 using functional style programming decorator, to decorate a existing compiler by adding ldflags implicitly. Pass in fcompile variants, or use fcompile->fcompile functors to decorate new compilers. My take is that A1 is as powerful as A0, and the complexity of compiler customization goes to the decoration process. We can certainly de-couple the APIs in A1. Right now compilation and linking are coupled, and a more powerful way to construct A1 might be as follows, and we can work toward that direction. ```python fcompile = attach_linker_opts(get_compiler()) ``` 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419671449
##
File path: jvm/core/src/main/java/org/apache/tvm/contrib/GraphRuntime.java
##
@@ -38,53 +38,14 @@
* @return Runtime graph module that can be used to execute the graph.
*/
public static GraphModule create(String graphJson, Module libmod, TVMContext
ctx) {
-Module graphModule = null;
-if (ctx.deviceType >= RPC.RPC_SESS_MASK) {
- if (!(ctx instanceof TVMRemoteContext)) {
-throw new IllegalArgumentException(
-"Looks like you are using remote context with no RPCSession bind."
-+ "Use session.context instead.");
- }
- RPCSession rpcSession = ((TVMRemoteContext) ctx).rpcSession;
- // check arguments
- if (!"rpc".equals(libmod.typeKey())) {
-throw new IllegalArgumentException("libmod.typeKey != rpc");
- }
- final int sessIndex = (int) ((Function) reflectionStaticCall(
- RPC.class, "getApi", "_SessTableIndex"))
- .pushArg(libmod).invoke().asLong();
- if (sessIndex != (Integer) reflectionGetField(rpcSession, "tblIndex")) {
-throw new IllegalArgumentException(String.format(
-"libmod SessTableIndex=%d mismatch rpcSession.tblIndex=%d",
-sessIndex, reflectionGetField(rpcSession, "tblIndex")));
- }
-
- Function rpcModuleHandle = (Function) reflectionStaticCall(
- RPC.class, "getApi","_ModuleHandle");
- if (rpcModuleHandle == null) {
-throw new RuntimeException("Cannot find global function
tvm.rpc._ModuleHandle."
-+ "Did you compile tvm_runtime with the correct version?");
- }
-
- Function fcreate =
Function.getFunction("tvm.graph_runtime.remote_create");
- if (fcreate == null) {
-throw new RuntimeException("Cannot find global function
tvm.graph_runtime.remote_create."
-+ "Did you compile tvm_runtime with correct version?");
- }
-
- TVMValue hmod = rpcModuleHandle.pushArg(libmod).invoke();
- graphModule = fcreate.call(graphJson, hmod,
- ctx.deviceType % RPC.RPC_SESS_MASK, ctx.deviceId).asModule();
-} else {
- Function fcreate = Function.getFunction("tvm.graph_runtime.create");
- if (fcreate == null) {
-throw new RuntimeException("Cannot find global function
tvm.graph_runtime.create."
-+ "Did you compile tvm_runtime with correct version?");
- }
- graphModule = fcreate.pushArg(graphJson)
- .pushArg(libmod).pushArg(ctx.deviceType).pushArg(ctx.deviceId)
- .invoke().asModule();
+Function fcreate = Function.getFunction("tvm.graph_runtime.create");
+if (fcreate == null) {
Review comment:
give it is in the java runtime and not in the scope of this PR, will
leave as it is. There is also a debatable tradeoff (common usage vs less
abstraction)
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419626688 ## File path: python/tvm/contrib/cc.py ## @@ -90,7 +90,8 @@ def get_target_triple(): def cross_compiler(compile_func, options=None, output_format=None, - get_target_triple=None): + get_target_triple=None, + add_files=None): Review comment: I am not very attached to the argument though. Pehaps changig to something like `default_libs` or `add_libs` would alleviate the concern? 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419626178
##
File path: python/tvm/contrib/cc.py
##
@@ -90,7 +90,8 @@ def get_target_triple():
def cross_compiler(compile_func,
options=None,
output_format=None,
- get_target_triple=None):
+ get_target_triple=None,
+ add_files=None):
Review comment:
Indeed it is an API tradeoff. The main advantage of functor decorator
style is that it is composable.
For example, we could pass use the following to attach minserver to the ndk
compiler.
```c++
mod.export_library("xyz", tvm.rpc.with_minrpc(cc.ndk))
```
Another reason is that in the case of minrpc, we need to attach a special
property(need_system_lib) to tell the export_library to check for the
`--system-lib` option.
We might need a bit more thoughts to make the alternative API(as follows)
work.
```c++
mod.export_library("xyz", cc.ndk, libs=[tvm.rpc.minrpc()])
```
Finally, such functor decorator is easier to work with in the AutoTVM(we
just have to pass the decorated version as build_func, instead of passing an
additional argument)
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484: URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419623838 ## File path: python/tvm/_ffi/base.py ## @@ -48,8 +48,11 @@ def _load_lib(): """Load libary by searching possible path.""" lib_path = libinfo.find_lib_path() lib = ctypes.CDLL(lib_path[0], ctypes.RTLD_GLOBAL) -# DMatrix functions lib.TVMGetLastError.restype = ctypes.c_char_p +# Put the libpath to LD_LIBRARY_PATH +# will be useful for pipe session to find libtvm +os.environ["LD_LIBRARY_PATH"] = "%s:%s" % ( Review comment: Given that it makes sense to make libtvm available to subprocesses, I think we can leave it as it is. While setting rpath for the binary would work if the binary is on the same machine, it may not work after we send it through an RPC, so it might be helpful to keep the env variable. 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: us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r419038742
##
File path: apps/cpp_rpc/rpc_server.cc
##
@@ -217,10 +217,10 @@ class RPCServer {
* \param opts Parsed options for socket
* \param ping_period Timeout for select call waiting
*/
- void AcceptConnection(TrackerClient* tracker,
+ void AcceptConnection(TrackerClient* tracker,
Review comment:
this is because apps are not covered by the linter as oppose the `src/`
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:
us...@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #5484: [REFACTOR][RPC][PROCOTOL-CHANGE] Modularize the RPC infra
tqchen commented on a change in pull request #5484:
URL: https://github.com/apache/incubator-tvm/pull/5484#discussion_r418663309
##
File path: src/runtime/rpc/rpc_channel.cc
##
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file rpc_channel.cc
+ */
+#include
+#include "rpc_channel.h"
+
+namespace tvm {
+namespace runtime {
+
+size_t CallbackChannel::Send(const void* data, size_t size) {
+ TVMByteArray bytes;
+ bytes.data = static_cast(data);
+ bytes.size = size;
+ int64_t n = fsend_(bytes);
+ if (n == -1) {
Review comment:
good point, fsend is a user provided function(since it is a callback),
so we will need to document it in the channel definition itself.
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:
us...@infra.apache.org
