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 does not send feedback to Routers. #2827

Closed
ukclivecox opened this issue Jan 10, 2021 · 4 comments · Fixed by #2865
Closed

Executor does not send feedback to Routers. #2827

ukclivecox opened this issue Jan 10, 2021 · 4 comments · Fixed by #2865
Assignees
Labels

Comments

@ukclivecox
Copy link
Contributor

https://github.com/SeldonIO/seldon-core/blob/master/executor/predictor/predictor_process.go#L135

@ukclivecox ukclivecox added the bug label Jan 10, 2021
@ukclivecox ukclivecox self-assigned this Jan 10, 2021
@axsaucedo
Copy link
Contributor

In this case it would skip calling the ROUTER component in favour of getting it from the meta (

route, err := p.routeFeedback(node, msg)
), and then it would just call the MODEL components via send_feedback instead of predict, so it should still pass the feedback to the router MODEL nodes.

Do you mean that currently it's not passing the data to the MODEL components?

@ukclivecox
Copy link
Contributor Author

For MABs there is logic in send-feedback to update state so I think it should send feedback to routers.

@axsaucedo
Copy link
Contributor

Oh I see what you mean, so basically it shouldn't call the "route" method, but it should still call the send_feedback method. Ok I can see that the MAB router also has a send_feedback logic, so it would make sense. It seems that also adding the v1.SEND_FEEDBACK method in:

if hasMethod(v1.SEND_FEEDBACK, node.Methods) {

It seems that the predictor bean also has similar logic:

if (predictorConfig.hasMethod(PredictiveUnitMethod.SEND_FEEDBACK, state)) {

I'm not as familiar with the java side, so just to confirm, is this assumption correct?

I do agree that it would make sense to also ensure the send_feedback is passed to the router element, as it could help update its current state. Do you think it should be done explicitly by providing the method in the CRD, or by default?

@ukclivecox
Copy link
Contributor Author

Yes. That makes sense. The logic is in the engine but missing from the executor.

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.

2 participants