feat(operator): refuse two hyperparameter rows that set one parameter - #7982
feat(operator): refuse two hyperparameter rows that set one parameter#7982kz930 wants to merge 1 commit into
Conversation
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>
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is 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
*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 | 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|
/request-review @aglinxinyuan @carloea2 |
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
uniqueAmongRowskey 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.uniqueItemsis 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.baseScala specs pass (17 tests), as does the widermachineLearningpackage (54 tests), withscalafmtCheckAlland scalafix clean.operator-property-edit-frame.component.spec.tspasses at 237 tests.I also dumped the generated JSON schema for the SVC trainer to confirm the new key lands on the
parameterproperty 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)