Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request metadata: discussion and design #1480

Closed
RafalSkolasinski opened this issue Feb 24, 2020 · 1 comment
Closed

Request metadata: discussion and design #1480

RafalSkolasinski opened this issue Feb 24, 2020 · 1 comment
Labels
priority/p1 Roadmap Feature High level roadmap feature for planning
Milestone

Comments

@RafalSkolasinski
Copy link
Contributor

RafalSkolasinski commented Feb 24, 2020

What this issue is about

This issue is part of greater scope of #1362 narrowed down to just request metadata.
Request metdata will be defined as in comment #1362 (comment) quoted below


The request metadata is in principle metadata about the request itself.

It can be immutable data that comes as a part of the request. In example:

  • some user_id who send the request (for audit purposes)
  • some unique request_id (could be useful in "Batch" processing)

This data should be exposed to predict method but guaranteed to be immutable.

The other example of request metadata provided by @adriangonz is recording version of external libraries accessed during the time of processing the request, e.g. some_api_version. This type of data is:

  • unknown to sender on time of the request
  • may change in between requests

We should provide an easy and thread-safe way to include that kind of metadata in the request response.

Originally posted by @RafalSkolasinski in #1362 (comment)


This issue is also partially related to #1474. The difference is that #1474 is only about preserving current functionality, between engine and executor, whereas this issue is to add new features to how request metadata is handled.

Proposal of changes

Key features

Distinguishing two subtypes of request metadata:

  1. incoming request metadata: part of initial request (e.g. user_id or request_id)
  2. runtime request metadata: become known during request processing (e.g. some_api_version)

These are key points I propose:

  • incoming request metadata is immutable
  • incoming request metadata should also included in the final response
  • incoming request metadata available throughout the inference graph
  • runtime reqest metadata can be set in a thread-safe way
  • runtime reqest metadata is included in the final resposne
  • runtime reqest metadata should be easily available to all further steps in the inference graph

Relation to tags

Tags are mentioned in the official documentation here and here. As this is part of seldon-core 1.0 released we should preserve that functionality as described in #1474.

It should be however clearly noted in the documentation that if that method accesses any stateful information of the model's instance (for example dynamically by .predict field self._meta) then this may not be thread-safe.

I suggest to completely "decouple" usage of tags from the problem of request metadata handling.

Possible user-side usage example

modifying meta input argument

In Python, if the function's argument is list, dict or any custom object then modification to it is actually modifying the input variable not its copy - a bit like this would be a pointer.

We could leverage that to have sth like

class Model: 
    ....
    def predict(self, data, names, meta):
        meta['runtime-meta-key'] = "some value"
        ...

Pros:

  • easy to implement by user's in their custom models

Cons:

  • may be difficult to guarantee immutability of incoming request metadata

through the return value of predict method

We could include runtime request metadata as return value of predict method, in example

class Model: 
    ....
    def predict(self, data, names, meta):
        ...
        return (output, runtime_metadata)

and inspect if the returned value is a tuple or not.

However, this can lead to possible confusion between outputs like return [1, 2] (output of model is two-element array) from return (1, meta_data) (two-element tuple with second element being metadata). It could definitely be doable (like inspect if second element is dict) but I don't really like it.

Other possibility could be to have it as

from seldon_core... import SeldonPredictResponse
class Model: 
    ....
    def predict(self, data, names, meta):
        ...
        return SeldonPredictResponse(data=..., meta=...)

The SeldonPredictResponse could allow to give better control over the response type (numpy array, json, list, string as well as clean way to return the runtime request metadata.

Pros:

  • easy to implement by user's in their custom models
  • immutability of the incoming request metadata can be handled at the wrapper / microservice level

Cons:

  • ???

Special method

Last possibility I propose would be using a separate method, like tags, but making sure that it can be used in a thread-safe way.
To guarantee that we could have it

class Model: 
    ....
    def predict(self, data, names, meta):
        # Can access `meta` but cannot modify it. 
        # Should not store any state in `self._meta` or similar.
        ...
        return output

    def runtime_metadata(self, data, names, meta):
        # some logic that defines the **runtime** `request metadata` but does 
        # not store nor access from via the instance state
        ...
        return runtime_metadata    

Pros:

  • easy to implement by user's in their custom models
  • actually could leverage existing tags method

Cons:

  • if executed long time after predict it may record different version of external APIs
@RafalSkolasinski RafalSkolasinski added the triage Needs to be triaged and prioritised accordingly label Feb 24, 2020
@ukclivecox ukclivecox removed the triage Needs to be triaged and prioritised accordingly label Mar 6, 2020
@ukclivecox ukclivecox added this to the 1.2 milestone Mar 6, 2020
@ukclivecox ukclivecox added the Roadmap Feature High level roadmap feature for planning label Mar 6, 2020
@RafalSkolasinski
Copy link
Contributor Author

RafalSkolasinski commented Apr 21, 2020

Just to update current state of request metadata with a short note.

Request metadata will be known as request tags and coupled to the tags method that can be defined in the user's models - not an independent being as I was proposing earlier.

There still stand an open problem how to define them correctly on the fly (for example set value in the predict method).

After #1474 the tags defined at one step in the inference graph will be passed to the next step and merged with tags defined there.

As side effect one can now modify meta["tags"] dict passed to the predict method

Model:
    ....
    def predict(self, features, names=[], meta=[]):
        logging.info(f"model meta: {meta}")
        meta["tags"]["predict-defined"] = "success"

and have this change successfully propagate further.

Let me just stress that this is not (yet?) supported feature, only side effect of current implementation.
It may be worth to consider having it this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/p1 Roadmap Feature High level roadmap feature for planning
Projects
None yet
Development

No branches or pull requests

2 participants