Increase test coverage of storage tests for single worker cases. by ytsmiling · Pull Request #1191 · optuna/optuna

ytsmiling · 2020-04-30T04:53:31Z

Motivation

Many storage features/specs are not tested. This low test coverage is a direct cause of the inconsistent implementations among storages. This PR aims to increase the test coverage and, at the same time, remove found bugs/inconsistent behaviors. This PR inevitably relies on #1175 and #1176. While we should test specs related to multi-worker settings, it will make this PR too complicated and it will be separated into another PR.

Description of the changes

This PR increases test coverage by adding more cases in tests/test_storage.py. Additionally, this PR resolves many inconsistent behaviors and bugs in the current implementation.
This PR slows down the test, which should be resolved by another PR by removing duplicated/redundant methods in the storage class.
This PR includes multi-study support from the in-memory storage.
This PR changes the interface of testing/StorageSuppliers and related tests. Importantly, this PR removes tests using "common sqlite's db file." Tests related to behaviors in multi-worker settings should be added in other ways by other PRs.

The following space will be used to list all fixed/modified behaviors of storage implementations.

Some behaviors which need discussion, but not tackled in this PR.

Deleted study_id is reused in RDB backend (sqlite).
In-memory database arrows to change RUNNING state into WAITING.
RDB database arrows to change RUNNING state into WAITING.
Redis database arrows to change RUNNING state into WAITING.
Add MySQL to tests.
Add PostgreSQL to tests.
In-memory database arrows to change state into FINISHED even when the value is not set.
RDB database arrows to change state into FINISHED even when the value is not set.
Redis database arrows to change state into FINISHED even when the value is not set.
In-memory storage became slow due to multi-study support.

…-storage-spec # Conflicts: # tests/storages_tests/test_storages.py

ytsmiling · 2020-05-11T10:33:03Z

Multi-study support will be addressed in #1228 instead of this PR.

# Conflicts: # optuna/storages/in_memory.py # tests/integration_tests/test_chainermn.py # tests/storages_tests/test_storages.py

tests/storages_tests/test_storages.py

ytsmiling · 2020-05-12T20:39:49Z

optuna/storages/redis.py

                system_attrs={},
                n_trials=0,
-                datetime_start=datetime.now(),
+                datetime_start=None,


comment: In InMemoryStorage and RDBStorage, the datetime_start is not the time the study created but the time the first trial in the study started.

ytsmiling · 2020-05-12T20:41:21Z

optuna/storages/redis.py

+        if trial.state.is_finished():
+            self._update_cache(trial_id)


These two lines are necessary to manage StudySummaries appropriately.

ytsmiling · 2020-05-12T20:42:44Z

tests/storages_tests/rdb_tests/test_storage.py

@@ -119,135 +110,6 @@ def test_set_default_engine_kwargs_for_mysql_with_other_rdb():
    assert "pool_pre_ping" not in engine_kwargs




comment: Logic tests for RDB storage are merged into tests/storage_tests/test_storages.py.

codecov-io · 2020-05-12T22:58:19Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e436a84). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1191   +/-   ##
=========================================
  Coverage          ?   86.28%           
=========================================
  Files             ?       92           
  Lines             ?     6781           
  Branches          ?        0           
=========================================
  Hits              ?     5851           
  Misses            ?      930           
  Partials          ?        0

Impacted Files	Coverage Δ
optuna/integration/__init__.py	`63.04% <0.00%> (ø)`
optuna/storages/rdb/models.py	`95.19% <0.00%> (ø)`
optuna/integration/lightgbm.py	`90.00% <0.00%> (ø)`
optuna/trial/_util.py	`100.00% <0.00%> (ø)`
optuna/multi_objective/samplers/_adapter.py	`100.00% <0.00%> (ø)`
optuna/integration/chainer.py	`80.85% <0.00%> (ø)`
optuna/cli.py	`32.05% <0.00%> (ø)`
optuna/storages/rdb/storage.py	`96.06% <0.00%> (ø)`
optuna/testing/integration.py	`100.00% <0.00%> (ø)`
optuna/integration/xgboost.py	`86.66% <0.00%> (ø)`
... and 82 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 e436a84...e436a84. Read the comment docs.

optuna/storages/rdb/storage.py

sile

LGTM but I left minor suggestions to make the style of the source code comments align to the Optuna coding convention.

tests/storages_tests/test_storages.py

Co-authored-by: Takeru Ohta <phjgt308@gmail.com>

toshihikoyanase · 2020-05-13T01:14:50Z

optuna/storages/in_memory.py


        if trial_id not in self._trial_id_to_study_id_and_number:
-            raise ValueError("No trial with trial_id {} exists.".format(trial_id))
+            raise KeyError("No trial with trial_id {} exists.".format(trial_id))


[Comment] The trial_id is an internal attribute, but I think it is reasonable to use it here because this KeyError is not usually raised by user-code problems.

toshihikoyanase

Thank you for improving the storage tests. It basically looks good to me.
I have small suggestions and a few questions.

optuna/testing/storage.py

tests/storages_tests/test_storages.py

toshihikoyanase · 2020-05-15T08:03:44Z

tests/storages_tests/test_storages.py

+def _check_trial_equality(output: FrozenTrial, expected: FrozenTrial) -> None:
+    assert output._trial_id == expected._trial_id
+    assert output.state == expected.state
+    assert output.value == expected.value
+    assert output.datetime_start == expected.datetime_start
+    assert output.datetime_complete == expected.datetime_complete
+    assert output.user_attrs == expected.user_attrs
+    assert output.system_attrs == expected.system_attrs
+    assert output.intermediate_values == expected.intermediate_values


assert output == expected is insufficient even if FrozenTrial has the __eq__ method?

It seems I have misunderstood somethin, and I replaced them with __eq__.

I found that for CategoricalDistribution, json_to_distribution(distribution_to_json(dist)) != dist in general and I'll create a PR to add an appropriate eq method to CategoricalDistribution.

I'll create a PR to add an appropriate eq method to CategoricalDistribution.

Good catch! Thank you.

tests/storages_tests/test_storages.py

Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>

toshihikoyanase

LGTM! Thank you for improving the storage tests.

By the way, how about creating issue on the issue on CategoricalDistribution.__eq__ if it takes time to create a PR?

Increase test coverage of storage tests.

280c4fb

ytsmiling force-pushed the follow-storage-spec branch from 78043d7 to 280c4fb Compare April 30, 2020 04:54

ytsmiling added 2 commits April 30, 2020 13:59

Increase test coverage of storage tests.

754642d

Fix comment style.

c8bf1c8

ytsmiling mentioned this pull request Apr 30, 2020

Major refactoring of storage classes. #1170

Closed

20 tasks

ytsmiling added 21 commits April 30, 2020 16:09

Raise KeyError for non-existent study in RDB storage.

024e360

Raise KeyError for non-existent study in RDB storage.

e7e47e5

Raise KeyError for non-existent trial in RDB storage.

e5201b7

Raise KeyError for non-existent trial or study in Redis storage.

396120a

Merge remote-tracking branch 'origin/follow-storage-spec' into follow…

f6ded29

…-storage-spec # Conflicts: # tests/storages_tests/test_storages.py

Remove redundant tests for in-memory storage.

7f66433

Fix wrong non-existent study check in get_study_id_from_name test.

174ae32

Remove test of get_study_user_attr method, which does not exist.

e5ee7e2

Fix bug in a test for create_new_trial method.

2e56eef

Support multiple studies in in_memory database.

d5d2a84

Fix bug in deep copy option for get_all_trials in storage tests.

09ea256

Fix wrong distribution cache in in-memory storage.

d9ee9b8

Check the existence of study in attributes fetch in RDB storages.

bdaedde

Check the existence of trial when fetching trial attributes.

8897a56

Remove direct comparisons of trials.

f19836f

Raise KeyError for non-existent study or trial in Redis storage.

5b94560

Fix deepcopy option of get_all_trials in RDB storage.

f2cc985

Fix wrong check in storage test.

da8d3b7

Fix wrong check in storage test.

1f37bb6

Update trial cache in redis storage on trial creation.

2391412

Remove unnecessary RDB-specific tests.

34c2276

ytsmiling mentioned this pull request May 11, 2020

Support multiple studies in InMemoryStorage. #1228

Merged

ytsmiling mentioned this pull request May 11, 2020

Add storage cache #1140

Merged

3 tasks

ytsmiling added a commit to ytsmiling/optuna that referenced this pull request May 11, 2020

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

84a6723

ytsmiling added 3 commits May 13, 2020 05:31

Merge branch 'upstream-master' into follow-storage-spec

2ba3274

# Conflicts: # optuna/storages/in_memory.py # tests/integration_tests/test_chainermn.py # tests/storages_tests/test_storages.py

Fix error raises in get_study_id_from_name in InMemoryStorage.

8226829

Remove unintentional uncommenting.

05e25ef

ytsmiling commented May 12, 2020

View reviewed changes

Merge branch 'upstream-master' into follow-storage-spec

d257652

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

sile reviewed May 13, 2020

View reviewed changes

optuna/storages/rdb/storage.py Show resolved Hide resolved

sile approved these changes May 13, 2020

View reviewed changes

tests/storages_tests/test_storages.py Outdated Show resolved Hide resolved

tests/storages_tests/test_storages.py Outdated Show resolved Hide resolved

Fix comment style.

6c4be80

Co-authored-by: Takeru Ohta <phjgt308@gmail.com>

toshihikoyanase reviewed May 13, 2020

View reviewed changes

toshihikoyanase mentioned this pull request May 15, 2020

Release Tasks for v1.5.0. #1245

Closed

2 tasks

toshihikoyanase requested changes May 15, 2020

View reviewed changes

ytsmiling and others added 10 commits May 15, 2020 18:10

Fix styling issues.

63e0eb9

Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>

Fix test name in storage test.

8300ad6

Co-authored-by: Toshihiko Yanase <toshihiko.yanase@gmail.com>

Fix type of CategoricalDistribution.

30ef0fa

Remove redundant function.

b5d6eb0

Remove unused trial generator function from storage test.

63a9da8

Remove unused import.

527612e

Remove redundant #NOQA comment.

0ee4d54

Remove StorageSupplier("common")-related codes.

87e9e6d

Reformat code.

3e0f443

Remove unused setup_module call in tests.

e436a84

ytsmiling requested a review from toshihikoyanase May 18, 2020 00:56

toshihikoyanase approved these changes May 18, 2020

View reviewed changes

toshihikoyanase merged commit c569c95 into optuna:master May 18, 2020

		@@ -119,135 +110,6 @@ def test_set_default_engine_kwargs_for_mysql_with_other_rdb():
		assert "pool_pre_ping" not in engine_kwargs

Uh oh!

Conversation

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

Uh oh!

Motivation

Description of the changes

The following space will be used to list all fixed/modified behaviors of storage implementations.

Some behaviors which need discussion, but not tackled in this PR.

Uh oh!

ytsmiling commented May 11, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

sile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toshihikoyanase left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toshihikoyanase left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ytsmiling commented Apr 30, 2020 •

edited

Loading

codecov-io commented May 12, 2020 •

edited

Loading