fix(workflow-operator): validate zero-shot-image-classification labels before running - #7585
fix(workflow-operator): validate zero-shot-image-classification labels before running#7585PG1204 wants to merge 3 commits into
Conversation
|
/request-review @xuang7 |
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7585 +/- ##
=========================================
Coverage 89.65% 89.66%
- Complexity 4397 4401 +4
=========================================
Files 1177 1177
Lines 46996 46990 -6
Branches 5268 5268
=========================================
- Hits 42136 42135 -1
Misses 3094 3094
+ Partials 1766 1761 -5
*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 | 366 | 0.223 | 25,411/42,565/42,565 us | 🔴 +17.3% / 🔴 +160.4% |
| 🟢 | bs=100 sw=10 sl=64 | 808 | 0.493 | 122,348/135,227/135,227 us | 🟢 -15.2% / 🔴 +25.7% |
| 🔴 | bs=1000 sw=10 sl=64 | 929 | 0.567 | 1,066,366/1,204,723/1,204,723 us | 🔴 +5.8% / 🔴 +16.2% |
Baseline details
Latest main 12169c2 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 366 tuples/sec | 436 tuples/sec | 774.89 tuples/sec | -16.1% | -52.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.223 MB/s | 0.266 MB/s | 0.473 MB/s | -16.2% | -52.8% |
| bs=10 sw=10 sl=64 | p50 | 25,411 us | 21,655 us | 12,738 us | +17.3% | +99.5% |
| bs=10 sw=10 sl=64 | p95 | 42,565 us | 36,855 us | 16,348 us | +15.5% | +160.4% |
| bs=10 sw=10 sl=64 | p99 | 42,565 us | 36,855 us | 18,848 us | +15.5% | +125.8% |
| bs=100 sw=10 sl=64 | throughput | 808 tuples/sec | 804 tuples/sec | 1,005 tuples/sec | +0.5% | -19.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.493 MB/s | 0.491 MB/s | 0.613 MB/s | +0.4% | -19.6% |
| bs=100 sw=10 sl=64 | p50 | 122,348 us | 119,228 us | 100,970 us | +2.6% | +21.2% |
| bs=100 sw=10 sl=64 | p95 | 135,227 us | 159,416 us | 107,605 us | -15.2% | +25.7% |
| bs=100 sw=10 sl=64 | p99 | 135,227 us | 159,416 us | 116,429 us | -15.2% | +16.1% |
| bs=1000 sw=10 sl=64 | throughput | 929 tuples/sec | 928 tuples/sec | 1,030 tuples/sec | +0.1% | -9.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.567 MB/s | 0.566 MB/s | 0.629 MB/s | +0.2% | -9.8% |
| bs=1000 sw=10 sl=64 | p50 | 1,066,366 us | 1,071,717 us | 991,433 us | -0.5% | +7.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,204,723 us | 1,138,683 us | 1,036,668 us | +5.8% | +16.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,204,723 us | 1,138,683 us | 1,070,470 us | +5.8% | +12.5% |
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,546.88,200,128000,366,0.223,25411.09,42565.07,42565.07
1,100,10,64,20,2475.64,2000,1280000,808,0.493,122348.44,135226.63,135226.63
2,1000,10,64,20,21525.19,20000,12800000,929,0.567,1066366.15,1204723.11,1204723.11
xuang7
left a comment
There was a problem hiding this comment.
LGTM. One thing worth a quick ack since it goes beyond what #7199 asked for: this also drops the prompt-column fallback for candidate labels, so any workflow still supplying labels through the prompt column will now fail with the config error instead. Given the fallback was a pre-#5574 transition shim and the error message points users to the right field, this seems fine.
@xuang7 thanks! yep, deliberate. It makes zero-shot-image-classification consistent with text zero-shot-classification (labels come only from the candidate labels field), and the pre-loop check turns any prompt-column usage into a clear error instead of silently-wrong results. Agreed that it's fine to drop. |
What changes were proposed in this PR?
When the
zero-shot-image-classificationtask ran without at least 2 candidate labels, the operator raised the error from inside the per-row loop, so it crashed mid-run instead of failing cleanly. This moves the check up front, alongside the operator's other config validations, so it fails fast with a clear message before any rows are processed.Labels now come from the Candidate Labels field only (matching the text
zero-shot-classificationtask); the old fallback that read labels from the prompt column is removed.Any related issues?
Closes #7199 (the Part B follow-up to #7297, which addressed the same issue).
How was this PR tested?
Unit tests + the generated-Python compile check, plus a quick headless run of the generated operator: with 0 labels it now raises a clear "requires at least 2 Candidate Labels" error before the loop; with 2+ labels it proceeds normally.
Was this PR authored or co-authored using generative AI tooling?
Yes, this PR was co-authored with Claude in compliance with ASF policy.