feat(operator): give a trainer's hyperparameter value the constraint its parameter implies - #7946
feat(operator): give a trainer's hyperparameter value the constraint its parameter implies#7946kz930 wants to merge 7 commits into
Conversation
…its parameter implies A hyperparameter row's value was a bare text box: no accepted values, no format, no example, and not required. The enum behind the parameter dropdown already pairs each parameter with the callable that converts its text, so the operator knew what it would accept and never said. The enum now declares that too, taken from scikit-learn itself rather than from judgement, and the descriptor writes it into its own schema. The rules sit under a Texera key rather than a JSON-Schema allOf, whose members the form builder merges into one control. The form then renders what they call for: a dropdown for a chosen-from-a-set parameter, a number input for a numeric one. Closes apache#7936 Generated-by: Claude Code (Claude Opus 5) 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 #7946 +/- ##
============================================
+ Coverage 92.52% 92.81% +0.29%
- Complexity 4620 4639 +19
============================================
Files 1175 1175
Lines 47466 47588 +122
Branches 5307 5346 +39
============================================
+ Hits 43916 44168 +252
+ Misses 1980 1909 -71
+ Partials 1570 1511 -59
*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 | 372 | 0.227 | 26,085/39,460/39,460 us | 🔴 +13.3% / 🔴 +190.1% |
| 🔴 | bs=100 sw=10 sl=64 | 801 | 0.489 | 121,405/155,942/155,942 us | 🔴 +7.5% / 🔴 +70.2% |
| ⚪ | bs=1000 sw=10 sl=64 | 929 | 0.567 | 1,075,927/1,119,991/1,119,991 us | ⚪ within ±5% / 🔴 +28.5% |
Baseline details
Latest main 3198595 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 372 tuples/sec | 382 tuples/sec | 908.02 tuples/sec | -2.6% | -59.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.227 MB/s | 0.233 MB/s | 0.554 MB/s | -2.6% | -59.0% |
| bs=10 sw=10 sl=64 | p50 | 26,085 us | 25,595 us | 10,907 us | +1.9% | +139.2% |
| bs=10 sw=10 sl=64 | p95 | 39,460 us | 34,828 us | 13,604 us | +13.3% | +190.1% |
| bs=10 sw=10 sl=64 | p99 | 39,460 us | 34,828 us | 16,697 us | +13.3% | +136.3% |
| bs=100 sw=10 sl=64 | throughput | 801 tuples/sec | 820 tuples/sec | 1,188 tuples/sec | -2.3% | -32.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.489 MB/s | 0.501 MB/s | 0.725 MB/s | -2.4% | -32.5% |
| bs=100 sw=10 sl=64 | p50 | 121,405 us | 120,220 us | 85,003 us | +1.0% | +42.8% |
| bs=100 sw=10 sl=64 | p95 | 155,942 us | 145,030 us | 91,599 us | +7.5% | +70.2% |
| bs=100 sw=10 sl=64 | p99 | 155,942 us | 145,030 us | 99,202 us | +7.5% | +57.2% |
| bs=1000 sw=10 sl=64 | throughput | 929 tuples/sec | 916 tuples/sec | 1,221 tuples/sec | +1.4% | -23.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.567 MB/s | 0.559 MB/s | 0.745 MB/s | +1.4% | -23.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,075,927 us | 1,087,416 us | 837,593 us | -1.1% | +28.5% |
| bs=1000 sw=10 sl=64 | p95 | 1,119,991 us | 1,144,293 us | 881,015 us | -2.1% | +27.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,119,991 us | 1,144,293 us | 915,550 us | -2.1% | +22.3% |
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.25,200,128000,372,0.227,26084.74,39460.07,39460.07
1,100,10,64,20,2496.81,2000,1280000,801,0.489,121404.58,155941.64,155941.64
2,1000,10,64,20,21531.65,20000,12800000,929,0.567,1075927.33,1119990.83,1119990.83…nd a number gamma takes either of the words scale and auto or a number, and scale is what the estimator defaults to. Declared as float it could take neither word, so the mode most users want was unreachable; declared as str it would lose every number instead. No converter of a name covers it. It now names a lambda that hands the two words through and puts everything else past float(), and its rule carries a pattern in place of a type, no type being able to describe a choice between a set and a number. The pattern is written from what that converter takes, with digits spelled [0-9] rather than \d so Python, the browser and the JVM read it alike. Also adds the component spec that should have come with the control itself. Closes apache#7945 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tor take customizeJsonSchema passes over a schema it cannot find the row in, and writes nothing for a descriptor whose type argument is not an enum. Neither arises from a schema the generator produced, so every existing case went down the one path that works and left the guards unexercised. Also covers the two message branches a pattern rule reaches: one with no example to point at, and one where the row moved on and no branch applies any more. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tor accepts Saying a value is read as a number leaves out the half of the constraint that actually bites: C is refused at zero, n_neighbors starts at one, and a negative anything is refused nearly everywhere. Each was accepted by the editor and raised from inside scikit-learn once the run started. Eleven parameters now declare the low end of their range as the estimator states it, open or closed, read from the same Interval the accepted sets came from. coef0 declares none, being the one bounded by nothing at either end, and max_iter's is -1 rather than zero because that is its own value for no limit. Checked against the operator over the boundary values rather than assumed: the form's verdict and the converter-then-sklearn verdict agree on every one, so nothing that runs is turned away and nothing turned away would have run. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The condition named the keyword the estimator takes. For SVR's shrinking that differs from the constant offering it, so the branch never held and the value it constrains stayed unconstrained. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@carloea2 @aglinxinyuan May you take a look at it please? Thank you! |
|
/request-review @aglinxinyuan @carloea2 |
carloea2
left a comment
There was a problem hiding this comment.
The value validator is tied to the value control, but its rule depends on the parameter beside it. If value 1 is valid for C and the parameter changes to kernel, the value control does not change, so Angular may keep the old valid result. Please revalidate the value when the parameter changes and add a test using the real field controls.
…ide it changes The value's rule comes from the parameter chosen next to it, but the validator sits on the value control, and Angular re-runs a validator only when the control carrying it changes. A value typed for one parameter therefore kept the verdict it earned there: 1.0 stayed valid once the row switched from C to kernel. The field now re-judges itself whenever a sibling a condition names changes, reading formly's own event rather than the sibling control's so that the row model the branch is chosen from is already the new one, and comparing parents so that one table row leaves the others alone. The wiring moves out of the property editor into setValueRules beside the validator it belongs with, which also lets the new tests start from what the editor really builds: a rendered form over the real field controls, where picking a parameter is what drives the assertions. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in 6345c7a. The value field now re-judges itself when a parameter beside it changes, reading formly's The new tests drive a rendered form over the real field controls rather than a hand-made field: with |
…he code Two facts a reader cannot get from reading it: Angular re-runs a validator only for its own control, and the row model is current by the time formly emits. The rest restated the body. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What changes were proposed in this PR?
The four advanced Sklearn trainers (SVC, SVR, KNN Classifier, KNN Regressor) take their hyperparameters as a table: each row picks a parameter from a dropdown and types a value beside it. The value box carried no constraint at all, so
1forkernel, or an empty box, was accepted by the editor and only failed once the run reached scikit-learn.The enum behind the parameter dropdown already pairs every parameter with the callable that converts the user's text. It now also carries what that parameter accepts, read out of scikit-learn rather than written from judgement: the accepted words where scikit-learn fixes a set, and one example value plus the low end of the range where it does not. The descriptor writes those rules into its own schema through a new
JsonSchemaCustomizerhook on the metadata generator, and a newconstrainedvalueFormly control renders what they call for: a dropdown for a fixed set, a number input for a numeric parameter, a plain text box otherwise. The value is now required as well, conditionally onparametersSource, which decides whether the row reads it from the box or from a column.gammaalso gets a new converter. It accepts either of the wordsscaleandautoor a number, which no converter named after a type can carry, so it is now a lambda and its rule is a pattern.metricandmetric_paramsare left alone: their converters are wrong in a different way, which is #7593's subject.Any related issues, documentation, discussions?
Closes #7936.
How was this PR tested?
New and extended specs over the generated schema (
SklearnAdvancedBaseDescSpec), the Formly branch selection and validator (formly-utils.spec.ts), and the new value control (constrained-value.component.spec.ts).Every set, bound, pattern and example was checked against scikit-learn 1.7.2 rather than assumed, and on the boundary values of every bounded parameter the form's verdict was compared with the converter-then-scikit-learn verdict. Tried in the running application as well, on an SVM Classifier Trainer.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)