fix(optimizers): build a pruner instance instead of passing the schema string - #823
Closed
Maarmapa wants to merge 1 commit into
Closed
fix(optimizers): build a pruner instance instead of passing the schema string#823Maarmapa wants to merge 1 commit into
Maarmapa wants to merge 1 commit into
Conversation
…a string
`OptunaSchema` declares `pruner` as an enum of strings, and `optimize()`
forwarded that string straight to `optuna.create_study()`. The sampler right
above it is resolved and instantiated; the pruner was not.
`create_study()` does not validate the argument, so nothing fails at study
creation — the study is simply built with `pruner` set to a `str`. Checked
against Optuna 4.9.0:
>>> study = optuna.create_study(direction="maximize", pruner="MedianPruner")
>>> study.pruner
'MedianPruner'
Two consequences follow. Today the setting is inert: `objective()` never calls
`trial.report()` or `trial.should_prune()`, so the pruner is never consulted and
selecting "MedianPruner" or "None" produces identical runs. And it breaks as
soon as pruning is wired up, since `should_prune()` calls
`self.study.pruner.prune(...)` and raises
`AttributeError: 'str' object has no attribute 'prune'`.
`_build_pruner()` resolves the name the same way the sampler is resolved.
"None" — the string the schema sends when pruning is disabled — maps to
`NopPruner` rather than to Python's `None`, because a bare `None` makes Optuna
fall back to its own default (`MedianPruner`), which is not what the user
picked. Pruners that cannot be built without arguments (`PatientPruner`,
`PercentilePruner`, `ThresholdPruner`) raise a message naming the ones that can;
exposing them means adding their configuration to the schema first.
Adds `tests/back/optimizers/test_optuna_pruner.py` — 10 cases covering instance
resolution, the "None" mapping, the unknown-name error and the
needs-configuration error.
Whether `objective()` should report intermediate values so pruning can actually
take effect is a separate decision: it needs a meaningful per-step metric, which
not every model exposes. That is deliberately left out of this change.
Irozuku
self-requested a review
August 18, 2026 11:12
Collaborator
|
This approach doesn't seem to address the main issue, since the pruner still won't work. Considering that most of the models currently available don't use epoch/step based training, removing the pruner as a parameter seems more reasonable for now. Feel free to try to adapt the optimizer so that it actually works with models such as MLP/CNN/Transformer based models, which can actually benefit from early stopping per trial. |
Maarmapa
pushed a commit
to Maarmapa/dashAI
that referenced
this pull request
Aug 18, 2026
Follow-up to the review on DashAISoftware#823. Resolving the pruner into an instance was necessary but not sufficient: a pruner only ever acts if the trial is told how it is doing while it still runs, and nothing was telling it. The deeper reason it could not work `optimize` called the model without validation data: self.model.train(input_dataset["train"], output_dataset["train"]) while the non-optimizer path in model_job.py passes all four: model.train(x["train"], y["train"], x["validation"], y["validation"]) Every epoch loop guards its validation metrics behind `if x_validation is not None`, so during optimization the per-epoch validation score was never computed at all. The number a pruner needs to decide did not exist — independently of whether the pruner was an instance or a string. Where the hook lives Five models train in epochs (base_torchvision, cnn, mlp, lenet5, and scikit_learn/mlp_regression) and they share no ancestor below `BaseModel`, so hooking each loop would mean five edits and would miss whatever is added next. But all five already route their per-epoch metrics through `BaseModel.calculate_metrics`, and so does the HuggingFace path via its own callback. That is the seam: one optional `_epoch_reporter` on the base class, invoked only for `level=EPOCH` and `split=VALIDATION`. Models that train in a single shot never reach that branch, so for them nothing changes. The reporter is invoked AFTER metrics are persisted, because it is allowed to raise — that is how Optuna prunes — and the epoch that triggered the stop should survive it. `TrialPruned` travels from inside the loop up to `study.optimize` with nothing in between catching it, so the trial is recorded as pruned rather than failed. A metric missing from an epoch is not an error: `calculate_metrics` drops metrics that return a non-finite value (one class present in a split, say), so the trial simply continues unpruned. Tests 11 new cases: that each epoch is reported with its step, that a rejected trial raises `TrialPruned`, that a missing metric is tolerated, that the reported metric is the one being optimized, that the hook fires for exactly epoch+validation and stays quiet for train metrics and trial-level summaries, that a model with no reporter behaves as before, and that an epoch's metrics are saved before the hook can abort. The three doubles in test_optuna_best_params.py now mirror the real `BaseModel.train` signature. They declared `train(self, x, y)`, which no actual model does — every one of them, across the torch, scikit-learn and HuggingFace families, already accepts `x_validation=None, y_validation=None`, because the normal training path has always passed them. Full suite: 806 passed. The remaining 1 failure and 6 errors reproduce identically on a clean develop checkout — the frontend build and the HuggingFace model downloads, neither reachable from this environment.
Contributor
Author
|
"Reworked and reopened as #828 ". |
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.
Closes #821
The problem
OptunaSchemadeclarespruneras an enum of strings, andoptimize()forwarded that string straight tooptuna.create_study(). The sampler on the line above is resolved and instantiated; the pruner was not.create_study()does not validate the argument, so nothing fails at study creation. Checked against Optuna 4.9.0:Two consequences follow:
objective()never callstrial.report()ortrial.should_prune(), so the pruner is never consulted — selectingMedianPrunerand selectingNoneproduce identical runs.should_prune()callsself.study.pruner.prune(...), which raisesAttributeError: 'str' object has no attribute 'prune'.The change
_build_pruner()resolves the name the same way the sampler is already resolved."None"— the string the schema sends when pruning is disabled — maps toNopPrunerrather than to Python'sNone. A bareNonemakes Optuna fall back to its own default (MedianPruner), which is not what the user picked. Happy to switch toNoneif you prefer that behaviour.Pruners that cannot be built without arguments (
PatientPruner,PercentilePruner,ThresholdPruner) raise an error naming the ones that can. Exposing them properly means adding their configuration to the schema first, which felt out of scope here.Tests
tests/back/optimizers/test_optuna_pruner.py— 10 cases covering instance resolution, the"None"mapping, the unknown-name error and the needs-configuration error.ruff checkandruff format --checkpass.Out of scope
Whether
objective()should report intermediate values so pruning can actually take effect is a separate decision — it needs a meaningful per-step metric, which not every model exposes. Left out on purpose.