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

Routers not working with new Executor #1876

Closed
neopobo opened this issue May 25, 2020 · 3 comments · Fixed by #1890
Closed

Routers not working with new Executor #1876

neopobo opened this issue May 25, 2020 · 3 comments · Fixed by #1890
Assignees
Milestone

Comments

@neopobo
Copy link
Contributor

neopobo commented May 25, 2020

Hi,

When using seldon 1.1 with new executor, router always return first children from its inference graph.

The problem is that when using python or R wrapper the format of message returned is:
{"data":{"ndarray":[[1]]}}
but new executor expects:
{"data":{"ndarray":[1]}}
where "1" is a route number.

Old java implementation is working ok with [[1]] ndarray.

I see in the api>rest>client_test.go file that there is wrong okRouteResponse expected.

My local build of the Executor works when changing in api>utill>utils.go func ExtractRouteFromSeldonMessage

Line 13: routeArr[i] = int(value.GetListValue().GetValues()[0].GetNumberValue())

@neopobo neopobo added bug triage Needs to be triaged and prioritised accordingly labels May 25, 2020
@ukclivecox
Copy link
Contributor

Thanks. Can we extend it to allow both for now. A small PR for this would be great?

@ukclivecox ukclivecox added priority/p1 and removed triage Needs to be triaged and prioritised accordingly labels May 25, 2020
@ukclivecox ukclivecox added this to the 1.2 milestone May 25, 2020
@ukclivecox ukclivecox self-assigned this May 28, 2020
@neopobo
Copy link
Contributor Author

neopobo commented May 28, 2020

I have created PR #1890,
Sorry do not know how to connect it with the issue.

I see I am a little bit late with PR, please feel free to throw it away if You already have the sollution

@ellisvalentiner
Copy link
Contributor

Is there a temporary fix for this or do we have to wait for the next release?

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.

3 participants