Skip to content

Speed up get_all_trials in InMemoryStorage#4716

Merged
c-bata merged 3 commits intooptuna:masterfrom
not522:faster-get-all-trials-in-memory
Jun 8, 2023
Merged

Speed up get_all_trials in InMemoryStorage#4716
c-bata merged 3 commits intooptuna:masterfrom
not522:faster-get-all-trials-in-memory

Conversation

@not522
Copy link
Copy Markdown
Member

@not522 not522 commented May 30, 2023

Motivation

Filtering trials with filter and list is slower than list comprehension. It is a bottleneck when the number of trials is large.

import optuna

def objective(trial):
    return sum([trial.suggest_float(f"x_{i}", -100, 100) ** 2 for i in range(10)])

study = optuna.create_study(sampler=optuna.samplers.RandomSampler())
study.optimize(objective, n_trials=10000)
  • master
$ time python bench.py
real	0m33.892s
user	0m33.397s
sys	0m0.530s
  • PR
$ time python bench.py
real	0m29.847s
user	0m29.413s
sys	0m0.497s

Description of the changes

  • Use a list comprehension to speed up get_all_trials.
  • Simplify the implementation.

@not522 not522 added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label May 30, 2023
@github-actions github-actions bot added the optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. label May 30, 2023
@c-bata
Copy link
Copy Markdown
Member

c-bata commented May 30, 2023

@Alnusjaponica Could you review this PR?

@not522 not522 force-pushed the faster-get-all-trials-in-memory branch from b21b7f1 to 246bdca Compare May 30, 2023 00:53
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 30, 2023 •
edited
Loading

Codecov Report

Merging #4716 (1a84747) into master (a031e95) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #4716      +/-   ##
==========================================
+ Coverage   90.86%   90.87%   +0.01%     
==========================================
  Files         188      188              
  Lines       14323    14321       -2     
==========================================
  Hits        13014    13014              
+ Misses       1309     1307       -2
Impacted Files Coverage Δ
optuna/storages/_in_memory.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

c-bata
c-bata reviewed May 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM with a nit.

@Alnusjaponica
Copy link
Copy Markdown
Contributor

Alnusjaponica commented Jun 1, 2023

The change itself LGTM. I am going to approve this PR after @not522 added the test for the commented part.

c-bata
c-bata approved these changes Jun 1, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM!

@c-bata
Copy link
Copy Markdown
Member

c-bata commented Jun 1, 2023

The change itself LGTM. I am going to approve this PR after @not522 added the test for the commented part.

I think that we might not need to add a test case if the existing test cases would fail without the shallow copy.

@not522 Do you think it's necessary to add more test cases?

@c-bata c-bata removed their assignment Jun 1, 2023
@not522
Copy link
Copy Markdown
Member Author

not522 commented Jun 1, 2023

I think the tests are insufficient. There is no test for this feature and only the SuccessiveHalvingPruner's test fails. https://github.com/optuna/optuna/actions/runs/5116276586/jobs/9198283871

@not522
Copy link
Copy Markdown
Member Author

not522 commented Jun 6, 2023

I added the test that returned FrozenTrials are not modified by the subsequent set_trial_xxx. PTAL.

Alnusjaponica
Alnusjaponica approved these changes Jun 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Alnusjaponica Alnusjaponica left a comment

Choose a reason for hiding this comment

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

LGTM.

Alnusjaponica
Alnusjaponica approved these changes Jun 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Alnusjaponica Alnusjaponica left a comment

Choose a reason for hiding this comment

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

LGTM.

@Alnusjaponica Alnusjaponica removed their assignment Jun 8, 2023
@c-bata c-bata added this to the v3.3.0 milestone Jun 8, 2023
@c-bata c-bata merged commit 73a3544 into optuna:master Jun 8, 2023
@not522 not522 deleted the faster-get-all-trials-in-memory branch June 9, 2023 04:11
@not522 not522 mentioned this pull request Jan 28, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@c-bata c-bata c-bata approved these changes

@Alnusjaponica Alnusjaponica Alnusjaponica approved these changes

Assignees

No one assigned

Labels

enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions.

Projects

None yet

Milestone

v3.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@not522 @c-bata @codecov-commenter @Alnusjaponica