feat(workflow-operator): constrain the Sklearn text column, and drop what an estimator cannot fit - #7645
Open
kz930 wants to merge 1 commit into
Open
feat(workflow-operator): constrain the Sklearn text column, and drop what an estimator cannot fit#7645kz930 wants to merge 1 commit into
kz930 wants to merge 1 commit into
Conversation
…what an estimator cannot fit Two configurations ended the run with an error from inside scikit-learn or from code generation, naming neither the column nor the field to change. The text column now states what it takes. Count Vectorizer tokenizes documents, so the column is a string, and it is required exactly when that switch is on: with the switch off nothing reads it, and with the switch on a blank one reached code generation as a null and became #EXCEPTION DURING CODE GENERATION. Both are schema constraints, the second in the conditional form Aggregate already uses, so the panel refuses the configuration while it is being written. Conditional rather than a plain required, so a freshly dropped operator, whose vectorizer is off, is not flagged for a field it has no use for. The feature set drops what it cannot fit rather than ending the run. These operators take every column but the target, so a text column beside the numbers, one the user never meant as a feature, raised ValueError: could not convert string to float, and a timestamp raised DTypePromotionError. Nothing in the configuration could exclude it and the message named neither the column nor a way out. Booleans are kept, fitting as 0/1. What was left out is printed, so the choice is visible rather than silent, and this follows the rest of the codebase, where twenty-four visualization operators drop missing values before plotting. The drop is skipped under the text pipeline, where X is one string column by construction and filtering would empty it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7645 +/- ##
============================================
- Coverage 89.65% 89.65% -0.01%
Complexity 4397 4397
============================================
Files 1177 1177
Lines 46996 46994 -2
Branches 5268 5266 -2
============================================
- Hits 42136 42134 -2
Misses 3094 3094
Partials 1766 1766
*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:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 409 | 0.249 | 22,017/35,101/35,101 us | 🔴 -15.0% / 🔴 +114.7% |
| 🟢 | bs=100 sw=10 sl=64 | 955 | 0.583 | 103,395/118,251/118,251 us | 🟢 -12.8% / 🔴 +9.9% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,093 | 0.667 | 906,607/1,000,983/1,000,983 us | 🔴 +6.8% / 🟢 -8.6% |
Baseline details
Latest main 12169c2 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 409 tuples/sec | 480 tuples/sec | 774.89 tuples/sec | -14.8% | -47.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.249 MB/s | 0.293 MB/s | 0.473 MB/s | -15.0% | -47.4% |
| bs=10 sw=10 sl=64 | p50 | 22,017 us | 20,334 us | 12,738 us | +8.3% | +72.8% |
| bs=10 sw=10 sl=64 | p95 | 35,101 us | 32,373 us | 16,348 us | +8.4% | +114.7% |
| bs=10 sw=10 sl=64 | p99 | 35,101 us | 32,373 us | 18,848 us | +8.4% | +86.2% |
| bs=100 sw=10 sl=64 | throughput | 955 tuples/sec | 973 tuples/sec | 1,005 tuples/sec | -1.8% | -4.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.583 MB/s | 0.594 MB/s | 0.613 MB/s | -1.9% | -4.9% |
| bs=100 sw=10 sl=64 | p50 | 103,395 us | 100,742 us | 100,970 us | +2.6% | +2.4% |
| bs=100 sw=10 sl=64 | p95 | 118,251 us | 135,608 us | 107,605 us | -12.8% | +9.9% |
| bs=100 sw=10 sl=64 | p99 | 118,251 us | 135,608 us | 116,429 us | -12.8% | +1.6% |
| bs=1000 sw=10 sl=64 | throughput | 1,093 tuples/sec | 1,120 tuples/sec | 1,030 tuples/sec | -2.4% | +6.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.667 MB/s | 0.684 MB/s | 0.629 MB/s | -2.5% | +6.1% |
| bs=1000 sw=10 sl=64 | p50 | 906,607 us | 891,133 us | 991,433 us | +1.7% | -8.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,000,983 us | 937,645 us | 1,036,668 us | +6.8% | -3.4% |
| bs=1000 sw=10 sl=64 | p99 | 1,000,983 us | 937,645 us | 1,070,470 us | +6.8% | -6.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,489.47,200,128000,409,0.249,22016.74,35100.53,35100.53
1,100,10,64,20,2094.98,2000,1280000,955,0.583,103395.03,118250.99,118250.99
2,1000,10,64,20,18291.88,20000,12800000,1093,0.667,906607.39,1000983.27,1000983.27
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Two configurations ended the run with an error from inside scikit-learn or from code generation, naming neither the column nor the field to change.
The text column now states what it takes.
Count Vectorizertokenizes documents, so the column is a string, and it is required exactly when that switch is on: with the switch off nothing reads it, and with the switch on a blank one reached code generation as a null and became#EXCEPTION DURING CODE GENERATION. Both are schema constraints, the second in the conditional form Aggregate already uses, so the panel refuses the configuration while it is being written. Conditional rather than a plainrequired, so a freshly dropped operator, whose vectorizer is off, is not flagged for a field it has no use for.The feature set drops what it cannot fit rather than ending the run. These operators take every column but the target, so a text column beside the numbers, one the user never meant as a feature, raised
ValueError: could not convert string to float, and a timestamp raisedDTypePromotionError. Nothing in the configuration could exclude it, and the message named neither the column nor a way out. Booleans are kept, fitting as 0/1. What was left out is printed, so the choice is visible rather than silent, and this follows the rest of the codebase, where twenty-four visualization operators drop missing values before plotting.The drop is skipped under the text pipeline, where X is one string column by construction and filtering would empty it.
The constraints sit on the shared base, so they reach all fifty-one operators of the Sklearn and Sklearn Training groups. The required marker for the text column needs #7642; without it the configuration is still refused, only unmarked.
Any related issues, documentation, discussions?
Closes #7643. The marker for the new conditional required comes from #7642.
How was this PR tested?
SklearnClassifierOpDescCodegenSpecandSklearnTrainingOpDescCodegenSpecalready pinned the generated feature-selection line and were updated to the new one, including that the text pipeline does not carry it.WorkflowOperator/testpasses: 2387 tests.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)