feat(sklearn): skip rows with missing values instead of ending the run - #7583
Open
kz930 wants to merge 1 commit into
Open
feat(sklearn): skip rows with missing values instead of ending the run#7583kz930 wants to merge 1 commit into
kz930 wants to merge 1 commit into
Conversation
Every Sklearn operator ended the execution when a cell it reads was empty, with an error raised by scikit-learn's own input validation rather than by the operator: ValueError: Input X contains NaN. Nothing in the family looked at missing values, so the message named X, a variable inside generated code, and pointed at neither the column nor the row. An empty value is ordinary input here. A blank CSV cell arrives as null, since univocity returns null for an empty field and AttributeTypeUtils.parseField passes it through by design. Skipping is what the rest of the codebase does with a value that is not there: twenty-four visualization operators open their generated Python with dropna, COUNT counts only non-null rows, CONCAT and MIN pass over them, and FilterPredicate answers false for every condition but IS_NULL / IS_NOT_NULL. This family was the only one with no answer at all. What skipping means follows what each operator emits. The training, classifier and testing operators produce a model, or scores over one, so a dropped row costs nothing the user can see. Training and classifier read every column but the target, so they drop on the whole table; with Count Vectorizer on they read only the text and the target and drop on those two, since a blank in a column they never touch should not cost the row. The advanced trainers read a named list of features and drop on that list plus the ground truth. Prediction is the exception. It adds a result column to each input row, so dropping would take the user's row out of the output along with the value the model had nothing to say about. It keeps the row and leaves the result empty, which is what the Hugging Face inference operators already do with a row they cannot process. Fitting on fewer rows changes the model, the way COUNT over a column with nulls changes the count. That trade is one this codebase has already made everywhere else, and it beats ending the run. Closes apache#7582 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7583 +/- ##
=========================================
Coverage 87.87% 87.87%
- Complexity 4275 4277 +2
=========================================
Files 1176 1176
Lines 46890 46897 +7
Branches 5228 5230 +2
=========================================
+ Hits 41205 41212 +7
Misses 3961 3961
Partials 1724 1724
*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 | 372 | 0.227 | 26,601/33,399/33,399 us | 🔴 +24.3% / 🔴 +111.3% |
| 🔴 | bs=100 sw=10 sl=64 | 774 | 0.472 | 126,620/167,794/167,794 us | 🔴 +9.7% / 🔴 +55.3% |
| ⚪ | bs=1000 sw=10 sl=64 | 905 | 0.553 | 1,108,295/1,170,084/1,170,084 us | ⚪ within ±5% / 🔴 +12.7% |
Baseline details
Latest main cb6e5c6 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 372 tuples/sec | 417 tuples/sec | 784.3 tuples/sec | -10.8% | -52.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.227 MB/s | 0.254 MB/s | 0.479 MB/s | -10.6% | -52.6% |
| bs=10 sw=10 sl=64 | p50 | 26,601 us | 21,393 us | 12,590 us | +24.3% | +111.3% |
| bs=10 sw=10 sl=64 | p95 | 33,399 us | 35,349 us | 15,991 us | -5.5% | +108.9% |
| bs=10 sw=10 sl=64 | p99 | 33,399 us | 35,349 us | 18,694 us | -5.5% | +78.7% |
| bs=100 sw=10 sl=64 | throughput | 774 tuples/sec | 816 tuples/sec | 1,002 tuples/sec | -5.1% | -22.8% |
| bs=100 sw=10 sl=64 | MB/s | 0.472 MB/s | 0.498 MB/s | 0.612 MB/s | -5.2% | -22.8% |
| bs=100 sw=10 sl=64 | p50 | 126,620 us | 120,582 us | 101,285 us | +5.0% | +25.0% |
| bs=100 sw=10 sl=64 | p95 | 167,794 us | 152,935 us | 108,068 us | +9.7% | +55.3% |
| bs=100 sw=10 sl=64 | p99 | 167,794 us | 152,935 us | 118,235 us | +9.7% | +41.9% |
| bs=1000 sw=10 sl=64 | throughput | 905 tuples/sec | 918 tuples/sec | 1,030 tuples/sec | -1.4% | -12.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.553 MB/s | 0.56 MB/s | 0.629 MB/s | -1.3% | -12.0% |
| bs=1000 sw=10 sl=64 | p50 | 1,108,295 us | 1,090,152 us | 991,882 us | +1.7% | +11.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,170,084 us | 1,151,417 us | 1,038,496 us | +1.6% | +12.7% |
| bs=1000 sw=10 sl=64 | p99 | 1,170,084 us | 1,151,417 us | 1,068,265 us | +1.6% | +9.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,538.16,200,128000,372,0.227,26600.94,33398.87,33398.87
1,100,10,64,20,2584.91,2000,1280000,774,0.472,126619.65,167793.75,167793.75
2,1000,10,64,20,22093.25,20000,12800000,905,0.553,1108295.44,1170083.88,1170083.88
Contributor
Author
|
@aglinxinyuan @carloea2 May you take a look at it? |
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
3 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?
Every Sklearn operator ended the execution when a cell it reads was empty, with
ValueError: Input X contains NaN.raised by scikit-learn's own input validation rather than by the operator. This skips those rows instead, which is what the rest of the codebase already does with a value that is not there.What skipping means follows what each operator emits.
SklearnTrainingOpDescsubclassesSklearnClassifierOpDescsubclassesSklearnTestingOpDescSklearnAdvancedBaseDescsubclassesSklearnPredictionOpDescThe last row differs deliberately. Prediction adds a column to the user's rows, so dropping would take the user's row out of the output along with the value the model had nothing to say about. That is what the Hugging Face inference operators already do with a row they cannot process.
Two of the drops name their columns rather than taking the whole table. With Count Vectorizer on, training and classifier read only the text and the target, and the advanced trainers read a named list of features, so a blank in a column they never touch should not cost the row.
Fitting on fewer rows changes the model, the way COUNT over a column with nulls changes the count. That trade is one this codebase has already made everywhere else, and it beats ending the run.
Any related issues, documentation, discussions?
Closes #7582
How was this PR tested?
Each changed operator gained a case in its existing spec asserting the drop appears in the generated Python, next to the cases already asserting on that output. The full
WorkflowOperatorsuite passes, 2192 tests.The behaviour itself was checked against the reproduction in the issue, a four-row CSV with one blank cell run through
CSV File ScanintoTraining: Bernoulli Naive Bayes, which ended the execution before this change.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)