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

Inference Graph’s response code should be influenced by node’s response code #2484

Closed
rachitchauhan43 opened this issue Oct 25, 2022 · 4 comments · Fixed by #3039
Closed

Comments

@rachitchauhan43
Copy link
Contributor

rachitchauhan43 commented Oct 25, 2022

/kind feature

Describe the solution you'd like
Users should be able to define every node as either a hard dependency or a soft dependency in an Inference Graph

Current situation:

IG assumes that every node in the graph is a soft dependency including the last node.
By soft dependency, I mean IG's router doesn't halt IG's execution even if any node returns non-200. It continues executing the graph silently.

For example:
If we have an IG with 3 nodes => { N1,N2,N3} in a Sequence, and now if the middle node, N2, returns non-200, IG will continue to call N3 with N2’s erroneous response body.
So, the user would have to make sure that the N3 node can handle that unexpected and erroneous response body from N2 properly.
Even if N3 (i.e. last node) handles it properly and returns 400 due to bad/unexpected request body, IG will still return 200 since the last node is also a soft dependency. And this behavior is not right.

So, basically IG’s response can not be influenced in any way and is always 200.

Only exception is, IG will return 500 if something went wrong when IG router called its node.
For example: Some http connection error that disrupted the connection.

Anything else you would like to add:
To make sure we do not release a breaking change, we can rollout this feature keeping every node, including last one, as a soft dependency by default.

Although I feel last should be always a hard dependency. If the last node returns non-200, IG should never return 200. But for the sake of keeping changes non-breaking, we can keep all nodes configurable.

Initial idea to implement:
We can add a field in IG spec to mark the node as a hard or soft dependency to decide whether to stop IG execution right away or not.

  • For nodes mentioned as hard dependency, IG's execution will stop if that node errors out
  • For nodes mentioned as soft dependency, IG's execution will not stop and continue
@rachitchauhan43
Copy link
Contributor Author

/assign @rachitchauhan43

@Iamlovingit
Copy link
Contributor

Hi, @rachitchauhan43 , great feature. I have a small question:
what do you think about Ensemble case? If add hard or soft to every branch node, when some nodes return bad status, and other nodes return 200, what should we do?

@rachitchauhan43
Copy link
Contributor Author

@Iamlovingit @yuzisun : This is the first proposal that I want to propose for this issue https://docs.google.com/document/d/1aZY7DfmESeEB5VmGeQPQoBDQS2p22_nMp0te7izoDcA/edit?usp=sharing

Would love to hear your thoughts on this.

@rachitchauhan43
Copy link
Contributor Author

rachitchauhan43 commented Nov 17, 2022

@Iamlovingit @yuzisun : I have updated the doc with the latest approach (see Proposal 1) https://docs.google.com/document/d/1aZY7DfmESeEB5VmGeQPQoBDQS2p22_nMp0te7izoDcA/edit#heading=h.ayq22ffbwvz8

@Iamlovingit : this is what we are proposing for Ensemble case: see here

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