singnet / snet-sdk-python

Make gRPC calls to SingularityNET services programmatically using Python
MIT License
1 stars 2 forks source link

Import client libraries, allow to make grpc calls to services #5

Closed vforvalerio87 closed 5 years ago

vforvalerio87 commented 5 years ago

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 commented 5 years ago

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

astroseger commented 5 years ago

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

tiero commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

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 https://github.com/singnet/snet-cli/pull/210 to run this example, I will probably merge it tomorrow)

As you can see user specify only org_id and service_id.

ferrouswheel commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

(@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

About the "making things nicer for the user" part, keep in mind that the python sdk has to be consistent with the sdk for every other language, so every abstraction we develop for the python sdk has to be developed the same and documented for every other language. The main goal is to make people be able to make grpc calls to the services by supplying automatically for them the correct metadata that the daemon expects. We want to use the constructs that grpc provides so that we support all the features that grpc offer and people can leverage all those to build their clients (so stuff like streaming, interceptors, reflection, context, etc...) We are not trying to make a framework to abstract the grpc layer, we are trying to abstract the daemon authorization layer. This way, we can also tell people how to instance the sdk and client and then just point them to the grpc documentation and leverage that. The reason why people supply org_id and service_id when they create a client instance is that in you program you might want to instance multiple clients for multiple services, possibly with different configurations. Example:

from snet_sdk import Snet

import config

snet = Snet(private_key=config.private_key)

calculator = snet.client("snet", "example-service")
translation = snet.client("snet", "translation")

calc_stub = calculator.grpc.example_service_pb2_grpc.CalculatorStub(calculator.grpc_channel)
calc_request = calculator.grpc.example_service_pb2.Numbers(a=20, b=30)

transl_stub = translation.grpc.translate_pb2_grpc.TranslationStub(translation.grpc_channel)
transl_request = translation.grpc.translate_pb2.Request(text="The result is.", source_language="en", target_language="de")

print(transl_stub.translate(transl_request).translation + " " + str(calc_stub.add(calc_request).value))
raamb commented 5 years ago

Reading thru the comments I see many useful suggestions and a few points for broader discussion (around consistent grpc support etc). The design aspect raised by @ferrouswheel on a consistent experience for services onboarded to the snet platform and for services yet to be onboarded has a lot of value and would be good to get into the beta. @vforvalerio87 can we get this in quickly? @astroseger @ferrouswheel please correct my understanding as expressed above, would like to have the first version merged in soon.

astroseger commented 5 years ago

@raamb @vforvalerio87 I have several comments.

  1. https is not supported. So I was not able to reproduce your example-service example, because example service is https service now.
  2. Only private-key identity is supported. Maybe we could directly use identity.py from snet-cli in order to support all identity types? At least we should add mnemonics identity, because we widely use it.
  3. _get_funded_channel is incorrect as I've described in slack (see MPEClientCommand._get_channel_state_statelessly in snet-cli and https://github.com/singnet/dev-portal/blob/master/docs/all/mpe/mpe-stateless-client.md)
  4. SDK will work incorrectly for channels in which signer != sender. At least you need to filter our channels which have signer != sender in _get_channels.
  5. Each time you run application you search for opened channels (_get_channels). In snet-cli we actually store "initialized" channels, so we don't need to filter events at each run... But maybe it is not so big problem, because event filtering is rather fast...

And the most important issue with the current implementation: You silently move funds. You silently create new channel and silently fund it with AGI tokens.... I would propose to not do it. All those operations should be done by special functions without any silent execution. Because in any case developer will need those functions to be expose to the user, or user could use snet-cli to move money.
So we should have the following function:

In normal situation user will not need two first function, because he could use snet-cli to move funds.

astroseger commented 5 years ago

@raamb @vforvalerio87 Sorry, I forgot one more issue

  1. You assume that we have only one price model (fixed_price) which will be change soon. I would propose to abstract it away right now. (and fail if pricing model is different from fixed_price)
ferrouswheel commented 5 years ago

I have to disagree with what you consider the most important issue @astroseger - I don't want to mess around with channels at all!

The SDK should automatically do this and use whatever balance is in my escrow account. For any reasonable size application, relying on 10s or 100s of channels to different services, it is not manageable to expect developers to manually set up channels.

I do think that any AGI transfer activities should be logged for auditing purposes, and there needs to be tooling to inspect what channels have been created, and manage them, but this latter activity is what the CLI is for.

astroseger commented 5 years ago

@ferrouswheel I see you point. But the problem is that AGI tokens have real value in it. So it is real money and we must deal with them as they are real money. Otherwise it will lose it's real value.

we have two possible attacks from the service provider

As result. I agree that we can have an option (only an option!) to move funds silently. And we must have build in protection against service attacks. But what we have in current version is no good for my opinion...

ferrouswheel commented 5 years ago

@astroseger I see what you mean...

Perhaps having some threshold the caller can set, saying "I agree to pay up to X amount automatically, with a maximum threshold of Y".

Eventually having the SDK use a manifest of services, with limits on how much prices/timeouts can change without user interaction, would be ideal.

astroseger commented 5 years ago

I would propose the following: We should have three functions in SDK:

  1. Function to reserve funds for a given service (org_id/service_id + group_id in case of several payment groups) for a given time. Which will automatically open new channel if needed
  2. Function to make a call without any funds movement
  3. Function to make a call with automatically reservation of funds. This function takes metadataURI as an argument! So it will work unless service provider change metadata. So application will be able to ask the confirmation from the user that he agrees to automatically work with the service with current price and other conditions. Application will remember this agreement alongside with metadataURI. We even can add special support in SDK for it (keep list of services for which we have an agreement from the user). It will prevent any attacks from the service provider.

3 can be an option for 2.

@raamb @vforvalerio87 @ferrouswheel I propose to do the following.

I think it will be enough for version 0.0.1

vforvalerio87 commented 5 years ago

Hey, sorry I just got back to the issue. So, I'm about to push a fix for 1 (https), 2 (add mnemonic for now), 3 (fix funded channels), 4 (signer != sender). For 5 see the issue about having a pluggable caching mechanism (so I might have a client that uses the same private keys for multiple applications which share logs/channel state).

About the other issue (dealing with channels), the idea I came up with @raamb was the following:

This sounds like the most sensible option to me: SDK can spend AGI you already have explicitly allocated to a given service provider but not do any blockchain transactions transparently for you unless you explicitly allow it to. So the standard SDK configuration would be like, import, create an instance, pass private key or mnemonic + index and "allow_transactions=True" to basically confirm that you allow the SDK to do transactions using your account on your behalf.

vforvalerio87 commented 5 years ago

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 commented 5 years ago

New changes:

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

raamb commented 5 years ago

@tiero @astroseger @ferrouswheel please review

vforvalerio87 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/singnet/snet-daemon/issues/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 commented 5 years ago

I'm sorry I forgot two issues (before I make my designe propostion):

  1. 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
  2. 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 commented 5 years ago

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 commented 5 years ago

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.

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

astroseger commented 5 years ago

@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 commented 5 years ago

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:

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 /. 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:

(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 commented 5 years ago

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 commented 5 years ago

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:

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

raamb commented 5 years ago

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