Skip to content

Refactor tell_with_warning to avoid unnecessary get_trial call #6133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor tell_with_warning to avoid unnecessary get_trial call #6133

wants to merge 2 commits into from
+68 −47

Conversation

sawa3030
Copy link
Collaborator

@sawa3030 sawa3030 commented Jun 6, 2025

Motivation

Remove unnecessary calls to storage.get_trial to improve efficiency.

Description of the changes

  • Removed the final storage.get_trial call in tell_with_warning.
  • Instead of returning the FrozenTrial, tell_with_warning now returns state, values, params, and values_conversion_failure_message, which are required for subsequent log output.

@sawa3030
Copy link
Collaborator Author

sawa3030 commented Jun 6, 2025

Performance Improvement

The runtime of _run_trial was compared between this PR and the main branch using the following code. The results shows a noticeable improvement in the cumulative time (cumtime) of _run_trial, as expected.

Branch Average Cumtime
master 37.3786 ± 0.5339
this PR 40.8345 ± 0.8265 
import optuna
import optunahub
import cProfile

storage = optuna.storages.RDBStorage(
    url="sqlite:///:memory:",
    engine_kwargs={"pool_size": 20, "connect_args": {"timeout": 10}},
)

def objective(trial: optuna.Trial) -> float:
    x = trial.suggest_float("x", -1, 1)
    y = trial.suggest_int("y", -1, 1)
    return x**2 + y

study = optuna.create_study(sampler=optuna.samplers.RandomSampler(), storage=storage)

profiller = cProfile.Profile()
study.optimize(objective, n_trials=500)

@sawa3030 sawa3030 marked this pull request as ready for review June 6, 2025 07:15
@not522
Copy link
Member

not522 commented Jun 10, 2025

The runtime of _run_trial was compared between this PR and the main branch using the following code. The results shows a noticeable improvement in the cumulative time (cumtime) of _run_trial, as expected.

Are the results written on the table reversed?

not522
not522 reviewed Jun 10, 2025
View reviewed changes
optuna/study/_tell.py
frozen_trial = study._storage.get_trial(frozen_trial._trial_id)

return frozen_trial, values_conversion_failure_message
return state, values, frozen_trial.params, values_conversion_failure_message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to return params? _run_trial stores them in the trial.

@c-bata
Copy link
Member

c-bata commented Jun 10, 2025

@kAIto47802 Could you review this PR?

@c-bata c-bata added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not522 not522 not522 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@not522 not522

@kAIto47802 kAIto47802

Labels
enhancement Change that does not break compatibility and not affect public interfaces, but improves performance.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sawa3030 @not522 @c-bata @kAIto47802