Skip to content

Validate positive range for log-scale distribution parameters#2679

Open
saivedant169 wants to merge 1 commit into
kubeflow:masterfrom
saivedant169:fix/validate-log-distribution-positive
Open

Validate positive range for log-scale distribution parameters#2679
saivedant169 wants to merge 1 commit into
kubeflow:masterfrom
saivedant169:fix/validate-log-distribution-positive

Conversation

@saivedant169

Copy link
Copy Markdown
Contributor

What this PR does / why we need it

Fixes #2678.

A parameter with a logUniform or logNormal distribution and a non-positive min or max is currently admitted by the webhook and then crashes the suggestion service at math.log (hyperopt create_hyperopt_domain, and skopt's hardcoded log-uniform prior) with ValueError: math domain error.

validateParameters now rejects logUniform / logNormal parameters whose min or max parses to a value <= 0, with a clear field error, so the experiment is refused at admission instead of crashing the suggestion pod. This matches how the validator already rejects other invalid feasibleSpace values, and follows the same fail-at-admission approach as #2666.

The check only fires when the value parses as a float and is <= 0, so non-numeric min/max are left to the existing handling.

Test

Added a TestValidateParameters case (non-positive min for logUniform).

go test ./pkg/webhook/v1beta1/experiment/validator/ -run TestValidateParameters
ok  github.com/kubeflow/katib/pkg/webhook/v1beta1/experiment/validator

/kind bug

A parameter with a logUniform or logNormal distribution and a non-positive
min or max is currently admitted by the webhook and then crashes the
suggestion service at math.log (hyperopt create_hyperopt_domain and skopt's
hardcoded log-uniform prior) with a math domain error.

validateParameters now rejects logUniform and logNormal parameters whose min
or max parses to a value <= 0, so the experiment is refused at admission with
a clear field error instead of crashing the suggestion pod.

Signed-off-by: Saivedant Hava <saivedant169@gmail.com>
@google-oss-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign electronic-waste for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow

Copy link
Copy Markdown

@saivedant169: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@saivedant169

Copy link
Copy Markdown
Contributor Author

@andreyvelich this one has been open about a week without a look. The red checks are the flaky ones (the trial-controller TestReconcileBatchJob gomock timing test and the e2e version-matrix timeouts), not the validator change, which passes. Since I can't trigger a re-run myself, could you kick the CI and take a look when you have a moment? It's a small kind/bug fix (positive-range validation for logUniform/logNormal params).

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.

Suggestion services crash on log-distribution parameters with non-positive min/max

1 participant