Import client libraries, allow to make grpc calls to services

[vforvalerio87]

Allows importing of client libraries by [org_id, service_id], dynamically loads all compiled modules for a service

Basic methods for programmatic interaction with MPE (deposit, withdraw, open channel, deposit and open, extend, add funds, extend and add funds)

When a client instance is created, it automatically gets the list of service endpoints and the list of open channels and it selects an open and funded channel for the client to use to make grpc calls to the service.

Automatically computes metadata for the next service call and adds the metadata to each service call transparently.

[astroseger]

I would propose to not upload state_service_pb2.py and state_service_pb2_grpc.py and compile them in setup.py...

[astroseger]

Where could I get examples of utilizing this SDK for making call ?

[tiero]

Where could I get examples of utilizing this SDK for making call ?

This works for me

https://github.com/singnet/snet-sdk-python/blob/1b4cce3ab301cb7090d6e9b757307b385b10de26/README.md

[vforvalerio87]

Where could I get examples of utilizing this SDK for making call ?

I made a repo called snet-code-examples which will feature server code and client code examples like the grpc repo (https://github.com/grpc/grpc/tree/master/examples) for various languages.

Anyway, something like what you see in the README or something like this works:

from snet_sdk import Snet
snet = Snet(private_key=<private key>)
translation = snet.client("snet", "translation")
stub = translation.grpc.translate_pb2_grpc.TranslationStub(translation.grpc_channel)
request = translation.grpc.translate_pb2.Request(text="Hello World.", source_language="en", target_language="de")
resp = stub.translate(request).translation
print(resp)

SDK expects to find the compiled libraries in <libraries_base_path (defaults to "grpc")>/org_id/service_id.

You can let the sdk automatically pick a channel or you can specify a channel_id for the client instance (like translation = snet.client("snet", "translation", channel_id=<xxx>))

[astroseger]

@vforvalerio87
In the current version user should specify by hands information which could be acquired automatically.
User should specify:

• the name of protobuf file (translate in your example)
• the name of request class (Request in your example)

Why user should know it?

I would argue that user should specify only org_id/service_id (and optionally channel_id in case of several "initialized " channels and group_name in case of multiply groups)

For example in my simplified example of using snet-cli as a library (which I do not propose to use as SDK, but it is simple demonstration of how it could be done ) the call looks like this:

stub = get_service_stub_for_call_statelessly(org_id = "DappTesOrganization", service_id = "DappTesthttpsService")
request_class = get_request_class(org_id = "DappTesOrganization", service_id = "DappTesthttpsService", method_name = "add")
request = request_class(a = 10, b = 32)
rez = stub.add(request)

see
https://github.com/astroseger/simple_snet_sdk/blob/master/test.py
(you need singnet/snet-cli#210 to run this example, I will probably merge it tomorrow)

As you can see user specify only org_id and service_id.

[ferrouswheel]

@astroseger It's not possible to hide the protobuf details. The message types grpc supports are more complicated than a key-value request. The objects can have either/or logic, and be a nested message of different types.

I think we should do the obvious thing, which is behave like grpc and protobuf. People know those. Maybe one day we can be more clever, but we haven't dealt with streaming data yet, and that will make it harder to know automatically what message type we need use for the streaming component and the order they should be presented.

[ferrouswheel]

However I do agree it would be nice to replace translate_pb2_grpc and translate_pb2 with a common name if possible.

Ideally:

...
translation = snet.client("snet", "translation")
stub = translation.grpc.TranslationStub(translation.grpc_channel)
request = translation.pb2.Request(text="Hello World.", source_language="en", target_language="de")
...

[ferrouswheel]

Although, unfortunately, I realise that isn't easy, because if service spec has multiple proto files they may have the same message name in different files. So trying to squash them all into the same module will cause problems.

I think the convenience methods that @astroseger mentions, or a common namespace (shown in my code example), are a nice idea, but we should still make it easy to use grpc interface as @vforvalerio87 has implemented. I think our sdk should do the correct thing first (work identically to grpc), and convenience can be added afterwards.

[astroseger]

@ferrouswheel Hmm... I do not understand... I simply propose to not repeat information which is already known to the system.

We can get stub_class and request_class by method_name (and protobuf_service_name in case of name conflict when two methods with the same name with different protobuf services). We do it in snet-cli and apparently in dApp. Why we cannot do it SDK?

In @vforvalerio87 example user should know the name of protobuf file and the name of request class.

But for example in snet-cli it looks like this:

snet channel open-init snet translation 0.0001 +10days
snet client call snet translation translate '{"text":"Hello World.", "source_language":"en", "target_language":"de"}'  -y

we don't need the knowledge about the real name of protobuf file, or the name of request or response class.
(the same can be done in my simplified "not quite SDK" which I've demonstrated before)

[astroseger]

(@ferrouswheel And it works perfectly in case of the same request name in different protobuf files, because we fetch stub_class + request_class by method_name (+ service_name in case of conflict))

[ferrouswheel]

The face-services are probably the most complicated use of protobuf in singularitynet. It is much simpler at the moment because I had to remove all the streaming support, but it still has a shared common file across 4 services. I have to hack it because we force all service proto files in the same directory, and snet doesn't allow us to select a single service to be published, so I have shuffle my protofiles into temporary directories when I publish the service spec.

While we can use json objects and convert them magically, in a compiled language it will be annoying. We will want to use the service's protobuf types to ensure the data we are providing to them is correct.

There are also other checks I'm pretty sure we don't do. Can the dapp and snet-cli handle one_of and other protobuf primitives correctly?

I have run into a fair number of problems due to assumptions about grpc and protobuf in our system, which is why I'd prefer us to just do the basic thing first, and then make sure any magic we add doesn't break things.

I'm starting to think we need a benchmark of grpc patterns, to test any assumptions we make when trying to simplify things. When we support streaming it will be affected by a lot of these decisions.

I think there is room to still have the simple/easy version of the sdk you propose @astroseger - I just don't think it is sufficient for the full set of services one can express in grpc.

[ferrouswheel]

Ok, after porting one of my grpc clients to this sdk (or the simple alternative) there is one major design thing I'm concerned about.

It looks like I need different code paths for singularitynet and for calling the service without the singularitynet layer. e.g. if I have a local service, and I'm testing it (without any blockchain stuff), I want to use almost the exact same code as when I'm calling the production service.

Ideally I'd be able to fetch the model from either a local directory (yet to be published), or from snet. Then it'd be compiled and I could create a stub and request, but it'd only add the MPE stuff if I'm actually calling the service via MPE.

Any idea if you've already allowed for this @vforvalerio87 - or if it'd be easy to support something like this?

snet = Snet(private_key=pk)
if snet_enabled:
    client = snet.client("snet", "face-detect")
else:
    client = snet.client(directory="service/service_spec", endpoint="http://localhost:50015")

### snet-sdk-python with full grpc specification
stub = client.grpc.face_detect_pb2_grpc.FaceDetectStub(client.grpc_channel)
request = client.grpc.face_detect_pb2.Request(image=client.grpc.face_detect_pb2.ImageRGB(content=...))
faces = stub.FindFace(request)

### simple-sdk version
stub = client.get_stub()
request_cls = client.get_request(method='FindFace')
request = request_cls(image={content=...}))
rez = stub.FindFace(request)

If a directory and endpoint is provided, then no MPE work is done, nothing is looked up on the registry etc.

Edit: updated to include what @astroseger's simple-sdk would have to look like.

[ferrouswheel]

The above example code has the advantage that I could download another service's spec, and then use it locally while testing. I can implement a mock interface (that just returns dummy responses), and I can then test the logic of my application without a live connection to an ethereum network.

Being able to test individual components or service is an important consideration for any microservice architecture, and SNet is essentially a giant microservice architecture for AI.

Eventually it'd be nice for the SDK to do this automatically, or for it to help the developer to run their application in offline/test mode.

[vforvalerio87]

I pushed the latest changes.

Now users can specify different private keys for ethereum transactions and for the mpe state channel signer. Technically you can also just specify a signer key. Channels are correctly filtered for both sender address and signer address. If user does not provide a dedicated signer private key, signer address defaults to sender address.
https is fixed (I tested it with example service, both the endpoint to get channel state and the service itself work).
For now if no usable channel is available and allow_transactions is True, a new channel will be opened. I'm working towards having it extend and/or fund an existing one if possible.

I'm also working towards fixing a thing regarding using a mnemonic, which goes along another change regarding working with unlocked accounts (ex: ganache-cli).

[vforvalerio87]

New changes:

• If a funded, non-expired channel exists, pick that (as before)
• If no non-expired and funded channels exist, but funded expired channels exist, extend one
• If no funded channels exist, but non-expired channels without funds exist, fund one
• If any channel exists, but they are all expired and with no funds, fund and extend one
• If no channels exist, create one

(provided "allow_transactions" is passed as a configuration parameter)

[raamb]

@tiero @astroseger @ferrouswheel please review

[tiero]

Should we add a requirements.txt so it's easier to uninstall all dependencies from global scope?
pip uninstall -r requirements.txt

I had a clash with another python project. (I do not use virtual env)

[vforvalerio87]

Should we add a requirements.txt so it's easier to uninstall all dependencies from global scope?
pip uninstall -r requirements.txt

I had a clash with another python project. (I do not use virtual env)

We would have to keep dependencies both in setup.py and requirements.txt then. Not ideal.

[tiero]

Should we add a requirements.txt so it's easier to uninstall all dependencies from global scope?
pip uninstall -r requirements.txt
I had a clash with another python project. (I do not use virtual env)

We would have to keep dependencies both in setup.py and requirements.txt then. Not ideal.

Yep, setup.py is the place where we add dependencies and that's fine. But how you manage to uninstall all dependencies in setup.py? pip uninstall want a txt file to be passed.

What we can do is the txt file being a slave of setup.py (pip freeze or python setup.py install --record text.txt)

Not blocking at all, but would be handy to understand how to clean the global dependencies.

[astroseger]

allow_trancsation parameter (False by default) is a good improvement!

Few comments:

1. mnemonics are still unsupported but as I understand support is on its way.
2. I couldn't run sdk in ipython because of some issues with __file__...
3. By default sdk search for compiled protobuf files in grpc/<org_id>/<service_id> but snet sdk generate-client-library generate library in python/<registry_address>/<org_id>/<service_id> which is confusing because I spent some time to understand that I need to make ln -s python/<registry_address> grpc.
4. _get_funded_channel is still incorrect. You overestimate unspent_amount in case then blockchain_nonce != current_nonce. But we need Fully implement "stateless" logic snet-daemon#220 in order to fully solve this problem.
5. You assume that we have only one price model (fixed_price), which is bad. SDK must fail with exception in case then pricing.price_model != fixed_price.
6. There are a lot of functions which are defined inside other function Snet.client: _get_channel_state _get_channel_states and others... Is it a standard pattern? For me it's looks like strange. Why not make the client a proper class or define functions outside other function?
7. In general I would propose to structure code (split into functions) a little bit more. It is difficult to follow so long functions as client. But I am not so good in reading code, so I'm not a good benchmark...

And I have a design proposition how to speedup SDK and simplify unification of SDK and snet-cli in the future (I will make it in the separate post).

[astroseger]

I'm sorry I forgot two issues (before I make my designe propostion):
8. I would propose to not include state_service_pb2_grpc.py and state_service_pb2.py in git (it is like including compiled executions). We need to compile them in setup.py like we do it in snet-cli and snet-daemon
9. SDK will not be able to deal (It will not even detect it) with situation then service provider update protobuf files. But my design proposition will solve this issue :)

[vforvalerio87]

allow_trancsation parameter (False by default) is a good improvement!

Few comments:

1. mnemonics are still unsupported but as I understand support is on its way.
2. I couldn't run sdk in ipython because of some issues with __file__...
3. By default sdk search for compiled protobuf files in grpc/<org_id>/<service_id> but snet sdk generate-client-library generate library in python/<registry_address>/<org_id>/<service_id> which is confusing because I spent some time to understand that I need to make ln -s python/<registry_address> grpc.
4. _get_funded_channel is still incorrect. You overestimate unspent_amount in case then blockchain_nonce != current_nonce. But we need singnet/snet-daemon#220 in order to fully solve this problem.
5. You assume that we have only one price model (fixed_price), which is bad. SDK must fail with exception in case then pricing.price_model != fixed_price.
6. There are a lot of functions which are defined inside other function Snet.client: _get_channel_state _get_channel_states and others... Is it a standard pattern? For me it's looks like strange. Why not make the client a proper class or define functions outside other function?
7. In general I would propose to structure code (split into functions) a little bit more. It is difficult to follow so long functions as client. But I am not so good in reading code, so I'm not a good benchmark...

And I have a design proposition how to speedup SDK and simplify unification of SDK and snet-cli in the future (I will make it in the separate post).

Thanks for the feedback, I'll address each point

1. (mnemonic) Working on it, should be done in the next couple of days (more urgent stuff was requested for today)
2. (ipython file) Same as above
3. I would honestly do just <libraries_base_path>/<org_id>/<service_id>. Reason: this directory in the sdk is not going to store state, or network-specific information, or information about channels. Just the compiled libraries. I assume in most cases people will have the same service deployed across different networks, so you can reuse the same client code and just change the configuration anyway. If the service you are interacting with has different implementations across different networks/registries, you are going to have to adapt your client code anyway, so you won't be able to reuse it fully. So, if we want this to be consistent, I would change the cli so that the default PROTODIR is grpc, remove the language prefix, remove the registry prefix. I wouldn't change this in the sdk because if we "force" the registry as a prefix, most of the times it means you are going to have the same generated code copied multiple times. Standard use case is, you go to your client application directory, run snet sdk generate-client-library python <org_id> <service_id> and you have the client library where the sdk expects it to be for your client application... no need to copy anything anywhere
4. Should I wait for issue #220 to be fixed to work on this?
5. Ok I'll add that in now
6. and 7. Yes, code needs to be refactored. My idea was to have the working code for a functional sdk merged in, then work on refactoring the solution. Client will be a separate class in a separate file and it will take an instance of the Snet class as a constructor parameter. I would to this after this PR is merged in though to be honest

[astroseger]

design proposition.
I propose to unify a way in which SDK and snet-cli deal with protobuf files, service metadata and initialized channels. By the way, I've already borrowed some ideas from @vforvalerio87 (storing compiled protobufs in <org_id>/<service_id>). So I've already made this unification on the snet-cli side by implementing good features from SDK. Now I propose to take good features from snet-cli.

In snet-cli we cache the following information.

• Compiled protobuf
• Service registration (we only need metadataURI here)
• Service metadata
• list of channels which was opened for this service (more or less we cache result of _get_channels)

I underline that we only cache this information. At each call we actually verify that metadataURI hasn't been changed in Registry and if we detect that metadataURI has changed we reinitialize service (download metadata and recompile protobuf file).

This approach has the following advantages

• We don't need to manually compile protobuf (with snet sdk command in case of SDK) or initialize service. We automatically do it then needed.
• We actually know then service provider has changed his service (for example increase price in 1000 times) and we can inform user about it...
• We don't need to filter channels at each run and we don't need to download metadata at each run (as SDK does!). We only need to check that metadataURI hasn't been changed, and we even can skip this check if we can go faster... So it is actually possible to make a call directly without any blockchain calls.
• And we even can implement state-full client, so we don't need to ask daemon the state of the channel each time we make a call (I don't think that we need to do it though...).

[astroseger]

@vforvalerio87
About storing compiled protobuf in grpc/<org_id>/<service_id> or in python/<registry_address>/<org_id>/<service_id>. I would solve this problem radically by following my design proposition.
Why do you store compiled protobuf files (in very rigid statically way, so SDK is not able to detect then protobuf files has changed ) but at each run you call _get_channels (which is quite slow)?
I would argue that it is faster to download protobuf files and recompile them then call _get_channels.

We should dynamically cache everything (compiled protobuf, service registration, service metadata, channels ( result of _get_channels)).

It is not logical to only (statically) cache compiled protobufs. It would be actually more logical to do everything dynamically, so simply recompile protobuf at each run (but I don't think it is a good idea because caching is the solution...)

[vforvalerio87]

About caching, I opened a dedicated issue because I think the SDK has special requirements.

Let me explain: while for the CLI it might be perfect to store everything locally in the file system (example, using pickle), for the SDK I think it would be better to have a pluggable caching mechanism, where you either specify a default method of storing data locally (by passing a string, example: "pickle", or whatever) which is known by the SDK, or you pass a class which exposes predefined methods to serialize/deserialize the data to be cached (example: a class with the "save" and "load" methods).

There are mainly two reasons for this:

• It works in systems where you don't have a file system that you can work with (example: AWS Lambda functions, or Google Cloud Functions, or Azure Cloud Functions)
• It works in systems where you have multiple machines that use the SDK with the same private key and share the state (example: nonces, logs, filtered logs, lists of channels, channel state, etc...) in a common system (example, MySQL, DynamoDB, a network mounted disk share, whatever)

So basically, I would like to keep the SDK completely stateless by default (or more precisely, it shouldn't mandate the way that data is persisted) with the option of caching data however you prefer.

This is mentioned in #7

[astroseger]

@vforvalerio87
For example for example-service on ropsten _get_channels takes 4 seconds, but downloading and recompiling of protobuf files takes only 1.5 seconds. So it is not logical to statically cache compiled protobuf and not caching list of channels..... We should dynamically cache everything.

[astroseger]

@vforvalerio87 I'm talking about dynamical caching. snet-cli will perfectly work in stateless case (ok you need to provide configuration). You can erase all cache at each run and it will perfectly work...
In contrast, SDK will not work if I remove compiled protobuf files....

[vforvalerio87]

Oh ok I see what you mean.
So, dynamically caching: yes, we can do that for everything.

I would still leave the option to provide the statically compiled _pb2 files, but the application would also download/compile them for you if you don't have them for the specified <org_id>/<service_id>. This is just for python and js by the way, not for the sdks for compiled languages of course. The reason why I would keep the option to provide the statically compiled libraries beforehand is:

• consistency with the implementation of other languages
• consistency with standard gRPC development practices. If you look at the code examples repos (https://github.com/grpc/grpc/tree/master/examples/) for dynamic, interpreted languages (example, nodejs) they provide the option for both dynamic and statically compiled client libraries
• (faster boot time, but ok, this is minor)

(Actually by the way with the sdk you can specify a channel_id in the client instance, at which point it won't look for a channel to use again)

[raamb]

Thanks @astroseger @vforvalerio87. I see that there are good suggestions which we can pick up in our subsequent iterations. From a first version perspective I see that we do not have any blockers as such. The discussion above will generate a bunch of issues that we will pick and resolve in the upcoming days. For now I am inclined to merge this.

[astroseger]

Yes, for sure, we could provide option for statically compiled languages. It might be more convenient for someone who writes the client for his own service (but even in this case I would still recommend to use dynamically compiled (and cached) protobuf, because it will automatically update everything in case of protobuf update in Registry).

So again my point is following: I think that a way in which we deal with caching protobuf/metadata/channels in snet-cli can be and should be reused in SDK. And it will go with idea of @ferrouswheel for having everything, for all components in .snet.

I can easily isolate the functions which we use in snet-cli to separate library which will be the first part of the common staff between SDK and snet-cli:
Actually there are two rather small functions:

• _init_or_update_registered_service_if_needed - which detect then service registration has changed (by checking metadataURI) and reinitialize service if something have been changed (download metadata and recompile protobuf )
• _smart_get_initialized_channel_for_service - get appropriate channel for service (from cached channels or from blockchain).

... So I see comment from @raamb . I agree that we could discuss it in separate issue...

[raamb]

Merging this in now and will create individual issues for the points raised