Copilot commented on code in PR #8617:
URL: https://github.com/apache/texera/pull/8617#discussion_r4060062822
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/huggingFace/codegen/QaRankingCodegenSpec.scala:
##########
@@ -117,7 +117,19 @@ class QaRankingCodegenSpec extends AnyFlatSpec with
Matchers {
out should include("""body.get("answer"""")
// #7195: chat-completions responses (third-party providers) are read from
// choices[0].message.content, not the native {"answer": ...} shape.
- out should include("""body["choices"][0]["message"]["content"]""")
+ out should include("""body["choices"][0].get("message", {}).get("content",
json.dumps(body))""")
+ }
+
+ it should "degrade instead of raising when a chat response is malformed
(#8486)" in {
+ // parsePython runs per row, so indexing straight into
+ // choices[0]["message"]["content"] turned one malformed provider response
+ // into an aborted run: an empty "choices" list raises IndexError and a
+ // choice missing "message"/"content" raises KeyError. All three chat
+ // extractions now use a truthiness guard plus .get chaining, matching the
+ // native shapes beside them, which already degrade via json.dumps(body).
+ val out = QaRankingCodegen.parsePython(makeCtx())
+ out should not include ("""["message"]["content"]""")
+ out.split("""body\.get\("choices"\)""").length - 1 shouldBe 3
Review Comment:
These assertions verify only that three source substrings changed, not the
claimed degradation behavior. They therefore miss the uncaught `AttributeError`
cases introduced by non-dict choices/messages. Run each generated QA/ranking
branch against valid and malformed nested response values and assert its actual
return value.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/TextGenCodegen.scala:
##########
@@ -50,5 +52,7 @@ object TextGenCodegen extends TaskCodegen {
override def parsePython(ctx: CodegenContext): String =
""" if task == "text-generation":
- | return
body["choices"][0]["message"]["content"]""".stripMargin
+ | if isinstance(body, dict) and body.get("choices"):
+ | return body["choices"][0].get("message",
{}).get("content", json.dumps(body))
+ | return json.dumps(body)""".stripMargin
Review Comment:
Malformed but valid JSON such as `{"choices":[null]}`,
`{"choices":[{"message":null}]}`, or a truthy non-list `choices` value still
reaches `.get` on a non-dictionary and raises `AttributeError`.
`_parse_response` does not convert that exception to raw JSON, so the outer row
handler stores a `Request failed` result instead. Validate each nested value
before extracting `content` so every malformed chat shape follows the promised
raw-JSON fallback.
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/huggingFace/codegen/TextGenCodegenSpec.scala:
##########
@@ -74,6 +74,18 @@ class TextGenCodegenSpec extends AnyFlatSpec with Matchers {
out should include("content")
}
+ it should "degrade instead of raising when a chat response is malformed
(#8486)" in {
+ // This extraction was fully unguarded: a non-dict body, an empty "choices"
+ // list, or a choice missing "message"/"content" raised, and since parsing
+ // runs per row that aborted the whole run over one bad response. It now
+ // falls back to the raw JSON body, as the other codegens do.
+ val out = TextGenCodegen.parsePython(makeCtx())
+ out should include("""if isinstance(body, dict) and
body.get("choices"):""")
+ out should include("""body["choices"][0].get("message", {}).get("content",
json.dumps(body))""")
+ out should include("return json.dumps(body)")
+ out should not include ("""["message"]["content"]""")
Review Comment:
This regression test only inspects generated source text; it never invokes
`_parse_response` with malformed data, so it passes even though `choices:
[null]` and `message: null` still raise `AttributeError`. Execute the generated
parser for valid, empty, missing, null, and wrong-type nested shapes and assert
that malformed inputs return the serialized body.
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/QaRankingCodegen.scala:
##########
@@ -79,18 +79,18 @@ object QaRankingCodegen extends TaskCodegen {
| if isinstance(body, dict):
| # Third-party chat providers answer via
choices[0].message;
| # hf-inference returns the native {"answer": ...}
shape.
- | if "choices" in body:
- | return body["choices"][0]["message"]["content"]
+ | if body.get("choices"):
+ | return body["choices"][0].get("message",
{}).get("content", json.dumps(body))
| return body.get("answer", json.dumps(body))
| return json.dumps(body)
| elif task == "table-question-answering":
| if isinstance(body, dict):
- | if "choices" in body:
- | return body["choices"][0]["message"]["content"]
+ | if body.get("choices"):
+ | return body["choices"][0].get("message",
{}).get("content", json.dumps(body))
| return body.get("answer", json.dumps(body))
| return json.dumps(body)
| elif task in ("zero-shot-classification",
"sentence-similarity", "text-ranking"):
- | if isinstance(body, dict) and "choices" in body:
- | return body["choices"][0]["message"]["content"]
+ | if isinstance(body, dict) and body.get("choices"):
+ | return body["choices"][0].get("message",
{}).get("content", json.dumps(body))
Review Comment:
All three chat extractions still assume that a truthy `choices` value is a
list whose first item and `message` are dictionaries. Inputs such as
`{"choices":[null]}`, `{"choices":[{"message":null}]}`, or `{"choices":"bad"}`
therefore raise `AttributeError`; the outer row handler stores `Request failed`
instead of the raw JSON promised by this change. Validate the nested types in
each branch before calling `.get`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
