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

Client library generation #192

Merged
merged 6 commits into from
Feb 18, 2019
Merged

Client library generation #192

merged 6 commits into from
Feb 18, 2019

Conversation

vforvalerio87
Copy link
Contributor

Generate python client libraries to target directory from cli by specifying organization id and service id

Updated grpicio-tools, and pyyaml for Python 3.7 compatibility

Libraries are to be used with snet-python-sdk (PR incoming) to accommodate for import paths in compiled .py grpc files and intercept client calls to add mpe metadata

@astroseger astroseger requested a review from vsbogd January 28, 2019 07:06
@vsbogd vsbogd requested a review from astroseger January 28, 2019 07:12
@astroseger
Copy link
Collaborator

astroseger commented Jan 28, 2019

SDKCommand.generate_client_library almost exactly duplicates MPEChannelCommand.init_channel_from_registry. With only one difference that init_channel_from_registry compile protobuf in predefined directory (~/.snet/mpe_client/<mpe_address>/<channel_id>).

Do we want SDK to be totally independent from snet-cli? Why we cannot use the same directories?

In any case, If we want to add the possibility to use different directories then I propose the following:

I could rewrite MPEChannelCommand and MPEClientCommand command in such a way that you will be able to easily reuse them in SDK. I only need to add the possibility to use arbitrary directory for the channel (not only ~/.snet/mpe_client/<mpe_address>/<channel_id>).

@astroseger
Copy link
Collaborator

I propose to make a list of function which we need for SDK. I will add the possibility to run them with arbitrary channel directory.
Probably you will need to following functions:

  • MPEChannelCommand.init_channel_from_registry
  • MPEChannelCommand.open_init_channel_from_registry
  • MPEClientCommand.call_server_statelessly

@vforvalerio87
Copy link
Contributor Author

@astroseger a base sdk needs to be independent from the CLI codebase because the sdk might not be in python.
The CLI needs to generate client libraries for any language, not just python, and you need to incorporate those client libraries in another program with functions to do what you say above in that specific language.
I'm about to open a PR for an "snet-sdk-python" which takes a directory where you have client libraries generated via CLI, imports the generated clients and creates everything else you need to make the grpc call from the client (the grpc channel and the client-side interceptor which handles the mpe). We will need the same in any other language we support.

However, some functions that we need for the cli are the same as what we need for a base Python sdk: what I would do is publish those utility methods as separate packages that we can take as dependencies in both snet-cli and snet-sdk-python.

@astroseger
Copy link
Collaborator

astroseger commented Jan 28, 2019

@vforvalerio87

However, some functions that we need for the cli are the same as what we need for a base Python sdk: what I would do is publish those utility methods as separate packages that we can take as dependencies in both snet-cli and snet-sdk-python.

It is exactly what I'm talking about. But I propose to not rewrite these function and then publish them as separate function, but directly use functions which already exist.

For example, as I've said, SDKCommand.generate_client_library almost exactly duplicate MPEChannelCommand.init_channel_from_registry. It would be much easier to slightly modify MPEChannelCommand and MPEClientCommand (we only need to add the possibility to use arbitrary directory for the channel) to use them in python-SDK.

Otherwise you will need to re-implement half of the code in MPEChannelCommand and MPEClientCommand (as you did in SDKCommand.generate_client_library)....

@astroseger
Copy link
Collaborator

astroseger commented Jan 29, 2019

@vforvalerio87
So, should I adapt

  • MPEChannelCommand.init_channel_from_registry
  • MPEChannelCommand.open_init_channel_from_registry
  • MPEClientCommand.call_server_statelessly

To have possibility to use them from python-SDK? If we don't do it, we will have two version of the code which do the same thing and we will force to struggle in the future to unify it.... (similar to SDKCommand.generate_client_library)...

@vforvalerio87
Copy link
Contributor Author

The issue is, the sdk will be a completely different package in a different repo. That's why I was suggesting that if we want to share code between sdk and cli, some functions should go in a common package that is shared between the two repos. The generate_client_library is really the only thing that goes in the cli, everything else is external.

@astroseger
Copy link
Collaborator

astroseger commented Jan 29, 2019

I don't see the reason of this decision. Why don't you want to reuse the code of snet-cli? I don't see why you want to reimplement everything.
Why don't you want to map python-SDK function to snet-cli functions???

For example.

  • In python-SDK you will probably have function to initialize the channel which already exists: you simply call MPEChannelCommand.init_channel_from_registry
  • In python-SDK you will probably have function to call a server in stateless manner: MPEClientCommand.call_server_statelessly

Please send me a list of function which you need. I will say which functions (and how) you should call...

Why do you want to re-implement these functions???

@vforvalerio87
Copy link
Contributor Author

I'm not saying I want to reimplement everything, I'm saying that a user of the platform should not need to incorporate the whole CLI inside his software just to use a few functions

@astroseger
Copy link
Collaborator

and what about incorporating web3.py? web3.py is much bigger than snet-cli.

@vsbogd
Copy link
Member

vsbogd commented Jan 30, 2019

I agree with @vforvalerio87 point that package hierarchy:

  snet-client-common
  ^               ^
  |               |
snet-cli   snet-client-sdk 

or

snet-client-common
    ^
    |
snet-client-sdk
    ^
    |
snet-cli

is more reasonable than

snet-cli
    ^
    |
snet-client-sdk

And it doesn't necessary mean re-implementing all from the scratch, it means moving code into separate package/repo.

@astroseger
Copy link
Collaborator

I will make PR on @vforvalerio87 branch with proposition how to avoid code duplication (it will do exactly the same :) )...

@astroseger
Copy link
Collaborator

vforvalerio87#3

@astroseger
Copy link
Collaborator

There is a potential problem with different Ethereum networks or with different Registry/MultiPartyEscrow on the same networks (even some vague possibilities to attack).

Maybe we could add "<registry_address>_<mpe_address>" in the path...

<library_language>/<registry_address>_<mpe_address>/<org_id>/<service_id>

@astroseger
Copy link
Collaborator

by the way. As part of #201 I will change directories inside snet-cli in the similar manner. So I will keep channels separately for each service in directories:

~/.snet/mpe_client/<registry_address>_<mpe_address>/<org_id>/<service_id>

So SDK and snet-cli will have the similar structure of directories..

@vforvalerio87
Copy link
Contributor Author

vforvalerio87 commented Feb 14, 2019

Merged in your PR, added a small change (generating client library relative to current working directory instead of relative to cli installation path)

Please take a look at singnet/snet-sdk-python#5 which is the SDK, essentially.

I'm also adding @raamb and @ferrouswheel to the discussion for feedback.

Presently, I would merge this in and merge the snet-sdk-python PR in; then we should open a separate issue on what to do with the sdk/cli code duplication issue, which comes down to three possible solutions in my opinion:

  1. Make snet-sdk a dependency of the CLI, make required changes to both
  2. Make a common shared package used by both
  3. Don't do anything for now, as the shared code parts are a few lines of code and are all not very likely to change. A soon as we need to change anything that is common to both codebases, we do the refactor

@astroseger
Copy link
Collaborator

astroseger commented Feb 14, 2019

@vforvalerio87 There is a potential problem that you keep compiled files in <library_language>/<org_id>/<service_id>.

What will happen if you have different service with the same name in different networks or simply in different versions of Registry?

I insist that you need to store compiled files in:
<library_language>/<registry_address>_<mpe_address>/<org_id>/<service_id>

@vforvalerio87
Copy link
Contributor Author

@vforvalerio87 There is a potential problem that you keep compiled files in <library_language>/<org_id>/<service_id>.

What will happen if you have different service with the same name in different networks or simply in different versions of Registry?

I insist that you need to store compiled files in:
<library_language>/<registry_address>_<mpe_address>/<org_id>/<service_id>

Done!

Copy link
Collaborator

@astroseger astroseger left a comment

Choose a reason for hiding this comment

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

you've added only registry address to the path, which is totally fine if you don't store any channels related information in these paths.....

@vforvalerio87
Copy link
Contributor Author

you've added only registry address to the path, which is totally fine if you don't store any channels related information in these paths.....

We don't, these are just the compiled client libraries which the user should move/copy over to where his client application is

@astroseger
Copy link
Collaborator

@vforvalerio87 should I merge your PR? (but It seems you are also could do it...)

@vforvalerio87
Copy link
Contributor Author

I can but didn't want to force anything... looks good to merge to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants