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

executor: tags do not propagate through inference graph #1474

Closed
RafalSkolasinski opened this issue Feb 21, 2020 · 11 comments · Fixed by #1481
Closed

executor: tags do not propagate through inference graph #1474

RafalSkolasinski opened this issue Feb 21, 2020 · 11 comments · Fixed by #1481
Assignees
Labels
Milestone

Comments

@RafalSkolasinski
Copy link
Contributor

RafalSkolasinski commented Feb 21, 2020

For example, same requests with engine

curl -X POST -H 'Content-Type: application/json' \
        -d '{"data": {"names": ["get"], "ndarray": ["data"]}}' \
        http://localhost:8003/seldon/seldon/step-experiments/api/v0.1/predictions
{
  "meta": {
    "puid": "qgrk9e6dlvpcdcgu0ip539afgt",
    "tags": {
      "current": "three",
      "one": "yes",
      "value_at_one": ["data"],
      "two": "yes",
      "value_at_two": ["one"],
      "three": "yes",
      "value_at_three": ["two"]
    },
    "routing": {
      "step-one": -1,
      "step-two": -1
    },
    "requestPath": {
      "step-three": "seldon-experiments:step-3",
      "step-one": "seldon-experiments:step-1",
      "step-two": "seldon-experiments:step-2"
    },
    "metrics": []
  },
  "data": {
    "names": [],
    "ndarray": ["two"]
  }

and with executor

curl -X POST -H 'Content-Type: application/json' \
        -d '{"data": {"names": ["get"], "ndarray": ["data"]}}' \
        http://localhost:8003/seldon/seldon/step-experiments/api/v0.1/predictions
{"data":{"names":[],"ndarray":["two"]},"meta":{"tags":{"current":"three","three":"yes","value_at_three":["two"]}}}

both performed using 1.0.3-SNAPSHOT and for following seldon deployment:

apiVersion: machinelearning.seldon.io/v1alpha2
kind: SeldonDeployment
metadata:
  labels:
    app: seldon
  name: step-experiments
spec:
  name: three-steps
  annotations:
    seldon.io/executor: "true"
  predictors:
  - componentSpecs:
    - spec:
        containers:
        - image: seldon-experiments:step-1
          name: step-one
          imagePullPolicy: IfNotPresent
          env:
          - name: SELDON_LOG_LEVEL
            value: DEBUG
        - image: seldon-experiments:step-2
          name: step-two
          imagePullPolicy: IfNotPresent
          env:
          - name: SELDON_LOG_LEVEL
            value: DEBUG
        - image: seldon-experiments:step-3
          name: step-three
          imagePullPolicy: IfNotPresent
          env:
          - name: SELDON_LOG_LEVEL
            value: DEBUG
    graph:
      children:
      - children:
        - endpoint:
            type: REST
          name: step-three
          type: MODEL
        endpoint:
          type: REST
        name: step-two
        type: MODEL
      endpoint:
        type: REST
      name: step-one
      type: MODEL
    name: steps-graph
    replicas: 1

(see models here)

Issue related to #1362 in order to provide backwards compatibility with behaviour present with engine.

@RafalSkolasinski RafalSkolasinski added bug triage Needs to be triaged and prioritised accordingly labels Feb 21, 2020
@ukclivecox
Copy link
Contributor

The current executor will pass the payloads through it won't modify any payloads. I assume this is why you just see the response meta from the final model.
If you want to keep metadata its up to each model to copy from request to response.

@RafalSkolasinski
Copy link
Contributor Author

The question is how much we need to preserve functionality provided by .tags method in order to not introduce backward incompatible changes to our users.

For just context of request metadata I am just about open another issue that tackle this specific metadata aspect.

@RafalSkolasinski
Copy link
Contributor Author

Just to define a behaviour we want to preserve I suggest to consider minimal scenario:

  • graph with few steps as above
  • model in each step has tags() method define
  • each tags() method takes no arguments (except self)
  • each tags() method output must be present in meta.tags of the final response (unless it is overwritten by later steps)

The means by which it happens should not be a concern of the user.

Before we move handling of metadata to headers (not sure if we gonna do it even at this point) the fix for that could happen at the python wrapper / microservice level.

I really wouldn't want require from users to define

def predict(self, data, names, meta):
    self._meta = meta

as this will just become ugly boilerplate.

@RafalSkolasinski
Copy link
Contributor Author

Should not forget to verify if graphs with combiner will behave also properly.

@RafalSkolasinski
Copy link
Contributor Author

#1481 Seems to have right now a working solution to the problem.
Here is a request with engine

curl -X POST -H 'Content-Type: application/json' \
        -d '{"meta": {"some": "input meta"}, "data": {"names": ["get"], "ndarray": ["data"]}}' \
        http://localhost:8003/seldon/seldon/step-experiments/api/v0.1/predictions | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   658  100   577  100    81  10122   1421 --:--:-- --:--:-- --:--:-- 11543
{
  "meta": {
    "puid": "lruk80c6r9f4m3r2fqt61r0ns1",
    "tags": {
      "current": "three",
      "one": "yes",
      "value_at_one": [
        "data"
      ],
      "two": "yes",
      "value_at_two": [
        "one"
      ],
      "three": "yes",
      "value_at_three": [
        "two"
      ]
    },
    "routing": {
      "step-one": -1,
      "step-two": -1
    },
    "requestPath": {
      "step-three": "seldon-experiments:step-3",
      "step-one": "seldon-experiments:step-1",
      "step-two": "seldon-experiments:step-2"
    },
    "metrics": []
  },
  "data": {
    "names": [],
    "ndarray": [
      "two"
    ]
  }
}

and here is with executor

curl -X POST -H 'Content-Type: application/json' \
        -d '{"meta": {"some": "input meta"}, "data": {"names": ["get"], "ndarray": ["data"]}}' \
        http://localhost:8003/seldon/seldon/step-experiments/api/v0.1/predictions | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   267  100   186  100    81   5166   2250 --:--:-- --:--:-- --:--:--  7416
{
  "data": {
    "names": [],
    "ndarray": [
      "two"
    ]
  },
  "meta": {
    "tags": {
      "current": "three",
      "one": "yes",
      "three": "yes",
      "two": "yes",
      "value_at_one": [
        "data"
      ],
      "value_at_three": [
        "two"
      ],
      "value_at_two": [
        "one"
      ]
    }
  }
}

@ukclivecox ukclivecox removed the triage Needs to be triaged and prioritised accordingly label Feb 27, 2020
@ukclivecox ukclivecox added this to the 1.1 milestone Feb 27, 2020
@RafalSkolasinski
Copy link
Contributor Author

And here is test with GRPC.

Engine:

cd /home/rskolasinski/work/seldon-core/executor/proto && grpcurl -d '{"data": {"names": ["get"], "ndarray": ["data"]}}' -rpc-header seldon:step-experiments-grpc -rpc-header namespace:seldon -plaintext -proto ./prediction.proto  0.0.0.0:8003 seldon.protos.Seldon/Predict | jq .
{
  "meta": {
    "puid": "ph3cafjilu65adnrke44to9kvj",
    "tags": {
      "current": "three",
      "one": "yes",
      "three": "yes",
      "two": "yes",
      "value_at_one": [
        "data"
      ],
      "value_at_three": [
        "two"
      ],
      "value_at_two": [
        "one"
      ]
    },
    "routing": {
      "step-one": -1,
      "step-two": -1
    },
    "requestPath": {
      "step-one": "seldon-experiments-grpc:step-1",
      "step-three": "seldon-experiments-grpc:step-3",
      "step-two": "seldon-experiments-grpc:step-2"
    }
  },
  "data": {
    "ndarray": [
      "two"
    ]
  }
}

Executor:

cd /home/rskolasinski/work/seldon-core/executor/proto && grpcurl -d '{"data": {"names": ["get"], "ndarray": ["data"]}}' -rpc-header seldon:step-experiments-grpc -rpc-header namespace:seldon -plaintext -proto ./prediction.proto  0.0.0.0:8003 seldon.protos.Seldon/Predict | jq .
{
  "meta": {
    "tags": {
      "current": "three",
      "one": "yes",
      "three": "yes",
      "two": "yes",
      "value_at_one": [
        "data"
      ],
      "value_at_three": [
        "two"
      ],
      "value_at_two": [
        "one"
      ]
    }
  },
  "data": {
    "ndarray": [
      "two"
    ]
  }
}

@RafalSkolasinski
Copy link
Contributor Author

Just updated PR with applying fix to more methods and adding unit tests.

@RafalSkolasinski
Copy link
Contributor Author

The PR does not (and probably won't) contain fix to send_feedback method.
This is because usage of tags in there is questionable and fix would require adjustment of protobufs.

@RafalSkolasinski RafalSkolasinski self-assigned this Mar 3, 2020
axsaucedo pushed a commit that referenced this issue Mar 4, 2020
* Fix tags propagation with Executor, closes #1474

* fix broken log formatting

* apply style fix and increase flakiness
@gmattar
Copy link

gmattar commented Jun 5, 2020

I'm still having some problems related to this.

This is my graph:
image

Basically, all models create tags and the Transformer Output needs to read them before forwarding the result of Model 3. I'm using Seldon 1.1 with REST requests.

The problem is that I can only see the tags from Model 3 inside Transformer.
These are the steps I took and what I found after inspecting the components:

  • First I included a breakpoint inside aggregate but was not able to see the tags, the meta parameter was empty. Then, I included an aggregate_raw function and was able to see them. meta is not passed to client_aggregate in this scenario:

    client_response = client_aggregate(user_model, features_list, names_list)

  • I changed SELDON_LOG_LEVEL to DEBUG and checked the response sent by the Combiner component. The tags were passed to executor

  • Next, I checked the meta parameter passed to Model 3 and the metas were empty again.

  • Last, I checked the tags in Transformer Ouput and only the data of Model 3 was available

@ukclivecox
Copy link
Contributor

I think transform output should have the meta. @RafalSkolasinski ?

@RafalSkolasinski
Copy link
Contributor Author

@gmattar It seems that aggregate methods are not passing meta argument which contain tags.
Could you open a new issue for this and add information what version of wrapper are you using?

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

Successfully merging a pull request may close this issue.

3 participants