Skip to content

feat(sklearn): refuse the label column as an input column - #8012

Open
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:feat/8010-label-not-an-input-column
Open

feat(sklearn): refuse the label column as an input column#8012
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:feat/8010-label-not-an-input-column

Conversation

@kz930

@kz930 kz930 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Two families of sklearn operators let the column already chosen as the label be chosen a second time as an input column, and neither said anything while the workflow was being written.

Target Attribute names the label and Text Attribute names the columns Count Vectorizer tokenizes, on the fifty-one operators of the Sklearn and Sklearn Training groups. The generated code drops the target before the text pipeline reads its columns, so naming it there asks the pipeline for a column that is no longer present and the run ends with ValueError: A given column is not a column of the dataframe, which names neither field. Failing is right, since vectorizing the label would train a model on the answer, so the pairing is refused at compile time instead, in the same getOutputSchemas that already refuses Count Vectorizer on the two Gaussian Naive Bayes operators. Only while the vectorizer is on: with it off nothing reads text and the panel hides it, so a value left behind by an earlier configuration must not report the operator invalid.

Ground Truth Attribute Column names the label and Selected Features names the columns handed to fit, on the four advanced trainers. Here nothing failed at all. The features go to fit as they are named, so the ground truth among them is the answer given to the estimator as an input, and the model scores far better than what it learned deserves. On two hundred rows whose label is random noise, a KNN classifier reaches 0.685 train accuracy on the two real features and 0.995 once the label is included, and nothing in the run says why. selectedFeatures defaults to null, so the check reads it through an Option rather than assuming a list is there.

Both messages name the column and both fields, and say what to change.

Any related issues, documentation, discussions?

Closes #8010.

How was this PR tested?

SklearnModelOpDescSpec gains three cases: the target named as a text column is refused, text columns that are not the target are let through, and a stale text column is left alone while Count Vectorizer is off. SklearnMLOperatorDescriptorSpec gains two: the ground truth named as a feature is refused, and features that do not include it are let through. Both files already owned the assertions on the getOutputSchemas these checks live in. WorkflowOperator/test passes: 2381 tests.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

The Target Attribute could also be named as a Text Attribute, and the
Ground Truth Attribute Column as a Selected Feature. The first ends the
run from inside scikit-learn naming neither field, and the second does
not fail at all: the estimator is handed the answer as an input. Both
are refused while the workflow is being written, beside the Count
Vectorizer refusal the shared base already carries.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @carloea2, @aglinxinyuan
    You can notify them by mentioning @carloea2, @aglinxinyuan in a comment.

@codecov-commenter

codecov-commenter commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.29%. Comparing base (9fe0657) to head (9582064).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8012      +/-   ##
============================================
- Coverage     93.29%   93.29%   -0.01%     
+ Complexity     4678     4677       -1     
============================================
  Files          1179     1179              
  Lines         47713    47722       +9     
  Branches       5314     5316       +2     
============================================
+ Hits          44515    44520       +5     
- Misses         1732     1734       +2     
- Partials       1466     1468       +2     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø)
agent-service 99.32% <ø> (ø) Carriedforward from 9fe0657
amber 89.40% <100.00%> (-0.02%) ⬇️
computing-unit-managing-service 73.67% <ø> (ø)
config-service 86.73% <ø> (ø)
file-service 86.70% <ø> (ø)
frontend 95.77% <ø> (ø) Carriedforward from 9fe0657
notebook-migration-service 79.31% <ø> (ø)
pyamber 97.78% <ø> (ø) Carriedforward from 9fe0657
workflow-compiling-service 77.19% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 6 better · 🔴 1 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 9fe0657 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🟢 bs=10 sw=10 sl=64 656 0.401 14,852/17,482/17,482 us 🟢 -27.5% / 🔴 +33.3%
🔴 bs=100 sw=10 sl=64 1,384 0.844 72,990/79,785/79,785 us 🟢 -23.8% / 🟢 -21.9%
🟢 bs=1000 sw=10 sl=64 1,552 0.947 641,148/694,608/694,608 us 🟢 -6.2% / 🟢 +29.5%
Baseline details

Latest main 9fe0657 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 656 tuples/sec 643 tuples/sec 894.86 tuples/sec +2.0% -26.7%
bs=10 sw=10 sl=64 MB/s 0.401 MB/s 0.392 MB/s 0.546 MB/s +2.3% -26.6%
bs=10 sw=10 sl=64 p50 14,852 us 14,519 us 11,139 us +2.3% +33.3%
bs=10 sw=10 sl=64 p95 17,482 us 24,118 us 13,843 us -27.5% +26.3%
bs=10 sw=10 sl=64 p99 17,482 us 24,118 us 16,839 us -27.5% +3.8%
bs=100 sw=10 sl=64 throughput 1,384 tuples/sec 1,376 tuples/sec 1,166 tuples/sec +0.6% +18.7%
bs=100 sw=10 sl=64 MB/s 0.844 MB/s 0.84 MB/s 0.711 MB/s +0.5% +18.6%
bs=100 sw=10 sl=64 p50 72,990 us 69,437 us 87,509 us +5.1% -16.6%
bs=100 sw=10 sl=64 p95 79,785 us 104,660 us 93,675 us -23.8% -14.8%
bs=100 sw=10 sl=64 p99 79,785 us 104,660 us 102,153 us -23.8% -21.9%
bs=1000 sw=10 sl=64 throughput 1,552 tuples/sec 1,564 tuples/sec 1,198 tuples/sec -0.8% +29.5%
bs=1000 sw=10 sl=64 MB/s 0.947 MB/s 0.955 MB/s 0.731 MB/s -0.8% +29.5%
bs=1000 sw=10 sl=64 p50 641,148 us 635,283 us 859,766 us +0.9% -25.4%
bs=1000 sw=10 sl=64 p95 694,608 us 740,491 us 902,897 us -6.2% -23.1%
bs=1000 sw=10 sl=64 p99 694,608 us 740,491 us 937,957 us -6.2% -25.9%
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,304.79,200,128000,656,0.401,14851.65,17481.86,17481.86
1,100,10,64,20,1445.54,2000,1280000,1384,0.844,72989.51,79785.31,79785.31
2,1000,10,64,20,12883.49,20000,12800000,1552,0.947,641147.52,694608.26,694608.26

@kz930

kz930 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @carloea2 @aglinxinyuan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The label column can also be chosen as an input column, and nothing says so

2 participants