Skip to content
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.

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

Remove RedisStorage #4156

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Remove RedisStorage #4156

merged 3 commits into from
Nov 15, 2022

Conversation

c-bata
Copy link
Member

@c-bata c-bata commented Nov 10, 2022

Motivation

The implementation of RedisStorage is a bit complex and code size is large since it is based on BaseStorage. RedisStorage has been found to have bugs mainly in concurrency

  • set_trial_state_values is not concurrent-safe (See Skip test_pop_waiting_trial_thread_safe on RedisStorage #4119).
  • The order of trial number and trial_id can be inconsistent (Please checkout the source code for details).
  • In the implementation of create_new_study, the return value of the GET instruction is being inserted in the transaction. (Not sure, but we can fix this problem by WATCH <KEY NAME> though.)

In Optuna v3.1, a brand-new Redis storage, which is based-on JournalStorage, is introduced. It much simpler than RedisStorage and improves maintainability as it is written in about 110 lines even after implemented a snapshot capability (#4102).

Description of the changes

In this PR, I removed RedisStorage since experimental features can be removed without a deprecation process in Optuna’s development policy.

@github-actions github-actions bot added optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. optuna.testing Related to the `optuna.testing` submodule. This is automatically labeled by github-actions. labels Nov 10, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #4156 (014de63) into master (f02f87d) will decrease coverage by 0.24%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #4156      +/-   ##
==========================================
- Coverage   90.11%   89.86%   -0.25%     
==========================================
  Files         162      161       -1     
  Lines       12734    12336     -398     
==========================================
- Hits        11475    11086     -389     
+ Misses       1259     1250       -9     
Impacted Files Coverage Δ
optuna/cli.py 20.42% <0.00%> (ø)
optuna/integration/allennlp/_executor.py 96.42% <ø> (+0.81%) ⬆️
optuna/testing/storages.py 94.87% <ø> (-0.48%) ⬇️
optuna/storages/__init__.py 100.00% <100.00%> (ø)
optuna/storages/_cached_storage.py 98.77% <100.00%> (-0.83%) ⬇️
optuna/study/_study_summary.py 84.44% <0.00%> (-6.67%) ⬇️
optuna/integration/botorch.py 97.39% <0.00%> (-0.87%) ⬇️

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

@c-bata c-bata marked this pull request as ready for review November 10, 2022 10:22
@c-bata c-bata added the compatibility Change that breaks compatibility. label Nov 10, 2022
@HideakiImamura HideakiImamura self-assigned this Nov 11, 2022
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Basically, looks good to me. I have some comments. Could you take a look?

optuna/cli.py Outdated Show resolved Hide resolved
optuna/storages/__init__.py Outdated Show resolved Hide resolved
optuna/storages/__init__.py Outdated Show resolved Hide resolved
tests/integration_tests/test_pytorch_lightning.py Outdated Show resolved Hide resolved
Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

LGTM.

@HideakiImamura HideakiImamura added this to the v3.1.0 milestone Nov 14, 2022
@HideakiImamura HideakiImamura removed their assignment Nov 14, 2022
@c-bata c-bata requested a review from knshnb November 14, 2022 10:05
@c-bata
Copy link
Member Author

c-bata commented Nov 14, 2022

@knshnb Could you review this PR?

@c-bata c-bata removed the request for review from knshnb November 15, 2022 04:40
@c-bata c-bata assigned not522 and unassigned knshnb Nov 15, 2022
@c-bata
Copy link
Member Author

c-bata commented Nov 15, 2022

Let me unassign knshnb-san since he is busy.

@not522 Could you review this PR?

Copy link
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 merged commit 016795d into optuna:master Nov 15, 2022
@not522 not522 changed the title Remove RedisStorage Remove RedisStorage Nov 15, 2022
@c-bata c-bata deleted the remove-redis-storage branch November 15, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Change that breaks compatibility. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. optuna.testing Related to the `optuna.testing` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants