Conversation
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
|
/request-review @Ma77Ball |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8617 +/- ##
============================================
- Coverage 92.75% 92.66% -0.09%
- Complexity 4923 4977 +54
============================================
Files 1244 1244
Lines 52525 52997 +472
Branches 6473 6582 +109
============================================
+ Hits 48718 49110 +392
- Misses 2203 2261 +58
- Partials 1604 1626 +22
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 385 | 0.235 | 24,950/32,822/32,822 us | 🔴 +16.3% / 🔴 +110.8% |
| 🟢 | bs=100 sw=10 sl=64 | 824 | 0.503 | 122,045/133,393/133,393 us | 🟢 -13.1% / 🔴 +23.2% |
| ⚪ | bs=1000 sw=10 sl=64 | 929 | 0.567 | 1,081,529/1,125,642/1,125,642 us | ⚪ within ±5% / 🔴 -9.2% |
Baseline details
Latest main 6defa2c from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 385 tuples/sec | 444 tuples/sec | 775.4 tuples/sec | -13.3% | -50.3% |
| bs=10 sw=10 sl=64 | MB/s | 0.235 MB/s | 0.271 MB/s | 0.473 MB/s | -13.3% | -50.3% |
| bs=10 sw=10 sl=64 | p50 | 24,950 us | 21,858 us | 12,716 us | +14.1% | +96.2% |
| bs=10 sw=10 sl=64 | p95 | 32,822 us | 28,234 us | 15,572 us | +16.3% | +110.8% |
| bs=10 sw=10 sl=64 | p99 | 32,822 us | 28,234 us | 19,527 us | +16.3% | +68.1% |
| bs=100 sw=10 sl=64 | throughput | 824 tuples/sec | 811 tuples/sec | 992.44 tuples/sec | +1.6% | -17.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.503 MB/s | 0.495 MB/s | 0.606 MB/s | +1.6% | -17.0% |
| bs=100 sw=10 sl=64 | p50 | 122,045 us | 119,515 us | 102,140 us | +2.1% | +19.5% |
| bs=100 sw=10 sl=64 | p95 | 133,393 us | 153,453 us | 108,282 us | -13.1% | +23.2% |
| bs=100 sw=10 sl=64 | p99 | 133,393 us | 153,453 us | 118,936 us | -13.1% | +12.2% |
| bs=1000 sw=10 sl=64 | throughput | 929 tuples/sec | 937 tuples/sec | 1,023 tuples/sec | -0.9% | -9.2% |
| bs=1000 sw=10 sl=64 | MB/s | 0.567 MB/s | 0.572 MB/s | 0.624 MB/s | -0.9% | -9.2% |
| bs=1000 sw=10 sl=64 | p50 | 1,081,529 us | 1,072,273 us | 998,263 us | +0.9% | +8.3% |
| bs=1000 sw=10 sl=64 | p95 | 1,125,642 us | 1,092,063 us | 1,036,365 us | +3.1% | +8.6% |
| bs=1000 sw=10 sl=64 | p99 | 1,125,642 us | 1,092,063 us | 1,064,941 us | +3.1% | +5.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,519.91,200,128000,385,0.235,24950.23,32822.41,32822.41
1,100,10,64,20,2426.75,2000,1280000,824,0.503,122045.20,133393.01,133393.01
2,1000,10,64,20,21535.67,20000,12800000,929,0.567,1081528.80,1125642.04,1125642.04
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Nested malformed values can still produce request errors instead of the promised raw-JSON fallback.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (4)
What changed in this PR
Improves Hugging Face code-generated parsers so malformed chat responses fall back to raw JSON.
Changes:
- Guards chat response extraction in text-generation and QA/ranking tasks.
- Updates generated-code assertions and regression tests.
| File | Description |
|---|---|
| common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/huggingFace/HuggingFaceInferenceOpDescSpec.scala | Updates generated-parser assertions. |
| common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/huggingFace/codegen/TextGenCodegenSpec.scala | Adds text-generation degradation coverage. |
| common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/huggingFace/codegen/QaRankingCodegenSpec.scala | Adds QA/ranking degradation coverage. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/TextGenCodegen.scala | Adds guarded text response parsing. |
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/huggingFace/codegen/QaRankingCodegen.scala | Adds guarded QA/ranking response parsing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | 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)) |
There was a problem hiding this comment.
Confirmed, and there were two more: {"choices": {...}} raises KeyError: 0 and {"choices": [42]} raises AttributeError. Two shapes were also silently wrong rather than raising a null content returned Python None into the cell, and a list-valued content returned the raw list.
Worth flagging what your comment uncovered: _parse_response already catches (KeyError, IndexError, TypeError), so on main these shapes were degrading to raw JSON correctly. Moving to .get chaining changed the exception type to AttributeError, which that handler doesn't catch, so this PR was introducing the escape, not just failing to fix it. Good catch.
Rather than repeat type checks at each site, I added a shared _chat_message_content(body) helper to HuggingFaceCodegenBase that validates every level and returns None when the body isn't a readable chat response. All four extractions here and in TextGenCodegen go through it.
| | if isinstance(body, dict) and body.get("choices"): | ||
| | return body["choices"][0].get("message", {}).get("content", json.dumps(body)) | ||
| | return json.dumps(body)""".stripMargin |
There was a problem hiding this comment.
Fixed by the same shared helper: TextGenCodegen.parsePython now just calls self._chat_message_content(body) and falls back to json.dumps(body).
One deliberate difference from the suggested patch: it returns content whenever it isn't None, so {"content": 42} would put 42 in the result column and a list-valued content would put a raw list there. The helper requires a string, and joins list-of-parts content ([{"type": "text", "text": ...}]) into text, which is a shape some OpenAI-compatible providers return.
| val out = QaRankingCodegen.parsePython(makeCtx()) | ||
| out should not include ("""["message"]["content"]""") | ||
| out.split("""body\.get\("choices"\)""").length - 1 shouldBe 3 |
There was a problem hiding this comment.
Agreed that these assert that the source text changed, not that behavior did. Added HuggingFaceParseBehaviorSpec, which renders the operator, lifts _parse_response and its helpers out of the generated source, and runs them under a real interpreter against 13 response shapes: valid, empty, missing, null and wrong-typed, asserting the actual return value for every task in this PR. The contract it pins: always returns a string, never raises, never None.
It has teeth: run against this PR's previous commit, 2 of its 4 tests fail on exactly the AttributeError cases you identified.
| 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"]""") |
There was a problem hiding this comment.
Same fix, HuggingFaceParseBehaviorSpec now executes the generated parser rather than inspecting its text, covering text-generation and all five QA/ranking tasks across valid, empty, missing, null and wrong-typed nested shapes, and asserting malformed input returns the serialized body.
It follows the existing PythonCodeRawInvalidTextSpec pattern for locating an interpreter, and cancels rather than fails where none is available. ImageTaskCodegen has the same extractions from #7920 and will be routed through the helper and added to this spec in a small follow-up PR, so that change arrives with its own failing-first test.
What changes were proposed in this PR?
The text-generation and QA/ranking codegens read a chat provider's reply by indexing straight into it:
That assumes the response is a dict, that
choicesis non-empty, and that the first choice carriesmessage.content. When a provider returns a 200 with anything else, the indexing raises, and since the parse step runs once per row, a single malformed response aborts the entire run.TextGenCodegenwas the most exposed: it had noisinstancecheck either, so even a plain list body raised.All four extractions (one in
TextGenCodegen, three inQaRankingCodegen) now guard on a truthychoicesand use.getchaining, falling back to the raw JSON body. That is the same idiomImageTaskCodegenadopted in #7920, and it matches how the nativehf-inferenceshapes beside it already degrade viabody.get("answer", json.dumps(body)). A bad response now lands in the result column as raw JSON instead of killing the run.Any related issues?
Closes #8486
How was this PR tested?
145 tests pass in the Hugging Face suites,
PythonCodeRawInvalidTextSpecpy-compiles the generated Python for all 117 operators, andscalafmtCheckandscalafix --checkare clean. Added a degradation test toTextGenCodegenSpecandQaRankingCodegenSpec, and updated the assertions inHuggingFaceInferenceOpDescSpecthat pinned the old form.The emitted Python was also run directly against the failing shapes, comparing this branch with main across 33 task/response combinations: 16 of them raised before (
IndexErroron an emptychoiceslist,KeyErroron a missingmessage/content,TypeErroron a list body) and none raise now. Well-formed chat responses and nativehf-inferenceresponses parse identically to before.Was this PR authored or co-authored using generative AI tooling?
Yes, this PR was co-authored with Claude in compliance with ASF policy.