Skip to content

Refactor tell_with_warning to avoid unnecessary get_trial call#6133

Merged
not522 merged 5 commits intooptuna:masterfrom
sawa3030:remove-the-last-get-trial-in-tell-with-warning
Jun 27, 2025
Merged

Refactor tell_with_warning to avoid unnecessary get_trial call#6133
not522 merged 5 commits intooptuna:masterfrom
sawa3030:remove-the-last-get-trial-in-tell-with-warning

Conversation

@sawa3030
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

sawa3030 commented Jun 6, 2025 •
edited
Loading

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 40.8345 ± 0.8265
this PR 37.3786 ± 0.5339 
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
Copy Markdown
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 Outdated
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
Copy Markdown
Member

@not522 not522 Jun 10, 2025

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
Copy Markdown
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
@sawa3030
Copy link
Copy Markdown
Collaborator Author

sawa3030 commented Jun 12, 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?

Thank you for pointing that out — the results were indeed reversed. I've corrected the comment accordingly.

@sawa3030 sawa3030 force-pushed the remove-the-last-get-trial-in-tell-with-warning branch from 4be65ae to 01a1f95 Compare June 12, 2025 08:20
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 12, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.30%. Comparing base (440d03a) to head (04f4f53).
Report is 196 commits behind head on master.

Files with missing lines Patch % Lines
optuna/study/study.py 77.77% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6133      +/-   ##
==========================================
- Coverage   88.39%   88.30%   -0.09%     
==========================================
  Files         207      207              
  Lines       14036    14027       -9     
==========================================
- Hits        12407    12387      -20     
- Misses       1629     1640      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

not522
not522 reviewed Jun 18, 2025
View reviewed changes
optuna/study/study.py
skip_if_finished=skip_if_finished,
)
return copy.deepcopy(frozen_trial)
return copy.deepcopy(_get_frozen_trial(self, trial))
Copy link
Copy Markdown
Member

@not522 not522 Jun 18, 2025

Choose a reason for hiding this comment

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

How about use self._storage.get_trial?

Copy link
Copy Markdown
Collaborator Author

@sawa3030 sawa3030 Jun 19, 2025

Choose a reason for hiding this comment

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

I've made the updates. PTAL!

Copy link
Copy Markdown
Member

@not522 not522 Jun 19, 2025

Choose a reason for hiding this comment

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

Since the argument is the trial number, we should use get_trial_id_from_study_id_trial_number. After reconsideration, this solution turns out to be more complex than I expected, so reverting to using _get_frozen_trial may be good. 🙇

not522
not522 approved these changes Jun 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

LGTM!

@not522 not522 removed their assignment Jun 20, 2025
kAIto47802
kAIto47802 approved these changes Jun 27, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@kAIto47802 kAIto47802 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. LGTM!

@kAIto47802 kAIto47802 removed their assignment Jun 27, 2025
@not522 not522 merged commit d1018c5 into optuna:master Jun 27, 2025
14 checks passed
@not522 not522 added this to the v4.5.0 milestone Jun 27, 2025
@sawa3030 sawa3030 deleted the remove-the-last-get-trial-in-tell-with-warning branch November 14, 2025 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@not522 not522 not522 approved these changes

@kAIto47802 kAIto47802 kAIto47802 approved these changes

Assignees

No one assigned

Labels

enhancement Change that does not break compatibility and not affect public interfaces, but improves performance.

Projects

None yet

Milestone

v4.5.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sawa3030 @not522 @c-bata @kAIto47802