Skip to content

Honor parameter distribution in Skopt and Goptuna suggestion services - #2689

Open
saivedant169 wants to merge 1 commit into
kubeflow:masterfrom
saivedant169:fix/skopt-goptuna-distribution
Open

Honor parameter distribution in Skopt and Goptuna suggestion services#2689
saivedant169 wants to merge 1 commit into
kubeflow:masterfrom
saivedant169:fix/skopt-goptuna-distribution

Conversation

@saivedant169

Copy link
Copy Markdown
Member

What this PR does / why we need it

The Skopt and Goptuna suggestion services built their search spaces from parameter.type only and never read parameter.distribution, so a declared distribution was silently discarded:

  • Skopt (skopt/base_service.py) hardcoded a "log-uniform" prior for every double parameter and left integer parameters uniform, so a distribution: uniform double (the default) was sampled log-uniformly.
  • Goptuna (goptuna/converter.go) always built UniformDistribution / IntUniformDistribution, so a distribution: logUniform parameter was sampled uniformly.

Both now honor the declared distribution. Because both libraries support uniform and log-uniform directly (skopt.space.Real/Integer take a prior, and goptuna has LogUniformDistribution), the distribution is mapped through rather than rejected. For normal / logNormal, which neither library samples natively (and logUniform for goptuna int parameters, which goptuna has no distribution for), the service now raises a clear error naming the parameter and the unsupported distribution, consistent with the Optuna service in #2666.

Note this changes the Skopt default for double parameters from log-uniform to uniform, which is the intended correction.

Which issue(s) this PR fixes

Fixes #2688

Testing

  • Added Goptuna unit tests in converter_test.go (log-uniform maps to LogUniformDistribution; normal on a double and logUniform on an int return an error). All pass with go test ./pkg/suggestion/v1beta1/goptuna/.
  • Added Skopt unit tests in test_skopt_service.py covering the distribution to prior mapping and the fail-loud path for unsupported distributions. These run under make pytest-skopt.
  • Verified the Skopt mapping against the actual create_optimizer: a uniform double now yields prior="uniform", a log-uniform double yields prior="log-uniform", and a normal double raises ValueError.

The Skopt and Goptuna suggestion services built their search spaces from
the parameter type only and ignored the declared distribution: Skopt
hardcoded a log-uniform prior for every double parameter, and Goptuna
always built a uniform distribution, so a parameter's distribution was
silently discarded.

Map the distribution to the backend equivalent (uniform and log-uniform)
and raise a clear error for distributions the backend cannot sample
(normal and log-normal, and log-uniform for Goptuna int parameters),
consistent with the Optuna suggestion service.

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

Copy link
Copy Markdown
Member Author

Hi @andreyvelich, whenever you get a window, mind taking a look at this one? Same distribution-handling line as #2666, just extended to the skopt and goptuna services, which were silently ignoring the declared distribution. No rush.

@andreyvelich

Copy link
Copy Markdown
Member

Thanks @saivedant169!
/ok-to-test
/lgtm
/approve

@google-oss-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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

The pull request process is described 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

@saivedant169

Copy link
Copy Markdown
Member Author

/retest

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage is 9.986%saivedant169:fix/skopt-goptuna-distribution into kubeflow:master. No base build found for kubeflow:master.

@saivedant169

Copy link
Copy Markdown
Member Author

Hi @andreyvelich, this one has been approved and is green apart from the tune-example-llm-optimization e2e, which times out with the trial still Running rather than failing, so it looks unrelated to this change. A /retest last week hit the same timeout. Could you re-run it when you get a moment, or let me know if there is anything you would like me to adjust on my end?

@saivedant169

Copy link
Copy Markdown
Member Author

@andreyvelich this one's still sitting on the flaky e2e, logs are from July. Mind kicking off a rerun?

@saivedant169

Copy link
Copy Markdown
Member Author

/retest

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.

skopt and goptuna suggestion services silently ignore parameter distributions

3 participants