Skip to content

feat(operator): refuse two hyperparameter rows that set one parameter - #7982

Open
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:feat/7952-distinct-hyperparameter-rows
Open

feat(operator): refuse two hyperparameter rows that set one parameter#7982
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:feat/7952-distinct-hyperparameter-rows

Conversation

@kz930

@kz930 kz930 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

An Advanced trainer emits one keyword argument per row of its Parameter Setting table. Nothing stopped two rows from naming the same parameter, and two rows naming one parameter emitted that keyword twice, which Python rejects while it compiles the operator, before any of it runs. What the user saw was a SyntaxError pointing at a line of generated code rather than at the row they had added.

The row is now refused in two places. In the form, a row whose parameter is already set by another row is marked invalid and says so under the dropdown, alongside the validators the property editor already builds from the schema. The constraint is declared on the field itself, as a uniqueAmongRows key the operator's schema carries, rather than by naming the operator in the frontend, so any repeated section that needs the same rule can ask for it. uniqueItems is not the constraint that was missing: two rows naming one parameter differ in their other fields, so they are distinct items while still colliding on the keyword.

The descriptor refuses the same thing while the workflow compiles. That is what names the operator and the parameter in the error, and it also covers a workflow submitted through the API, which never passes through the form.

Any related issues, documentation, discussions?

Closes #7952

How was this PR tested?

New unit tests on both sides. On the descriptor, two rows naming one parameter must be refused with the parameter named, including the mixed case where one row reads a value and the other reads a column, and two rows naming different parameters must still emit both. On the frontend, the property editor must attach the validator to a field the schema marks unique, reject a value another row already holds, accept one no row holds, and produce the message shown to the user.

The three sklearnAdvanced.base Scala specs pass (17 tests), as does the wider machineLearning package (54 tests), with scalafmtCheckAll and scalafix clean. operator-property-edit-frame.component.spec.ts passes at 237 tests.

I also dumped the generated JSON schema for the SVC trainer to confirm the new key lands on the parameter property next to its enum of parameter names, which is what the frontend reads.

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

Generated-by: Claude Code (Claude Opus 5)

An Advanced trainer emits one keyword argument per parameter row, so two rows
naming one parameter emitted that keyword twice and Python rejected the operator
with a repeated-keyword SyntaxError before any of it ran. The user was shown a
line of generated code rather than the row they added.

The form now warns on the offending row, driven by a uniqueAmongRows key the
schema puts on the field rather than by an operator name in the frontend, since
uniqueItems cannot say this: two rows naming one parameter differ in their other
fields, so they are distinct items while still colliding on the keyword. The
descriptor refuses the same thing while the workflow compiles, which names the
operator and the parameter and also covers a workflow submitted through the API.

Closes apache#7952

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added feature frontend Changes related to the frontend GUI common labels Aug 26, 2026
@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, @Ma77Ball, @mengw15
    You can notify them by mentioning @carloea2, @Ma77Ball, @mengw15 in a comment.

@codecov-commenter

codecov-commenter commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.14%. Comparing base (7fbf64e) to head (496adf4).

Files with missing lines Patch % Lines
...it-frame/operator-property-edit-frame.component.ts 75.00% 1 Missing and 1 partial ⚠️
...sklearnAdvanced/base/SklearnAdvancedBaseDesc.scala 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7982      +/-   ##
============================================
- Coverage     93.14%   93.14%   -0.01%     
- Complexity     4654     4655       +1     
============================================
  Files          1174     1174              
  Lines         47580    47594      +14     
  Branches       5316     5319       +3     
============================================
+ Hits          44318    44330      +12     
- Misses         1785     1786       +1     
- Partials       1477     1478       +1     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø)
agent-service 99.32% <ø> (ø) Carriedforward from 7fbf64e
amber 89.23% <83.33%> (+<0.01%) ⬆️
computing-unit-managing-service 73.67% <ø> (ø)
config-service 86.73% <ø> (ø)
file-service 85.56% <ø> (ø)
frontend 95.64% <75.00%> (-0.01%) ⬇️
notebook-migration-service 79.31% <ø> (ø)
pyamber 97.57% <ø> (ø) Carriedforward from 7fbf64e
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

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

Compared against main 7fbf64e 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 424 0.259 24,712/27,558/27,558 us 🟢 -25.5% / 🔴 +126.6%
🟢 bs=100 sw=10 sl=64 945 0.577 105,006/140,386/140,386 us 🟢 -9.5% / 🔴 +53.3%
bs=1000 sw=10 sl=64 1,117 0.682 891,490/950,868/950,868 us ⚪ within ±5% / 🔴 -8.5%
Baseline details

Latest main 7fbf64e from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 424 tuples/sec 445 tuples/sec 908.02 tuples/sec -4.7% -53.3%
bs=10 sw=10 sl=64 MB/s 0.259 MB/s 0.272 MB/s 0.554 MB/s -4.8% -53.3%
bs=10 sw=10 sl=64 p50 24,712 us 19,774 us 10,907 us +25.0% +126.6%
bs=10 sw=10 sl=64 p95 27,558 us 36,990 us 13,604 us -25.5% +102.6%
bs=10 sw=10 sl=64 p99 27,558 us 36,990 us 16,697 us -25.5% +65.0%
bs=100 sw=10 sl=64 throughput 945 tuples/sec 957 tuples/sec 1,188 tuples/sec -1.3% -20.4%
bs=100 sw=10 sl=64 MB/s 0.577 MB/s 0.584 MB/s 0.725 MB/s -1.2% -20.4%
bs=100 sw=10 sl=64 p50 105,006 us 100,625 us 85,003 us +4.4% +23.5%
bs=100 sw=10 sl=64 p95 140,386 us 155,161 us 91,599 us -9.5% +53.3%
bs=100 sw=10 sl=64 p99 140,386 us 155,161 us 99,202 us -9.5% +41.5%
bs=1000 sw=10 sl=64 throughput 1,117 tuples/sec 1,119 tuples/sec 1,221 tuples/sec -0.2% -8.5%
bs=1000 sw=10 sl=64 MB/s 0.682 MB/s 0.683 MB/s 0.745 MB/s -0.1% -8.5%
bs=1000 sw=10 sl=64 p50 891,490 us 893,439 us 837,593 us -0.2% +6.4%
bs=1000 sw=10 sl=64 p95 950,868 us 957,906 us 881,015 us -0.7% +7.9%
bs=1000 sw=10 sl=64 p99 950,868 us 957,906 us 915,550 us -0.7% +3.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,471.28,200,128000,424,0.259,24712.42,27557.89,27557.89
1,100,10,64,20,2117.16,2000,1280000,945,0.577,105006.46,140386.10,140386.10
2,1000,10,64,20,17897.97,20000,12800000,1117,0.682,891490.25,950868.03,950868.03

@kz930

kz930 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan @carloea2

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

Labels

common feature frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

An advanced trainer accepts the same hyperparameter twice and emits Python that will not compile

2 participants