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

requestPath meta missing in new executor #2505

Closed
RafalSkolasinski opened this issue Sep 30, 2020 · 6 comments · Fixed by #2587
Closed

requestPath meta missing in new executor #2505

RafalSkolasinski opened this issue Sep 30, 2020 · 6 comments · Fixed by #2587
Assignees
Milestone

Comments

@RafalSkolasinski
Copy link
Contributor

RafalSkolasinski commented Sep 30, 2020

Legacy engine used to set meta field request.Path that is missing in executor.
Example of payload differences may be find in this issue: #1474

@RafalSkolasinski RafalSkolasinski added bug triage Needs to be triaged and prioritised accordingly labels Sep 30, 2020
@RafalSkolasinski RafalSkolasinski added this to the 1.4 milestone Oct 1, 2020
@RafalSkolasinski RafalSkolasinski removed the triage Needs to be triaged and prioritised accordingly label Oct 1, 2020
@RafalSkolasinski RafalSkolasinski self-assigned this Oct 1, 2020
@Isaacwhyuenac
Copy link

I would also appreciate adding back the requestPath information which is somehow lost after the engine replaced its Spring boot server to gorilla mux.
https://github.com/SeldonIO/seldon-core/pull/246/files

@RafalSkolasinski
Copy link
Contributor Author

I believe Spring boot -> gorilla mux is just replacing Java based Engine with Golang based Executor so basically the same as spirit of this issue.

@axsaucedo axsaucedo modified the milestones: 1.4, 1.5 Oct 15, 2020
@RafalSkolasinski
Copy link
Contributor Author

On this specific issue, I have the feeling that almost the same functionality can be achieved on model side with a minimal implementation in the model:

model:

import os


UNIT_ID = os.environ.get("PREDICTIVE_UNIT_ID", "???")
MODEL_IMAGE = os.environ.get("PREDICTIVE_UNIT_IMAGE", "???")


class Model:
    def predict(self, features, names=[], meta=[]):
        print(features)
        return features

    def tags(self):
        return {f"requestPath:{UNIT_ID}": MODEL_IMAGE}

deployment.yaml:

apiVersion: machinelearning.seldon.io/v1
kind: SeldonDeployment
metadata:
  labels:
    app: seldon
  name: seldon-mock-model
spec:
  name: mock-deployment
  predictors:
  - componentSpecs:
    - spec:
        containers:
        - image: mock-model:latest
          imagePullPolicy: IfNotPresent
          name: model-1

        - image: mock-model:latest
          imagePullPolicy: IfNotPresent
          name: model-2
    graph:
      name: model-1
      type: MODEL
      children:
      - name: model-2
        type: MODEL
    name: default
    replicas: 1

test:

❯❯❯ make request
curl -s -H 'Content-Type: application/json' \
	-d '{"meta": {"tags": {"foo": "bar"}}, "data": {"names": ["input"], "ndarray": ["data"]}}' \
	http://localhost:8003/seldon/seldon/seldon-mock-model/api/v1.0/predictions  | jq .
{
  "data": {
    "names": [],
    "ndarray": [
      "data"
    ]
  },
  "meta": {
    "tags": {
      "foo": "bar",
      "requestPath:model-1": "mock-model:latest",
      "requestPath:model-2": "mock-model:latest"
    }
  }
}

This however does not solve problem for prepackaged model servers.

@RafalSkolasinski
Copy link
Contributor Author

RafalSkolasinski commented Oct 27, 2020

It seems we could restore the exact original functionality by modifying Python Wrapper only.
Though, it probably wouldn't apply for custom models using _raw methods, .e.g. predict_raw.

@anirudh1800
Copy link

Thanks for doing an analysis of this issue.

Please let me know if you need any help in contributing to the codebase. I can pick some issues/bugs in my free time.

@RafalSkolasinski
Copy link
Contributor Author

Hi @anirudh1800,

I have a draft of PR #2587 that adds this feature back on Python Wrapper level - if you could give it a test drive that would probably be very helpful :)

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

Successfully merging a pull request may close this issue.

5 participants