Support multiple studies in `InMemoryStorage`. by ytsmiling · Pull Request #1228 · optuna/optuna

ytsmiling · 2020-05-11T10:26:44Z

Motivation

This PR adds multi-study support to in-memory storage.
This partially addresses #1170 and relates to #1191.

Description of the changes

Add multi-study support to in-memory db.

Other notes

I checked that this passes tests in #1191 in python3.5.

Update:

In this PR, in-memory storage raises ValueError instead of KeyError to pass deprecated tests, which will be fixed in Increase test coverage of storage tests for single worker cases. #1191.
This PR removes some deprecated tests which will be completely removed or replaced by Increase test coverage of storage tests for single worker cases. #1191.

ytsmiling · 2020-05-11T11:33:05Z

tests/storages_tests/test_storages.py

    if isinstance(storage, InMemoryStorage):
-        with pytest.raises(ValueError):
-            # InMemoryStorage shares the same study if create_new_study is additionally invoked.
-            # Thus, the following line should fail due to distribution incompatibility.
-            storage.set_trial_param(trial_id_3, "y", 1, distribution_y_2)
+        pass


These "tests" will be completely removed by #1191.

ytsmiling · 2020-05-11T12:03:50Z

tests/integration_tests/test_chainermn.py

-
-        with pytest.raises(ValueError):
-            ChainerMNStudy(study, comm)
+        pass


sile · 2020-05-12T01:29:22Z

@ytsmiling Thank you for your PR. I'll review this PR today.
By the way, could you run a benchmark to measure the execution speed of the new implementation?

ytsmiling · 2020-05-12T02:03:59Z

Result of the same benchmark with #1135: (1000 trials 30 params)
Current master:
214310942 function calls (192531182 primitive calls) in 130.340 seconds
This PR:
81599936 function calls (81348176 primitive calls) in 82.710 seconds

So, rewriting the in-memory db codes resulted in over 30% speedup.

sile · 2020-05-12T02:06:45Z

Oh, great! Honestly, I was a bit concern that this PR might introduce some overhead.

ytsmiling · 2020-05-12T02:11:20Z

I guess that the speed up comes from rewriting the get_trials method and other methods have increased latency.
But anyway, the overhead seems not that problem.

optuna/storages/in_memory.py

hvy

Thanks for the PR. I'm also concerned about the locking but left some minor comments and questions from my side.

optuna/storages/in_memory.py

hvy · 2020-05-12T04:56:50Z

optuna/storages/in_memory.py

+        self._check_study_id(study_id)
        with self._lock:
-            return copy.deepcopy(self.study_user_attrs)
+            return self._studies[study_id].user_attrs


Please correct me if I'm wrong but changing the copy behavior seems to go a bit beyond the scope of this PR (ditto for get_study_system_attrs and get_trial).

Yes, it can be addressed in another PR.
However, the current implementation of this PR relies on this copying behavior of get_trial, and modifying the line requires a large change of codes.

I'll create another PR to change the copying behavior and I'd like to merge it before this PR.

After examining the code, I decided not to create separate PR and keep the original copying behavior in this PR.

optuna/storages/in_memory.py

ytsmiling · 2020-05-12T06:17:59Z

#1228 (comment)
This probably is a reason for the speed gain by this PR.

ytsmiling · 2020-05-12T06:47:04Z

After reverting the deep-copy behavior in #1228 (comment), the benchmark became as follows:

         209306939 function calls (188385179 primitive calls) in 129.212 seconds

This is almost the same as the current master implementation.

hvy

Thanks for your quick fixes. I left some additional comments related to your latest changes.

optuna/storages/in_memory.py

sile

LGTM.
I'm looking forward to merging this PR as the inconsistent behavior of InMemoryStrage was a concern for a long time.

hvy

LGTM!

ytsmiling added 4 commits May 11, 2020 19:19

Support multi-studies in in-memory storage.

26453d5

Introduce _StudyInfo class to simplify codes in in-memory storage.

a02e166

Fix unintentional access to a removed attribute in in-memory db.

7b2e211

Remove unnecessary and deprecated tests for in-memory storage.

aaeb9e5

ytsmiling mentioned this pull request May 11, 2020

Increase test coverage of storage tests for single worker cases. #1191

Merged

47 tasks

ytsmiling added 2 commits May 11, 2020 19:52

Intentionally introduce a bug to pass deprecated tests.

17a5cff

Remove deprecated test codes which is already addressed in optuna#1191.

84a6723

ytsmiling commented May 11, 2020

View reviewed changes

Remove deprecated tests.

f93e58e

ytsmiling commented May 11, 2020

View reviewed changes

Remove unused import.

8f05519

ytsmiling added bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. labels May 11, 2020

sile added compatibility Change that breaks compatibility. and removed compatibility Change that breaks compatibility. labels May 12, 2020

sile requested changes May 12, 2020

View reviewed changes

Lock in-memory storage at each method for thread safety.

72d084d

hvy requested changes May 12, 2020

View reviewed changes

ytsmiling added 5 commits May 12, 2020 14:52

Remove duplicated code fragment.

f2a18bb

Use sum instead of len to simplify code.

6d2aacf

Reduce the number of indirect accesses.

88e142b

Simplify deep-copy code.

d55573a

Use max_trial_id in in-memory db to ensure trial_id uniqueness.

b7f6df7

ytsmiling added 2 commits May 12, 2020 15:39

Copy mutable values before returning.

8989d63

Reformat code.

8756267

ytsmiling mentioned this pull request May 12, 2020

Major refactoring of storage classes. #1170

Closed

20 tasks

hvy requested changes May 12, 2020

View reviewed changes

optuna/storages/in_memory.py Outdated Show resolved Hide resolved

optuna/storages/in_memory.py Outdated Show resolved Hide resolved

optuna/storages/in_memory.py Outdated Show resolved Hide resolved

ytsmiling added 2 commits May 12, 2020 17:02

Fix deep-copy behavior in get_best_trial in InMemoryStorage.

e7d37e5

Resolve nested lock acquisition.

a62ace6

sile approved these changes May 12, 2020

View reviewed changes

sile added this to the v1.5.0 milestone May 12, 2020

hvy approved these changes May 12, 2020

View reviewed changes

hvy merged commit 99b924a into optuna:master May 12, 2020

c-bata mentioned this pull request May 12, 2020

Support multiple studies at InMemoryStorage and BlackholeStorage c-bata/goptuna#128

Open

ytsmiling mentioned this pull request May 31, 2020

[RFC] Change the default argument of gc_after_trial in Study.optimize to False. #1307

Closed

Uh oh!

Conversation

ytsmiling commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description of the changes

Other notes

Uh oh!

ytsmiling May 11, 2020

Choose a reason for hiding this comment

Uh oh!

ytsmiling May 11, 2020

Choose a reason for hiding this comment

Uh oh!

sile commented May 12, 2020

Uh oh!

ytsmiling commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sile commented May 12, 2020

Uh oh!

ytsmiling commented May 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hvy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hvy May 12, 2020

Choose a reason for hiding this comment

Uh oh!

ytsmiling May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytsmiling May 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ytsmiling commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ytsmiling commented May 12, 2020

Uh oh!

hvy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sile left a comment

Choose a reason for hiding this comment

Uh oh!

hvy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ytsmiling commented May 11, 2020 •

edited

Loading

ytsmiling commented May 12, 2020 •

edited

Loading

ytsmiling May 12, 2020 •

edited

Loading

ytsmiling commented May 12, 2020 •

edited

Loading