SDK API discussion - Githubissues

vsbogd commented 5 years ago

I have created this issue to discuss the API which should be provided by SDK for client development. Currently implemented approach is to generate client API stubs on the fly when new client instance is created (see Usage section of the README.md).

Example I have posted below has two items which I would like to discuss:

it contains some example of API which can be used to configure SingularityNet service interaction session
it demonstrates client implementation when client stubs are generated in advance, before developing a client code.

import snet
import snet.example_service as example_service

# Define identity, ethereum endpoint, IPFS endpoint, session cache strategy
identity = snet.PrivateKeyIdentity("path/to/private/key") 
eth = snet.KovanEth()
ipfs = snet.SnetIpfs()
cache = snet.FileSystemSessionCache("path/to/cache/folder")
session = snet.Session(identity=identity, eth=eth, ipfs=ipfs, cache=cache)

# Check AGI tokens balance and MultiPartyEscrow contract balance of session
# identity, and deposit tokens to the MultiPartyEscrow.
print("AGI tokens available:", session.balance())
session.deposit(1000)
print("AGI tokens available:", session.balance())

# Instantiate "Example Service" client and call the method.
# example_service.Calculator is generated as prerequisite step, for example
# user calls `snet sdk` command providing org_id and service_id to generate
# snet.example_service package with all necessary stubs. All logic related to
# opening channel, finding proper endpoint, switching between endpoints is
# embedded into service stub.
example_service = example_service.Calculator(session)
numbers = example_service.Numbers(a=7, b=6)
result = example_service.mul(numbers)
print("Calculation result:", result.value)
print("AGI tokens available:", session.balance())

Item 2 is not so important probably for interpreted languages like Python or JavaScript but it can be faster and may be simpler than generating stubs on the fly. BTW it is harder to reuse such stubs in snet-cli so it is a bit contradicts to issue #9.

vsbogd commented 5 years ago

I would like to welcome anyone to share your thoughts on both items.

raamb commented 5 years ago

@vsbogd the session concept sounds quite interesting. Will it abstract out the channel completely? I.e. it will automatically create / fund / extend a channel as well as internally manage channels by service

If I were to extend your example to interact with two different services as shown below, is the session object expected to manage channels separately?

session.deposit(1000)

example_service = example_service.Calculator(session)
numbers = example_service.Numbers(a=7, b=6)
result = example_service.mul(numbers)
print("Calculation result:", result.value)
print("AGI tokens available:", session.balance())

cntk_image_service=cntk_image_service.Recognizer(session)
image = cntk_image_service.Input(img_path="/some_path/some_file.jpg")
result = cntk_image_service.flowers(image)
print("Response is ",result.top_5)
print("AGI tokens available:", session.balance())

astroseger commented 5 years ago

We need an abstraction for configuration. As a basis we could take snet-cli configuration and extend it, or probably we could define new SDK format from scratch. Of course it should be an abstract class, so user can provide config as they want (not necessary via config file).
We need to pass org_id and service_id somehow, or we need to hardcode it somewhere, because we need to find metadata for this service. ( As an alternative we could directly hardcode metadata, but in this case we will not support any updates to metadata.)
I'm not sure that we need the session. There is no real session here. There is only web3 client which might work well when we initialize it multiply times. It should be tested.
@raamb It is very straightforward to dynamically manage channels (create/fund/extend). But we need very very very carefully manage the obvious danger associated with it! We need to have the following parameters in configuration: is_allow_transaction, maximal_channel_expiration, maximal_channel_value, maximal_price_per_call, ...

For example statically compiled example of @vsbogd will look like this: (I will write it without "session", but I'm not 100% sure that it is the best strategy)

import snet
import snet.example_service as example_service

# load configuration which contains: 
# - current ethereum identity
# - current network (with all information: rpc_endpoint, default_gas_price_strategy and optional contract_addresses)
# - ipfs endpoint
# (SDK specific) - caching strategy
# (SDK specific) - "is_allow_transaction" and others..

config = snet.config("/path/to/your/config")

# we need to pass <org_id> <service_id> here, or hardcode it in "snet.example_service"
example_service = example_service.Calculator(config, "<org_id>", "<service_id>")
# the rest is like in @vsbogd example
....

We might also want to support dynamically compiled calls:

import snet
import snet.call_stub_generator as call_stub_generator

config = snet.config("/path/to/your/config")

stub = call_stub_generator("snet", "example-service", "add")
# parameters could be a dictionary, I don't think that we need Request class here
params = {"a":1, "b":2}
rez = stub(params)

ferrouswheel commented 5 years ago

What will people use if they are testing their service and want to use the same code path, but making grpc requests without the extra SNet headers?

This is why I suggested matching the grpc API, unless we are planning to do major API work for every programming language and want to force people to learn our new API when the grpc API already exists.

The alternative is the dynamically compiled calls that Sergey has already implemented. It is perfect for ease of use when people are prototyping an integration with an SNet service. I'm not sure a halfway point between fully dynamic and fully grpc API is useful.

~~I don't understand how this will work:~~

import snet.example_service as example_service

~~Since the session is set up after the import, and the snet package will have no way to know what network, registry or org to use to find example_service.~~

Rereading the code example I see the mention of running a CLI command to generate the stub. This makes sense and will be required for any compiled language anyhow. However, there are then these questions:

how do we check that the generated code is correct for the network/registry being used in the session?
what if there are multiple orgs with the same service name that the sdk user wants to integrate with? (e.g. if they are comparing the accuracy of two similar services provided by different people)
if we have most of the session information when the stub is generated (ipfs, network id), why not encode that in the generated code to avoid mismatches?
How to we translate characters that are not allowed in module names like -?

If we do this anyway, I think we should namespace services from the rest of our sdk snet.service.ORG.SERVICE_NAME. Mixing it in with the root module could result in weird name collisions and possible security issues.

ferrouswheel commented 5 years ago

After thinking about this, I realise the session object could be configured in a way to remove all the SNet stuff and preserve the same code path when people are testing their service outside of SNet. Something like:

session = DirectSession(endpoint="127.0.0.1:8080")

which could then be passed to the stub creator.

The question about how much we should diverge from standard grpc/protobuf API still remains though.

ferrouswheel commented 5 years ago

If we use a Session object, I would prefer us not to go overboard with argument classes. It makes sense for identity, but there is no need to make ipfs and network setup an object, just make them string arguments to the session. If we want to have defaults that are easy to use instead of remembering endpoints, provide those as class or module constants: e.g.

session = Session(identity=..., ipfs=snet.DEFAULT_IPFS, network=snet.KOVAN)
# of course, if these are the default, then it just becomes:
session = Session(identity=...)

Or KovanSession(), MainnetSession() to make it clearer which environment is being used. These kind of extra class objects I'm okay with because it simplifies the API for the user rather than making them have to worry about composition of the configurations.

(Sorry for three comments in a row!)

astroseger commented 5 years ago

@ferrouswheel There are more parameters which should be passed, and not only identity/network/ipfs... Why not make an abstraction for config? Even if you want to stick to session abstraction...

config = snet.config("/path/to/your/config")

# or it might be like this:
# config2 = snet.static_config(identity=, ipfs=...,network, buch_of_other_parameteters=...)

session = session(config=config)

However here a session is actually a state of current configuration. I'm not sure that we need to initialize something in the session (it means that we might not need an abstraction for session, but of course we might keep it... )

ferrouswheel commented 5 years ago

I'm not fussed about session vs config. I'd prefer fewer things to set up, but I don't know enough to say whether a session is necessary...

Sorry, I didn't add all the parameters to try to keep it simple... everything that has a sensible default should use the sensible default. What are the bare minimum arguments required that we can't assume?

As far as I can tell it's network and identity.

Anyhow, I was primarily wanting us to avoid snet.KovanEth() and other objects for things that don't need be an object. Let people use strings unless there is a really really good reason to introduce a new class.

raamb commented 5 years ago

4. @raamb It is very straightforward to dynamically manage channels (create/fund/extend). But we need very very very carefully manage the obvious danger associated with it! We need to have the following parameters in configuration: is_allow_transaction, maximal_channel_expiration, maximal_channel_value, maximal_price_per_call, ...

Exactly, we need to be very careful. Even if we go with the session approach, I would prefer to have channels called out as an explicit entity. The channel concept is very important to call out even if we simplify it.

astroseger commented 5 years ago

After playing with current SDK and trying to merge it with functions from snet-cli I came the the following API:

I propose to have two types of clients:

one which do not send blockchain transactions at all (BasicClient)
one which automatically add funds and extend channel (AutoRefundingClient)

It should be noted that:

It is plain GRPC (exactly like in @vforvalerio87 SDK which was a basis for this code) with some syntactic sugar.
Here we automatically download and compile protobuf files (with dynamical caching for everything). But static location for protobuf files can be easily specified if needed.
I propose to discuss API first and after we will discuss the implementation (I do not insist on this implemented, we can adapt the current SDK without using functions from snet-cli, and then reuse it in snet-cli).
I haven't solve problem with possible attack from the server provider who change the price...

You should get this branch to run the following examples: https://github.com/astroseger/snet-cli/tree/new-sdk (new-sdk branch in my repo!).

Example for AutoFundingClient

from snet_cli.sdk import Session, BasicClient, AutoFundingClient
from snet_cli.config import Config

# we directly use snet-cli configuration
# another types of configuration can be easiliy implemented (for example hard-coded configuration)
session = Session(Config())

# we automatically open channel if needed
# each time we run out of funds we add 10 cogs
# each time we run out of "expiration" we add 20 days
client = AutoFundingClient(session, "snet", "example-service", "add", 10, "+20days")

for i in range(1000):
    request = client.request_class(a=20, b=i)
    rez = client.stub.add(request)
    print(rez)

Example with BasicClient (and two helper functions: reserve_funds and unspent_amount):

from snet_cli.sdk import Session, BasicClient, AutoFundingClient
from snet_cli.config import Config

session = Session(Config())

# we automatically open channel if needed!
# add 10 cogs to snet/example-service escrow
session.reserve_funds("snet", "example-service", 10, "+100days")
client = BasicClient(session, "snet", "example-service", "add")

for i in range(1000):
    unspent_amount = client.unspent_amount()
    if (unspent_amount < 1):
        session.reserve_funds("snet", "example-service", 10, "+100days")

    request = client.request_class(a=20, b=i)
    rez = client.stub.add(request)
    print(rez)

ferrouswheel commented 5 years ago

client = BasicClient(session, "snet", "example-service", "add")

We should not force the client to be dependent on the method called. When people start providing richer APIs (beyond a single request/response method) it will be annoying to create a new client for every method.

It is plain GRPC

Can you include an example of how you would use plain GRPC? Are all the request and response protobuf classes available via the client? What happens if their API names clash with client property names?

astroseger commented 5 years ago

@ferrouswheel It is plain grpc, because:

client.stub is proper grpc stub
client.request_class is proper grpc request_class
(if you need it) client.response_class is a proper grpc response class

Can you include an example of how you would use plain GRPC?

What exactly do you want?

If you want to test your service before registered it then I have the question to your: Do you want to do it with or without a daemon?

If you want to test your service without the daemon (so without any channels and payments), then you can directly use grpc. If you want I can add a class with the same syntactic sugar... Something like this:

# ATTENTION!!! this example will not work, it is not implemented yet
from snet_cli.sdk import TestClient

client = TestClient("/path/to/your/compiled/protobuf/files/", "add")

for i in range(1000):
    request = client.request_class(a=20, b=i)
    rez = client.stub.add(request)

If you want to test your service with a daemon (but without registered it in Registry). Then we have mechanism in snet-cli to initialize service directly from metadata (@raamb preventing me to remove this mechanism recently :) ). So you initialize your service and directly use BasicClient or AutoFundingClient. I want to underline that if your service is Registered in Registry then all initialization is done automatically on fly. Of course without registration we cannot do it on fly.

ferrouswheel commented 5 years ago

There is more complexity to grpc and protobuf messages than just request and response classes, and those will become far more obvious outside of python (in any staticly typed language).

And as I said. I really think making clients dependent on the specific method is a bad idea. If you were creating an http request library, you wouldn't create a new client for every http url!

On Mon, 11 Mar 2019, 9:40 PM Sergey Rodionov, notifications@github.com wrote:

@ferrouswheel https://github.com/ferrouswheel It is plain grpc, because:

client.stub is proper grpc stub

client.request_class is proper grpc request_class

(if you need it) client.response_class is a proper grpc response class

Can you include an example of how you would use plain GRPC?

What exactly do you want?

If you want to test your service before registered it then I have the question to your: Do you want to do it with or without a daemon?

If you want to test your service without the daemon (so without any channels and payments), then you can directly use grpc. If you want I can add a class with the same syntactic sugar... Something like this:

ATTENTION!!! this example will not work, it is not implemented yetfrom snet_cli.sdk import TestClient

client = TestClient("/path/to/your/compiled/protobuf/files/", "add") for i in range(1000): request = client.request_class(a=20, b=i) rez = client.stub.add(request)

If you want to test your service with a daemon (but without registered it in Registry). Then we have mechanism in snet-cli to initialize service directly from metadata (@raamb https://github.com/raamb preventing me to remove this mechanism recently :) ). So you initialize your service and directly use BasicClient or AutoFundingClient. I want to underline that if your service is Registered in Registry then all initialization is done automatically on fly. Of course without registration we cannot do it on fly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/singnet/snet-sdk-python/issues/16#issuecomment-471449970, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHWB1SZoWExFDKSEwPZice1ydf40ekNks5vVhaNgaJpZM4bilJ6 .

chetankothari commented 5 years ago

Extending the idea @astroseger proposed here about multiple clients, with and without auto funding.

Rather than having two different clients we could rather have one client and allow the user change the funding strategy and we could provide a default strategy which the user needs to explicitly connect to the client.

Something along these lines (extending @astroseger's example)

from snet_cli.sdk import Session, Client, DefaultFundingStrategy
from snet_cli.config import Config

# we directly use snet-cli configuration
# another types of configuration can be easiliy implemented (for example hard-coded configuration)
session = Session(Config())

# we automatically open channel if needed
# each time we run out of funds we add 10 cogs
# each time we run out of "expiration" we add 20 days
client = Client(session, DefaultFundingStrategy(), "snet", "example-service", "add", 10, "+20days")

for i in range(1000):
request = client.request_class(a=20, b=i)
rez = client.stub.add(request)
print(rez)

If the user wants to define her own funding strategy we could define a contract which a strategy needs to adhere to.

This then opens up possibilities for a lot of different strategies the user can define without changing the client.

If we think this is a good path to take then we could work towards refining this further.

chetankothari commented 5 years ago

@raamb agree with you on this. This post takes care of this concern. Of course we need to refine the API/contract of the strategy but should be doable.

astroseger commented 5 years ago

@ferrouswheel

I don't think that specifying the name of protobuf file (like in @vforvalerio87 SDK) is a right path. Sure It is possible to do the same, but I think it is not the best we could do...

What about API in which you specify the name of protobuf-service (in case of multiply protobuf services )?

For example: (it is not implemented yet, so this example will not work!)

from snet_cli.sdk import Session, Client, AutoFundingFundingStrategy
from snet_cli.config import Config

# we directly use snet-cli configuration
# another types of configuration can be easiliy implemented (for example hard-coded configuration)
session = Session(Config())

# In case of multiply grpc_service names you should specify the name of grpc_service
# Here we have only one grpc_service (Calculator), so we can omit this parameter
client = Client(session, "snet", "example-service", AutoFundingFundingStrategy(10, "+20days"))

for i in range(1000):

    # unfortunately you must know the name of request_class
    request = client.classes.Numbers(a=20, b=i)
    rez = client.stub.add(request)
    print(rez)
    rez = client.stub.mul(request)
    print(rez)
    rez = client.stub.sub(request)

astroseger commented 5 years ago

@chetankothari
I agree with having one client + different funding strategies. But I don't think that we need separate contract for it... Which funding strategy we cannot handle without new contract?

ferrouswheel commented 5 years ago

@astroseger I think the combination of your TestClient idea and having the protocol buffer classes available in client.classes or client.pb (short for "protocol buffers", since they are message types and using classes is just how Python implements them) is close enough.

However, we'd also need to figure out how to handle multiple proto files, since you could have the same pb message name in different files, so collapsing them all into one namespace is going to cause trouble eventually. e.g. if I had BoundingBox in car.proto and BoundingBox in human.proto, both used as part of a self-driving car service defined in driving.proto, but each BoundingBox has slightly different information, we'd need make sure these are distinct types in client.classes.

This is one of many reasons for trying to make the core SDK as close to pb/grpc as possible which already handles all of these edge cases. One other reason is that we'd need to implement our own code generation for C++/C/Rust SDKs, on top of what pb/grpc provides, if we wanted our SDKs to be similar across languages.

I originally liked your "simple" SDK idea as a separate interface which is specialised to Python and would work for 90% of service develoeprs, but with a core SDK would expose the pb/grpc details for people that needed it.

raamb commented 5 years ago

@chetankothari I agree with having one client + different funding strategies. But I don't think that we need separate contract for it... Which funding strategy we cannot handle without new contract?

@astroseger by contract @chetankothari was referring to the API interface contract and not a blockchain contract.

vsbogd commented 5 years ago

However, we'd also need to figure out how to handle multiple proto files, since you could have the same pb message name in different files, so collapsing them all into one namespace is going to cause trouble eventually. e.g. if I had BoundingBox in car.proto and BoundingBox in human.proto, both used as part of a self-driving car service defined in driving.proto, but each BoundingBox has slightly different information, we'd need make sure these are distinct types in client.classes.

@ferrouswheel , I believe multiple protobuf files problem should be resolvable via protobuf packages. In your example even protobuf compiler should complain when you use the same BoundingBox name to define two different messages and include them in driving.proto. To solve this one can use different package names in car.proto and human.proto and use fully qualified name in driving.proto.

I think we need compile protobuf supporting packages and adding some prefix package as you suggested here. For instance <org_id>.<service_id> package prefix.

For example service it would be ~~snet.example_service.ExampleService~~ example_organization.example_service.example_service.ExampleService and ~~snet.example_service.Request~~ example_organization.example_service.example_service.Request. (UPDATE: example_service is repeated twice because protobuf file contains package.

vsbogd commented 5 years ago

We need to pass org_id and service_id somehow, or we need to hardcode it somewhere, because we need to find metadata for this service. ( As an alternative we could directly hardcode metadata, but in this case we will not support any updates to metadata.)

Answering @astroseger comment here. In my example org_id and service_id are not required to create service instance as they are imprinted into service stubs code after snet sdk step. Thus code generated by snet sdk for "example service" could be the following:

class ExampleService:
    def __init__(self, session):
        self.session = session
        self.org_id = "example-organization"
        self.service_id = "example-service"

    def add(self, request):
        # below are pseudo-code steps we need to execute to call method
        md = formGrpcMetadata("add", request)
        response = self.grpc_stub().add(request, metadata=md)
        return response

To generate this code from protobuf service description I thought about developing protobuf compiler plugin.

vsbogd commented 5 years ago

Regarding channel management API I see two main controls we should provide to API user:

Payment channel state caching strategy - it depends on client application architecture whether there is:
1. Only client instance which uses the channel - in such case SDK can cache channel state locally and rely on this state
2. Channel is shared between multiple clients - I would provide to client developer the following options:
  1. share channel state automatically via daemon channel service (like it is implemented in snet-cli)
  2. client developer may implement channel state sharing mechanism by himself.
Payment channel update strategy, we could have the following strategies:
1. Automatic channel update - strategy may have parameters like: minimal funds, minimal expiration time, funds increment, expiration time increment
2. Manual channel update - client developer decides when and how update channel state, then SDK logic just checks that channel state is good enough to make a call and returns error if it is not a case

Payment address and channel are bounded to service replicas group. So I think both group_id and payment channnel strategies should be parameters of the service instance constructor.

vsbogd commented 5 years ago

Answering @ferrouswheel comments here

What will people use if they are testing their service and want to use the same code path, but making grpc requests without the extra SNet headers?

In my example generated service API is absolutely identical to gRPC once the only difference is the way of "gRPC channel"/"Snet session" initialization. I think the most simplest way is to switch between plain gRPC and snet infrastructure is to replace line:

example_service = example_service.Calculator(session, channel_update_strategy, channel_caching_strategy)

by line:

example_service  = example_service_pb2_grpc.ExampleServiceStub(channel)

and vice versa.

how do we check that the generated code is correct for the network/registry being used in the session?

Generated service code should do this work it should download service metadata and make all necessary checks.

what if there are multiple orgs with the same service name that the sdk user wants to integrate with? (e.g. if they are comparing the accuracy of two similar services provided by different people)

As I wrote here, I think we need to generate different python packages for them and then use fully qualified names for classes.

if we have most of the session information when the stub is generated (ipfs, network id), why not encode that in the generated code to avoid mismatches?

I think having them outside will allow us testing same service in different environments.

How to we translate characters that are not allowed in module names like -?

We can translate them as _ and if some client developer needs to work with conflicts in resulting packages let him rename packages as he wants. It will not work for some languages (Java for example). Another option is provide user ability to rename package on code generation stage. For example using snet sdk --package <target package name>

If we do this anyway, I think we should namespace services from the rest of our sdk snet.service.ORG.SERVICE_NAME. Mixing it in with the root module could result in weird name collisions and possible security issues.

Agree it is a good option.

vsbogd commented 5 years ago

We have two slightly different threads above:

statically compiled API stubs to call snet services like if there were local classes (suggested by me)
dynamically generated API to call snet services (suggested by @astroseger)

Thanks to Python abilities they doesn't look very different. But for statically compiled languages without reflection like Golang second way will not be possible in such natural form. It is the reason why I started this issue from statically compiled example.

Dynamically generated API suggested by @astroseger is based on using plain protobuf compiler with grpc plugin. But it also can be based on using snet sdk compiler which is protobuf compiler with snet-sdk plugin. In such case dynamic API incorporates static API and static approach is reused to implement dynamic one.

ferrouswheel commented 5 years ago

if we have most of the session information when the stub is generated (ipfs, network id), why not encode that in the generated code to avoid mismatches?

I think having them outside will allow us testing same service in different environments

My issue with this is that the service could have a completely different API in different environments. I think that once the API is generated for a given environment, it should be bound to it. Or at least calling a kovan service with a mainnet session should raise an error if the service has different metadata.

(this only applies to the statically compiled API stubs case that @vsbogd mentions)

chetankothari commented 5 years ago

SDK API design proposal (Dynamic languages)

I think we have great ideas throughout the thread and taking inspiration from most of them here is an API design that I and @raamb have come up with:

The scope of this API proposal is for building an SDK for AI service consumer (client). We could re-use existing code from snet-cli if needed by extracting them out into a common package.

The high level api looks something like this.

import snetSdk from 'snet-client-sdk';
import { DefaultChannelManagementStrategy, DefaultChannelSelectionStrategy } from 'snet-client-sdk';

const sdk = snetSdk.init(configFilePath);

sdk.account.depositToEscrowAccount(0.00000001);

const defaultChannelSelectionStrategy = new DefaultChannelSelectionStrategy();
const defaultChannelManagementStrategy = new DefaultChannelManagementStrategy(defaultChannelSelectionStrategy);

const serviceClient = sdk.createServiceClient('org-id', 'service-name', defaultChannelManagementStrategy);

const addRequestData = {a: 1, b: 2};
const addResponse = serviceClient.stub.add(addRequestData);

const multiplyRequestData = {a: 3, b: 6};
const multiplyResponse serviceClient.stub.mul(multiplyRequestData);

DefaultChannelManagementStrategy is a strategy which could be provided by us but the developers can define their own ChannelManagementStrategy and pass it to sdk.createServiceClient . Following would be the api contract for ChannelManagementStrategy

class ChannelManagementStrategy {
  setup(mpeContract, serviceClient, account, tokens)
}

The DefaultChannelManagementStrategy implementation would be something like this

class DefaultChannelManagementStrategy extends ChannelManagementStrategy {
  constructor(channelSelectionStrategy) {
    this.channelSelectionStrategy = channelSelectionStrategy;
  }

  setup(mpeContract, serviceClient, account, tokens) {
    // pseudo code
    // get all the channels
    // use channel selection strategy to pick a channel for performing the operation (this could be done by either using the ChannelSelectionStrategy abstraction or however the developer would like it to. Since we would be giving default channel management strategy we could rely on ChannelSelectionStrategy)
    // create channel no channel found
    // fund channel if insufficient banance with appropriate tokens
  }
}

// Note: the extends here just signifies that it follows ChannelManagementStrategy contract

class ChannelSelectionStrategy {
  get select(channels)
}

class DefaultChannelSelectionStrategy extends ChannelSelectionStrategy {
  get select(channels) {
    // pseudo code
    // return null if channels list is empty
    // pick a channel with max unsued funds and return the channel id
  }
}

The general idea of adding ChannelManagementStrategy and ChannelSelectionStrategy is that we can provide a template of interacting with the AI service while allowing the critical sections to be configurable. We could provide a bunch of default strategies which the developers could use if they are satisfied with.

sdk.createServiceClient will take care of fetching the proto definition from the IPFS and generating appropriate stubs. This will only be possible for dynamic languages and the above code is in javascript. I am sure the same should work for python too.

The statically typed languages could use the same api but with just an additional parameter towards the end, which could be the generated stub and the sdk in this case will not take care of generating the stubs. This could be done outside the scope of sdk and imported.

This is the simplest use case and we need to define the complete set of methods and operations that could be possible with the sdk. Once we have a high level API defined we can get started implementing it and iterate on it as we get more clarity.

vsbogd commented 5 years ago

My issue with this is that the service could have a completely different API in different environments. I think that once the API is generated for a given environment, it should be bound to it. Or at least calling a kovan service with a mainnet session should raise an error if the service has different metadata.

@ferrouswheel , could you please explain this case further? Do you mean that you usually have two different service API versions published in different Ethereum network? Like you have service release version published in Mainnet and some work-in-progress version of API published in local ethereum or Kovan for debugging purposes?

I am not sure if scenario I have described above is a big issue as:

breaking change in API means that you need to rewrite your client code, so you cannot have same code working with two different API versions;
you can have two different API packages versions generated for different API versions;
typical service client developer will probably work with only version of API.

pennachin commented 5 years ago

On this: most developers would much rather not have to think about channels. They will want us to provide them with a default approach that is sensible so they can focus on feature development, not payment handling. So I think we should spend our time figuring out how to make the default way to use the SDKs reflect that preference. Making it configurable doesn't need to be there in v1 and there's plenty of higher priority issues.

The general idea of adding ChannelManagementStrategy and ChannelSelectionStrategy is that we can provide a template of interacting with the AI service while allowing the critical sections to be configurable. We could provide a bunch of default strategies which the developers could use if they are satisfied with.

raamb commented 5 years ago

@pennachin we will be seeding defaults for all these options so that the developer can use the SDK without having to configure much. However we do want to have the provision in the SDK to customise this in future, given that once we release the SDK we need to worry about backward compatibility we want to ensure that the first approximation is as good as possible.

raamb commented 5 years ago

@vsbogd @astroseger @ferrouswheel @vforvalerio87 please share your thoughts on the API, if nothing is amiss we can start implementation in respective languages. Of course we will iterate based on our findings as we implement.

vsbogd commented 5 years ago

@chetankothari , @raamb , few remarks on design example you have posted:

(1) In following code snippet from your example snetSdk.init method loads configuration from file:

const sdk = snetSdk.init(configFilePath);

I would prefer have a choice between providing configuration in a code itself or load it from external storage. So I would instead introduce configuration interface (or class) and pass it to snetSdk.init method like this:

configuration = new Configuration();
configuration.setIpfsEndpoint(...);
configuration.setEthereumEndpoint(...);
const sdk = snetSdk.init(configuration);

Also I think we need to list which parameters are kept in configuration, at list major one.

(2) I think we should add ChannelUpdateStrategy and ChannelStateProvider to the API to implement things I have described here. If I understand your suggestion correctly then ChannelManagementStrategy should be just sum of ChannelSelectionStrategy, ChannelUpdateStrategy and ChannelStateProvider.

To be clear ChannelStateProvider responsibility to provide a state of the channel and we have options to calculate is using algorithm implemented in snet-cli, or cache it locally, or ask developer write custom strategy implementation.

interface ChannelStateProvider {
  ChannelState getState(BigInt channelId)
}

ChannelUpdateStrategy is something that is less clear to me, it should be an interface which either updates channel state automatically to have funds up to date or delegates this to client developer. In your example ChannelManagementStrategy is something similar but it contains ChannelUpdateStrategy so for me looks as more general thing.

(3) I don't understand whether we really need this stub part in serviceClient.stub. Could you please explain why do we need it?

vsbogd commented 5 years ago

I have tried to draw UML diagram for design we have right now: SVG diagram

You can see source code and edit it here

chetankothari commented 5 years ago

(1) In following code snippet from your example snetSdk.init method loads configuration from file:

const sdk = snetSdk.init(configFilePath); I would prefer have a choice between providing configuration in a code itself or load it from external storage. So I would instead introduce configuration interface (or class) and pass it to snetSdk.init method like this:
configuration = new Configuration();
configuration.setIpfsEndpoint(...);
configuration.setEthereumEndpoint(...);
const sdk = snetSdk.init(configuration);
Also I think we need to list which parameters are kept in configuration, at list major one.

Makes sense, because this could allow clients running on machines with no access to filesystem.

Listing the configuration needed for the sdk to work.

Network (can have default)
Registry contract address with ABI (can have default)
Token contract address with ABI (can have default)
MPE contract address (can have default)
IPFS address (can have default)
Identity (mandatory)

As mentioned in the list we only need the identity to as mandatory and we could have defaults for the rest of them.

2) I think we should add ChannelUpdateStrategy and ChannelStateProvider to the API to implement things I have described here. If I understand your suggestion correctly then ChannelManagementStrategy should be just sum of ChannelSelectionStrategy, ChannelUpdateStrategy and ChannelStateProvide.

To be clear ChannelStateProvider responsibility to provide a state of the channel and we have options to calculate is using algorithm implemented in snet-cli, or cache it locally, or ask developer write custom strategy implementation.
interface ChannelStateProvider {
  ChannelState getState(BigInt channelId)
}
ChannelUpdateStrategy is something that is less clear to me, it should be an interface which either updates channel state automatically to have funds up to date or delegates this to client developer. In your example ChannelManagementStrategy is something similar but it contains ChannelUpdateStrategy so for me looks as more general thing.

Yes, your are right we were also thinking along similar lines about ChannelManagementStrategy being a sum of all the other strategies. This could be dealt at implementation level as the sdk would only have to rely on ChannelManagementStrategy.

(3) I don't understand whether we really need this stub part in serviceClient.stub. Could you please explain why do we need it?

This was just to simplify the part where we have to dynamically add methods to the ServiceClient. Some languages may allow us to add new methods to an existing class at runtime but some may not. So tried to keep it simple and this will help us have a uniform api for the client developer.

raamb commented 5 years ago

Hi @vsbogd here is a simplified version of the diagram that you had. We focused on how the SDK design would look like. Essentially we are still looking at a ChannelManagementStrategy which will compose ChannelSelectionStrategy, ChannelUpdateStrategy and ChannelStateProvider. We are looking at keeping the abstraction at the ChannelManagementStrategy level for the SDK but the implementation would use the classes you mentioned. Again the idea is to get started with a fairly good approximation as the first step and then iterate over it.

SDK

I feel that we have a pretty good high level API to start implementing with. We will have additional discussions where necessary as we proceed with the implementation.

vsbogd commented 5 years ago

I feel that we have a pretty good high level API to start implementing with. We will have additional discussions where necessary as we proceed with the implementation.

@raamb , @chetankothari , I agree that essentially we can start from this.

vsbogd commented 5 years ago

I think that couple of interfaces should be enough to provide user an API to manage channel state:

One is to update channel state:

class PaymentChannel {
  Transaction addAmount(amount);
  Transaction extendExpiration(expiration);
}

class Transaction {
  Result get(timeout);
  cancel();
  State state();
}

SDK could return PaymentChannel instance to the user:

sdk.paymentChannel(channelId).addAmount(0.001);
sdk.paymentChannel(channelId).extendExpration(...);

And second is to listen channel state updates:

class ChannelListener {
  onUpdate(ChannelState old, ChannelState new);
}

Then client can implement channel listener and subscribe to updates from channel provider to implement any channel funding strategy.

vsbogd commented 5 years ago

I have updated diagram above:

I have removed unclear ChannelFundingStrategy and replaced it by PaymentChannel and ChannelListener
ThresholdChannelUpdateStrategy implements ChannelListener and keeps reference to PaymentChannel to react on channel state changes and plan channel update (after service method call or blockchain operation on channel)
naming improvements: Account -> MultiPartyEscrowAccount (as Account is too general), Channel -> PaymentChannel (to not confuse with gRPC Channel)
added Transaction interface to provide user async interface of working with transactions

The diagram: SVG diagram

The source code

vsbogd commented 5 years ago

Sharing the result of discussion with @vforvalerio87 , @raamb and @astroseger. We have discussed three ways to implement the SDK:

Use plain gRPC protobuf plugin to generate gRPC stubs and add SDK functionality using client-side gRPC channel interceptors;
Write SDK gRPC plugin to generate gRPC stubs with SDK specific code embedded, solution which I mentioned at very first example
Subclass gRPC Channel and ClientCall to wrap gRPC API with SDK functionality

We have agreed on using approach (1) to implement SDK, it seems most simple and powerful enough to implement metadata injection and choose endpoint. Other ways can be used in case gRPC API is not enough to do all SDK related work.

astroseger commented 5 years ago

@raamb @vforvalerio87

It will be a little bit complicated so please follow me.

As I see in API which proposed @chetankothari for js there is the following line:

const addRequestData = {a: 1, b: 2};
const addResponse = serviceClient.stub.add(addRequestData);

So he passes parameters as a "dictionary", not as request_class. Which actually makes a lot of sense for me, because it seems there is not advantage in passing parameters as request_class in python, because, in any case, everything will be validated inside the stub. It is especially true in complicated cases!

Let's consider a slightly complicated example with message contains another messages.

Let us have example service with the following protobuf (message Numbers contains message Number)

syntax = "proto3";
package example_service;

message Number {
     float x = 1;
     string name = 2;
}

message Numbers {
    Number a = 1;
    Number b = 2;

}

message Result {
    Number value = 1;
}

service Calculator {
    rpc add(Numbers) returns (Result) {}
    rpc sub(Numbers) returns (Result) {}
}

We can have two variants of python SDK API.

Pass parameters as request_class (API which I've discussed here: https://github.com/singnet/snet-sdk-python/issues/16#issuecomment-471613380 )

Please note that you actually have two alternatives regarding nested messaged here. You can pass nested message as a dictionary and it will work!

from snet_cli.sdk import Session, Client, AutoFundingFundingStrategy
from snet_cli.config import Config
session = Session(Config())
client = Client(session, "snet", "example-service", AutoFundingFundingStrategy(10, "+20days"))

# V1.1: using Number and Numbers classes
a = client.classes.Number(x=20, name="a")
b = client.classes.Number(x=10, name="b")
request = client.classes.Numbers(a=a, b=b)
rez = client.stub.add(request)

# V1.2: using only Numbers classes (yes! it will work in grpc python automatically)
request = client.classes.Numbers(a={"x":20, "name":"a"}, b={"x":10, "name" : "a"})
rez = client.stub.add(request)

Second variant of python SDK API (which will be slightly more complicated to implement though)

from snet_cli.sdk import Session, Client, AutoFundingFundingStrategy
from snet_cli.config import Config
session = Session(Config())
client = Client(session, "snet", "example-service", AutoFundingFundingStrategy(10, "+20days"))

rez = client.stub.add({"a" : {"x":20, "name":"a"}, "b" : {"x":10, "name" : "a"} })

which variant of API do you prefer?

ferrouswheel commented 5 years ago

@sergey For dynamic languages I think it's fine to allow dictionary as request parameters, so long as we can still pass the original message objects.

For example, if you call one service e.g. face-detect, and then want to use the response as a parameter to call face-landmarks, the response from face-detect will be proto message objects, unless we also convert responses to dictionaries.

The client should be able to easily chain data from one service to the next, without extra conversion steps (even if they are implicitly handled by the SDK, it is extra overhead/latency that is unnecessary)

BTW - nested messages, represented as a dictionary, already works with snet-cli when I call my services. So maybe it's supported by grpc python already? (or you added that feature ;-) )

astroseger commented 5 years ago

@ferrouswheel Yes! Passing nested messages as a dictionaries is a build feature in grpc python. It means that you can do as following (in context of nested protobuf in https://github.com/singnet/snet-sdk-python/issues/16#issuecomment-477704271):

# Numbers is a request_class for client_stub 
# client_stub is grpc client stub
request = Numbers(a = {x:10, "name" : "a"}, b = {x:20, "name": "b"})
client_stub.add(request)

But it seems there is no build in mechanism for passing full message as a dictionary (you must have a request_class)... or is it?

request = {"a" : {x:10, "name" : "a"}, "b" : {x:20, "name": "b"}}

# this will not work :( 
client_stub.add(request)

# It seems the only one way is to convert to request_class by hand
client_stub.add( Numbers(**request) )

But in any case I do agree with you that we should allow passing request_class (not only a dict).

Ok as a first demonstration I would prose the following API, by default we pass request_class, but we have mechanism for passing dictionary without knowing the name of request_class

for example (in context of https://github.com/singnet/snet-sdk-python/issues/16#issuecomment-477704271):

request_dict = {"a":{"x":20, "name":"a"}, "b":{"x":10, "name" : "a"}}
rez = client.stub.add(client.get_request_class("add")(**request_dict))

raamb commented 5 years ago

@astroseger the approach of supporting both message object and dictionary sounds good to me

astroseger commented 5 years ago

I've implemented python-SDK which fully comply to discussed API (branch new-sdk on my fork on snet-cli https://github.com/astroseger/snet-cli/tree/new-sdk). @vforvalerio87 @raamb I think our target now is to do the same but more elegant.

@vforvalerio87 you could reuse utils_proto4sdk.py, general architecture and CI.

I want to note that it has the following features:

CI for SDK! (see test/functional_tests/script14_test_sdk.sh)
Dynamical caching of channels, metadata and compiled protobuf.
Support of json encoding!
Two funding strategy (Basic and Automatic)

You can tests it by yourself, everything is functional

from snet_cli.sdk import Session, Client, AutoFundingFundingStrategy
from snet_cli.config import Config

# we directly use snet-cli configuration
# another types of configuration can be easiliy implemented (for example hard-coded configuration)
session = Session(Config())

# In case of multiply grpc_service names you should specify the name of grpc_service
# Here we have only one grpc_service (Calculator), so we can omit this parameter
client = Client(session, "snet", "example-service", AutoFundingFundingStrategy(amount_cogs = 4, expiration = "+10days"))
for i in range(10):
    request = client.classes.Numbers(a=20, b=i)
    rez = client.stub.add(request)
    print("add", rez)
    request = client.get_request_class("mul")(a=20, b=i)
    rez = client.stub.mul(request)
    print("mul", rez)

vsbogd commented 5 years ago

I have redrew the diagram incorporating results of our today discussion.

PaymentChannelStrategy interface has two implementations:

ThresholdChannelUpdateStrategy updates channel or creates new one when required
SingleChannelStrategy just returns the only channel it contain

ThresholdChannelUpdateStrategy uses MultiPartyEscrowAccount to get list of channels, open channel, PaymentChannel has methods to get the channel state or extend channel.

SDK has methods to get ServiceMetadata, create SnetGrpcChannel, get DynamicClassProvider and get MultiPartyEscrowAccount.

Source code

SVG Diagram

chetankothari commented 5 years ago

Looks great @vsbogd. I have further added more details to individual components and tried to simplify some components. One major change we will see is the ServiceClient abstraction which encapsulates most of the service related information and operation to be performed on the service. Source code

snet-sdk-class-diagram

ksridharbabuus commented 5 years ago

@chetankothari @vsbogd Quick thoughts:

Is it possible to add Identity type for json file with PWD? I remember we support this in CLI as well.
Is it possible to refer the base class or SDK initialization using the name SingulairtyNet. Or atleast can look at keeping it in package name or namespace name? I saw this as std implementation in many SDKs, recent one Uniswap also followed the same.
Somehow I am comfortable calling class as sdk, might need to rename (refer above point)
Do we completely abstracted Organization Details from the sdk? In case if the user wants to know about the service publisher, is there any way we can expose through sdk or do they have to query from Blockchain? Look at some of the requirements where users can build their own DApps using SDK.

Minor spelling correction in the Class Name: DinamicClassProvider -> DynamicClassProvider

raamb commented 5 years ago

@vforvalerio87 @vsbogd @astroseger please review and sign off on the API. We can start to building this out right away

raamb commented 5 years ago

@vforvalerio87 @vsbogd @astroseger havent received any comments so take it that we are all good with the API proposed in https://github.com/singnet/snet-sdk-python/issues/16#issuecomment-488007331 Lets proceed accordingly in Python and JS first and then move on to GO. @vforvalerio87 this become your priority for this week.

vsbogd commented 5 years ago

@chetankothari, @raamb, sorry for a late response. My comments to the questions of @chetankothari above:

Is it possible to add Identity type for json file with PWD? I remember we support this in CLI as well.

Not sure what is PWD but list of Identity implementations can be extended for sure.

Is it possible to refer the base class or SDK initialization using the name SingulairtyNet. Or atleast can look at keeping it in package name or namespace name? I saw this as std implementation in many SDKs, recent one Uniswap also followed the same.

Yes, actually I think all this stuff should have some root package namespace to separate it from other software. singularitynet should be fine.

Do we completely abstracted Organization Details from the sdk? In case if the user wants to know about the service publisher, is there any way we can expose through sdk or do they have to query from Blockchain? Look at some of the requirements where users can build their own DApps using SDK.

I don't think we should abstract Organization. I didn't add it to the diagram yet as I think it is not so important for the first SDK version and I would not like thinking about all things at once. We can add some separate method to work with organization to the SDK later.

vsbogd commented 5 years ago

Major comments on ChannelManagementStrategy:

I would rename it to PaymentChannelManagementStrategy or PaymentChannelStrategy as "channel" has at least two senses in this context (gRPC channel, payment channel)
I see select method gets ServiceClient and SDK as input; SDK is a context do you think we need to pass it each time as parameter when it can be passed only once when strategy instance is created?

singnet / snet-sdk-python

SDK API discussion #16

ATTENTION!!! this example will not work, it is not implemented yetfrom snet_cli.sdk import TestClient

SDK API design proposal (Dynamic languages)