Skip to content

Support Copy-on-Write for thread safety in in-memory storage.#1139

Merged
hvy merged 1 commit intooptuna:masterfrom
ytsmiling:ensure-copy-on-write
Apr 27, 2020
Merged

Support Copy-on-Write for thread safety in in-memory storage.#1139
hvy merged 1 commit intooptuna:masterfrom
ytsmiling:ensure-copy-on-write

Conversation

@ytsmiling
Copy link
Copy Markdown
Member

Since optuna internals are assumed to work in a copy-on-write manner, samplers and others access to trials without deep-copy. However, the in-memory storage class actually does not follow a copy-on-write fashion.
While I did not check every sampler/study logics, the missing copy can result in untractable bugs under multi-thread settings.
This PR implements the missing copy-on-write logics for in-memory storage.

@ytsmiling ytsmiling added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Apr 19, 2020
@codecov-io
Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@93a55d7). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1139   +/-   ##
=========================================
  Coverage          ?   90.41%           
=========================================
  Files             ?      128           
  Lines             ?    11203           
  Branches          ?        0           
=========================================
  Hits              ?    10129           
  Misses            ?     1074           
  Partials          ?        0           
Impacted Files Coverage Δ
optuna/storages/in_memory.py 98.02% <100.00%> (ø)
tests/test_logging.py 100.00% <0.00%> (ø)
optuna/visualization/utils.py 77.77% <0.00%> (ø)
optuna/visualization/contour.py 92.24% <0.00%> (ø)
...ests/visualization_tests/test_intermediate_plot.py 97.29% <0.00%> (ø)
optuna/cli.py 32.05% <0.00%> (ø)
optuna/integration/pytorch_ignite.py 75.00% <0.00%> (ø)
optuna/visualization/optimization_history.py 96.66% <0.00%> (ø)
tests/samplers_tests/test_samplers.py 92.36% <0.00%> (ø)
tests/storages_tests/rdb_tests/test_storage.py 97.07% <0.00%> (ø)
... and 119 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93a55d7...2602b76. Read the comment docs.

@sile sile added this to the v1.4.0 milestone Apr 22, 2020
@sile sile self-assigned this Apr 22, 2020
Copy link
Copy Markdown
Member

@sile sile left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

FYI I executed a benchmark and the result is shown here. The execution speed overhead introduced by this PR is about 5%. I think that this is an acceptable cost for safety.

@hvy hvy self-assigned this Apr 27, 2020
Copy link
Copy Markdown
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants