Skip to content
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

[RUTNIME] Support C++ RPC #4281

Merged
merged 13 commits into from
Nov 10, 2019
Merged

[RUTNIME] Support C++ RPC #4281

merged 13 commits into from
Nov 10, 2019

Conversation

FrozenGene
Copy link
Member

@FrozenGene FrozenGene commented Nov 8, 2019

C++ RPC is very useful for embedded devices, which can not have Python environment easily. And thanks @siju-samuel ' great work, based on #1756, we could get C++ RPC work on these embedded devices. I want to summary some different places compared that PR.

  • The users could only use libtvm_runtime, doesn't need to link libtvm. This requirement is the same as deployment.
  • Previous PR could not work well and process can not exist normally. We write one method named as waitpid_eintr to handle it, we also modify the incorrect argument from --timeout to -timeout.
  • Fix the logic of GetCmdOption. Previous PR can not work well like --key=mtk6763, which will recognize the second k.
  • Fix the logic of GetPath. In our python api, we could add one argument to specify where we would like to place the deployed .so

Others small changes are not listed.

The screenshot of work example:

Device:
image

Server:
image

Better to do:
Modify the runtime code and cpp_rpc code to be C++03 compatible.

@tqchen @siju-samuel @snowolfhawk @yzhliu

@tqchen tqchen changed the title Support C++ RPC [RUTNIME] Support C++ RPC Nov 8, 2019
apps/cpp_rpc/rpc_env.h Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_env.h Show resolved Hide resolved
apps/cpp_rpc/rpc_env.h Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_env.h Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_server.cc Outdated Show resolved Hide resolved
src/common/socket.h Outdated Show resolved Hide resolved
src/common/socket.h Outdated Show resolved Hide resolved
src/common/socket.h Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_tracker_client.h Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_env.cc Outdated Show resolved Hide resolved
@tqchen tqchen self-assigned this Nov 8, 2019
@tqchen
Copy link
Member

tqchen commented Nov 8, 2019

@weberlo @srkreddy1238 @snowolfhawk it would be great if you can also help to review the PR

@FrozenGene
Copy link
Member Author

@tqchen Wish I have solved many comments. I also replace SelectHelper into PoolHelper.

Remained:
Ideally, we should just use std::istringstream to do the validation(to reduce memory complexity). Given that this is a util function, we would like to apply such higher standard
-> Split internally is used istringstream to do things in fact, need to make sure the meaning.

@tqchen
Copy link
Member

tqchen commented Nov 8, 2019

split then check creates additional data structures such as vector and string(additional data structures), which can be avoided as you can simply do istringstream to parse the ints (check the bits) and avoid creation of additional containers

@FrozenGene
Copy link
Member Author

split then check creates additional data structures such as vector and string(additional data structures), which can be avoided as you can simply do istringstream to parse the ints (check the bits) and avoid creation of additional containers

@tqchen When I implement completely, I find using system api inet_pton is a better solution, we could add ipv6 check too.

@tqchen
Copy link
Member

tqchen commented Nov 9, 2019

+1 for ipv6 compatibility

apps/cpp_rpc/rpc_env.cc Outdated Show resolved Hide resolved
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some final nits

apps/cpp_rpc/main.cc Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_server.cc Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_env.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@weberlo weberlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few very minor nits. Otherwise, LGTM.

apps/cpp_rpc/rpc_env.h Outdated Show resolved Hide resolved
src/common/socket.h Outdated Show resolved Hide resolved
apps/cpp_rpc/rpc_server.cc Show resolved Hide resolved
apps/cpp_rpc/rpc_server.cc Outdated Show resolved Hide resolved
src/common/socket.h Outdated Show resolved Hide resolved
src/runtime/rpc/rpc_session.h Outdated Show resolved Hide resolved
src/runtime/rpc/rpc_session.h Outdated Show resolved Hide resolved
@FrozenGene
Copy link
Member Author

@tqchen @weberlo I should resolve all the comments. Please review again.

Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some final nits

apps/cpp_rpc/rpc_env.cc Outdated Show resolved Hide resolved
src/common/util.h Outdated Show resolved Hide resolved
@FrozenGene
Copy link
Member Author

FrozenGene commented Nov 10, 2019

@tqchen Have resolved LOG(ERROR) -> LOG(FATAL) and gnulib issue. Please review again

@tqchen tqchen merged commit d2fc025 into apache:master Nov 10, 2019
@tqchen
Copy link
Member

tqchen commented Nov 10, 2019

Thanks @FrozenGene @weberlo !

@FrozenGene FrozenGene deleted the cpp_rpc branch November 11, 2019 02:36
zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Nov 13, 2019
@tqchen tqchen mentioned this pull request Nov 27, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants