feat: improve output rails error handling for SSE format - #1058
Conversation
|
@mikemckiernan I made the necessary change to the docs, please feel free to change it as you wish (you can open a separate MR if you prefer and I'll delete the doc commit), Thanks! |
Documentation preview |
14df903 to
d9e0c6e
Compare
tgasser-nv
left a comment
tgasser-nv
left a comment
There was a problem hiding this comment.
Approved, please take a look and address the comments before shipping
| MAX_ERROR_MESSAGE_SIZE = 400 | ||
| MAX_JSON_SIZE = 500 | ||
| MAX_PARSING_DEPTH = 2 |
There was a problem hiding this comment.
Curious where these configs came from? How do we know this is sufficient to capture all errors we could get back?
There was a problem hiding this comment.
some random (reasonable) guess, like I don't expect langchain providers to generate error messages that are too nested or larger. Do you have any suggestion?
| if len(error_message) > MAX_ERROR_MESSAGE_SIZE: | ||
| error_message = error_message[:MAX_ERROR_MESSAGE_SIZE] + "... (truncated)" | ||
|
|
||
| # OpenAI error format typically has "Error code: XXX - {...}" format |
There was a problem hiding this comment.
This is fine for now, but we need a more formal specification of the OpenAI spec that we put in one file and test against. Ideally this would be vended by OpenAI themselves, but we can use observed behaviour from their prod LLMs and/or their Python client to explicitly modify the behavioiur
Could you add a backlog item to come back to this (maybe "Consolidate OpenAI Interface Spec"?)
|
|
||
| # looking for suspicious patterns | ||
| # is it ok? | ||
| if re.search(r"__[\w]+__", json_part): |
There was a problem hiding this comment.
Could you get some help from Security to define what unsafe content is here? I'm concerned we pass this into ast.literal_eval() and could potentially run unsafe code from a man-in-the-middle type attack
There was a problem hiding this comment.
the error_message should come from an error message that a langchain provider generates, for MS we use our own client so we should be safe, in the toolkit should we ensure this or is it on user's ? Also Github Code QL is not complaining,I think @erickgalinkin can help here.
| expected_output = { | ||
| "event": "ABORT", | ||
| "data": {"reason": "Blocked by self check output rails."}, | ||
| expected_error = { |
There was a problem hiding this comment.
We have a quite a few "stringly typed" errors in the codebase. Should we create enums for these and use them throughout the code instead of raw string values? It looks like the type, param and code could all be enums. I suspect the message comes from Colang (is there a way to programmatically pull out the response message from a colang file)?
There was a problem hiding this comment.
The message comes from langchain via LLMException. But I'll maked it typed 👍🏻
The update looks good. TY for making it and merging. |
3 participants
Description
resolves #1032
Related PRs
#1012
Test plan
see nmp#745
TODO: