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.

Sign up for GitHub

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

Jump to bottom

Move STORAGE_MODES to testing/storage.py #2231

Merged
merged 2 commits into from
Jan 21, 2021
Merged

Move STORAGE_MODES to testing/storage.py #2231

merged 2 commits into from
Jan 21, 2021

Conversation

HideakiImamura
Copy link
Member

Motivation

As suggested in #2218 (comment), this PR moves the STORAGE_MODES variable which is used several times in tests to testing/storage.py.

Description of the changes

  • Move STORAGE_MODES to testing/storage.py

@github-actions github-actions bot added the optuna.testing Related to the `optuna.testing` submodule. This is automatically labeled by github-actions. label Jan 19, 2021
@HideakiImamura HideakiImamura added the test Unit test. label Jan 19, 2021
hvy
hvy approved these changes Jan 20, 2021
View reviewed changes
Copy link
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.

Thanks for the quick action, LGTM!

@hvy hvy self-assigned this Jan 20, 2021
@toshihikoyanase
Copy link
Member

@himkt Because this is a follow-up PR of #2218, could you review this PR, please?
Thank you for your cooperation in the PR review.

himkt
himkt reviewed Jan 20, 2021
View reviewed changes
Copy link
Member

@himkt himkt 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 simplifying the codebase. It looks good.
I left a minor question.

tests/storages_tests/test_storages.py
@@ -23,6 +23,7 @@
from optuna.storages import RDBStorage
from optuna.storages import RedisStorage
from optuna.storages._base import DEFAULT_STUDY_NAME_PREFIX
from optuna.testing.storage import STORAGE_MODES
Copy link
Member

Choose a reason for hiding this comment

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

[question]

This order of imports seems not to be alphabetical. Is this intended?

>>> sorted(["STORAGE_MODES", "StorageSupplier", "a"])[::-1]
['a', 'StorageSupplier', 'STORAGE_MODES']

Copy link
Member Author

Choose a reason for hiding this comment

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

This order is determined by isort automatically, so I think it is fine.

@himkt
Copy link
Member

himkt commented Jan 20, 2021

Also, please resolve a conflict. 🙏

@HideakiImamura
Copy link
Member Author

@himkt Thanks for the review! PTAL.

himkt
himkt approved these changes Jan 21, 2021
View reviewed changes
Copy link
Member

@himkt himkt left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the quick response!

@himkt himkt merged commit 08a5ea5 into optuna:master Jan 21, 2021
@toshihikoyanase toshihikoyanase added this to the v2.5.0 milestone Jan 21, 2021
@HideakiImamura HideakiImamura deleted the tests/define-STORAGE-MODE-in-test-utils branch June 9, 2023 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himkt himkt himkt approved these changes

@hvy hvy hvy approved these changes

Assignees

@himkt himkt

@hvy hvy

Labels
optuna.testing Related to the `optuna.testing` submodule. This is automatically labeled by github-actions. test Unit test.
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@HideakiImamura @toshihikoyanase @himkt @hvy