Skip to content

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
@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

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

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.

@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

@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