fix(workflow-operator): validate the spam operator's result attributes like its siblings - #8483
fix(workflow-operator): validate the spam operator's result attributes like its siblings#8483PG1204 wants to merge 2 commits into
Conversation
…s like its siblings
|
/request-review @Ma77Ball this is a small fix unrelated to HuggingFaceInferenceOpDesc, this fix is for the HuggingFaceSpamSMSDetectionOpDesc op. |
Yicong-Huang
added
release/v1.3
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 #8483 +/- ##
============================================
+ Coverage 93.69% 93.93% +0.24%
- Complexity 4825 4831 +6
============================================
Files 1209 1203 -6
Lines 49871 48990 -881
Branches 6099 5948 -151
============================================
- Hits 46726 46019 -707
+ Misses 1652 1505 -147
+ Partials 1493 1466 -27
*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 | 538 | 0.328 | 17,058/27,859/27,859 us | 🔴 +13.8% / 🔴 +72.7% |
| ⚪ | bs=100 sw=10 sl=64 | 1,178 | 0.719 | 81,986/108,291/108,291 us | ⚪ within ±5% / 🟢 +20.2% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,334 | 0.814 | 747,790/785,623/785,623 us | ⚪ within ±5% / 🟢 +31.9% |
Baseline details
Latest main 0e549fa from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 538 tuples/sec | 608 tuples/sec | 755.28 tuples/sec | -11.5% | -28.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.328 MB/s | 0.371 MB/s | 0.461 MB/s | -11.6% | -28.8% |
| bs=10 sw=10 sl=64 | p50 | 17,058 us | 16,408 us | 12,957 us | +4.0% | +31.7% |
| bs=10 sw=10 sl=64 | p95 | 27,859 us | 24,490 us | 16,134 us | +13.8% | +72.7% |
| bs=10 sw=10 sl=64 | p99 | 27,859 us | 24,490 us | 20,333 us | +13.8% | +37.0% |
| bs=100 sw=10 sl=64 | throughput | 1,178 tuples/sec | 1,157 tuples/sec | 980.1 tuples/sec | +1.8% | +20.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.719 MB/s | 0.706 MB/s | 0.598 MB/s | +1.8% | +20.2% |
| bs=100 sw=10 sl=64 | p50 | 81,986 us | 83,557 us | 101,894 us | -1.9% | -19.5% |
| bs=100 sw=10 sl=64 | p95 | 108,291 us | 109,138 us | 108,718 us | -0.8% | -0.4% |
| bs=100 sw=10 sl=64 | p99 | 108,291 us | 109,138 us | 122,482 us | -0.8% | -11.6% |
| bs=1000 sw=10 sl=64 | throughput | 1,334 tuples/sec | 1,349 tuples/sec | 1,011 tuples/sec | -1.1% | +31.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.814 MB/s | 0.823 MB/s | 0.617 MB/s | -1.1% | +31.9% |
| bs=1000 sw=10 sl=64 | p50 | 747,790 us | 743,892 us | 996,422 us | +0.5% | -25.0% |
| bs=1000 sw=10 sl=64 | p95 | 785,623 us | 781,560 us | 1,037,670 us | +0.5% | -24.3% |
| bs=1000 sw=10 sl=64 | p99 | 785,623 us | 781,560 us | 1,072,152 us | +0.5% | -26.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,371.64,200,128000,538,0.328,17057.96,27859.34,27859.34
1,100,10,64,20,1697.47,2000,1280000,1178,0.719,81985.84,108291.19,108291.19
2,1000,10,64,20,14993.35,20000,12800000,1334,0.814,747790.09,785623.05,785623.05
mengw15
removed
release/v1.2
5 participants
What changes were proposed in this PR?
HuggingFaceSpamSMSDetectionOpDesc.getOutputSchemaswas the only one of the four legacy Hugging Face operators that neither validated its result-attribute names nor keyed the input schema by port id.getOutputSchemasis called as the user configures an operator, so it needs an answer for "not filled in yet".HuggingFaceSentimentAnalysisOpDescandHuggingFaceIrisLogisticRegressionOpDescanswer by returningnull; this operator instead passed the unset name straight intoSchema.add. It now returnsnullwhen eitherresultAttributeSpamorresultAttributeProbabilityis null or blank, matching its siblings.The input schema is also now read as
inputSchemas(operatorInfo.inputPorts.head.id)rather thaninputSchemas.values.head, consistent with the sibling operators. With a single input port these are equivalent, so this is a consistency change rather than a behavioral fix.Unifying the error contract across all four legacy operators, two return
null, one throws, this one did neither — is a broader question and is not attempted here.Any related issues?
Closes #8482
How was this PR tested?
347 tests pass across the
huggingFaceand operator-metadata suites, andscalafmtCheckis clean for main and test sources. Two tests were added toHuggingFaceSpamSMSDetectionOpDescSpeccovering an unset and a blank name for both result attributes. Reverting the operator change and re-running makes exactly those two tests fail, confirming they exercise the fix; the existing happy-path test already keys the input schema by the declared input port, so it covers the lookup change.Was this PR authored or co-authored using generative AI tooling?
Yes, this PR was co-authored with Claude in compliance with ASF policy.