`BaseStorage.set_trial_param` to return `None` instead of `bool`. #1327

hvy · 2020-06-05T00:44:12Z

Motivation

Description of the changes

Fixes #1308 and the following.

The storage always sets, i.e the distribution compatibility check is removed. The check is performed by the trial.
Simplified Trial by removing _set_new_param_or_get_existing.

hvy · 2020-06-05T00:44:47Z

tests/test_trial.py

+    assert trial.suggest_float("h", 0.5, 0.5, step=1.0) == 0.5  # Suggesting a param.
+    assert trial.suggest_float("h", 0.5, 0.5, step=1.0) == 0.5  # Suggesting the same param.
+    assert trial.suggest_int("i", 1, 1, log=True) == 1  # Suggesting a param.
+    assert trial.suggest_int("i", 1, 1, log=True) == 1  # Suggesting the same param.


Maybe we can remove this test function altogether.

I found it useful to make sure that sampler.sample_independent is not called. If you don't want to add the check, I think you can remove this test function.
If you remove this test, I'm not sure if there is a test that calls the distributions._get_single_value () method. I want to make sure that tests for the method are added when you remove this test.

Right, thanks for you suggestion. I changed this test to check the number of calls to distributions._get_single_value. What do you think?

codecov-commenter · 2020-06-05T01:23:58Z

Codecov Report

Merging #1327 into master will increase coverage by 1.16%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master    #1327      +/-   ##
==========================================
+ Coverage   86.69%   87.85%   +1.16%     
==========================================
  Files          95       98       +3     
  Lines        7412     7339      -73     
==========================================
+ Hits         6426     6448      +22     
+ Misses        986      891      -95

Impacted Files	Coverage Δ
optuna/storages/base.py	`67.59% <ø> (ø)`
optuna/storages/in_memory.py	`97.66% <ø> (-0.08%)`	⬇️
optuna/distributions.py	`93.68% <85.71%> (-0.31%)`	⬇️
optuna/storages/cached_storage.py	`96.44% <100.00%> (-0.04%)`	⬇️
optuna/storages/rdb/storage.py	`93.66% <100.00%> (-0.08%)`	⬇️
optuna/storages/redis.py	`94.13% <100.00%> (+1.02%)`	⬆️
optuna/trial/_trial.py	`92.10% <100.00%> (+0.49%)`	⬆️
optuna/visualization/slice.py	`98.18% <0.00%> (-0.04%)`	⬇️
... and 38 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 1c1b4ca...033b6e0. Read the comment docs.

ytsmiling

Thank you for creating this PR.
Please correct me if it's wrong but does this PR remove distribution compatibility check across trials (both within a single worker and across multiple workers)? If yes, I think this point needs discussion.

hvy · 2020-06-05T04:34:01Z

You are right, we should probably not change this behavior. Do you think we should put the check back to the storage? IMO, we can move this check to the trial to simplify the storage logic since it's possible with the current storage API. However, it might be suboptimal (if there are lots of conditionals) and we'll probably want some other storage API to optimize this check in follow-ups.

hvy · 2020-06-05T06:17:29Z

Let me fix the code so that the logic isn't altered for the case you described. I'll add tests on the trial layer as well.

hvy · 2020-06-05T08:59:07Z

PTAL. I moved the distribution compatibility back to the storage from the trial. The reason being that we cannot keep the same behavior explained by @ytsmiling above otherwise. If we try to check for compatibility in the trial, we cannot guarantee the expected behavior in a distributed environment where all initial trials sample the same parameter (having the same name) but with different distributions as they're exposed to race condition when writing the sampled parameter back to the storage.

ytsmiling

Thank you for addressing #1308. This PR looks almost good, but I left a minor comment and I'd like you to check that.

ytsmiling · 2020-06-07T23:37:58Z

tests/test_trial.py

+    assert trial.suggest_float("h", 0.5, 0.5, step=1.0) == 0.5  # Suggesting a param.
+    assert trial.suggest_float("h", 0.5, 0.5, step=1.0) == 0.5  # Suggesting the same param.
+    assert trial.suggest_int("i", 1, 1, log=True) == 1  # Suggesting a param.
+    assert trial.suggest_int("i", 1, 1, log=True) == 1  # Suggesting the same param.


I found it useful to make sure that sampler.sample_independent is not called. If you don't want to add the check, I think you can remove this test function.
If you remove this test, I'm not sure if there is a test that calls the distributions._get_single_value () method. I want to make sure that tests for the method are added when you remove this test.

hvy · 2020-06-08T04:17:09Z

Right, thanks for the suggestion about the test. I changes it to count the number of calls to distributions._get_single_value. What do you think? It's not the most beautiful code but I think it's acceptable for this test.

PTAL.

ytsmiling

Thank you for the quick fix. LGTM!

toshihikoyanase

It basically looks good to me. I add a comment about a test case, but I think it is optional.

toshihikoyanase · 2020-06-10T00:15:13Z

optuna/trial/_trial.py

+            if self._is_fixed_param(name, distribution):
+                param_value = storage.get_trial_system_attrs(trial_id)["fixed_params"][name]
+            elif distribution.single():
+                param_value = distributions._get_single_value(distribution)


This is optional, but how about adding a test case to raise warnings if single-value distributions are given with fixed params that are out of ranges? This code correctly handles such a case, and I propose it for future development.

Thank you for the suggestion. I added tests for enqueue_trial since it's relevant (although a bit stretched) to this PR.

hvy · 2020-06-10T00:42:15Z

PTAL.

toshihikoyanase

Thank you for your swift action! LGTM.

hvy added 2 commits June 4, 2020 23:41

Simplify trial for suggestions

73270fd

Remove return value from set_trial_param

00a2e11

hvy added compatibility Change that breaks compatibility. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. labels Jun 5, 2020

hvy commented Jun 5, 2020

View reviewed changes

ytsmiling self-requested a review June 5, 2020 00:52

Reduce storage access

308b49b

ytsmiling reviewed Jun 5, 2020

View reviewed changes

ytsmiling self-requested a review June 5, 2020 03:56

hvy changed the title ~~BaseStorage.set_trial_param to return None instead of bool.~~ [WIP] BaseStorage.set_trial_param to return None instead of bool. Jun 5, 2020

Add distribution check back to storage

d463f3b

hvy force-pushed the set-trial-param-no-return branch from 710032a to d463f3b Compare June 5, 2020 08:06

hvy changed the title ~~[WIP] BaseStorage.set_trial_param to return None instead of bool.~~ BaseStorage.set_trial_param to return None instead of bool. Jun 5, 2020

ytsmiling reviewed Jun 7, 2020

View reviewed changes

Test calls distributions._get_single_value

0d0d4d2

ytsmiling approved these changes Jun 8, 2020

View reviewed changes

toshihikoyanase self-assigned this Jun 9, 2020

toshihikoyanase reviewed Jun 10, 2020

View reviewed changes

Add missing tests for enqueue_trial

033b6e0

toshihikoyanase approved these changes Jun 10, 2020

View reviewed changes

toshihikoyanase merged commit b5d680a into optuna:master Jun 10, 2020

toshihikoyanase added this to the v2.0.0 milestone Jun 10, 2020

hvy deleted the set-trial-param-no-return branch June 10, 2020 02:03

ytsmiling mentioned this pull request Jul 13, 2020

Retry after insertion into RDB-storage. #1509

Closed

ytsmiling mentioned this pull request Jul 14, 2020

Major refactoring of storage classes. #1170

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`BaseStorage.set_trial_param` to return `None` instead of `bool`. #1327

`BaseStorage.set_trial_param` to return `None` instead of `bool`. #1327

hvy commented Jun 5, 2020

hvy Jun 5, 2020

ytsmiling Jun 7, 2020

hvy Jun 8, 2020

codecov-commenter commented Jun 5, 2020 •

edited

ytsmiling left a comment

hvy commented Jun 5, 2020 •

edited

hvy commented Jun 5, 2020

hvy commented Jun 5, 2020

ytsmiling left a comment

ytsmiling Jun 7, 2020

hvy commented Jun 8, 2020

ytsmiling left a comment

toshihikoyanase left a comment

toshihikoyanase Jun 10, 2020

hvy Jun 10, 2020

hvy commented Jun 10, 2020

toshihikoyanase left a comment

BaseStorage.set_trial_param to return None instead of bool. #1327

BaseStorage.set_trial_param to return None instead of bool. #1327

Conversation

hvy commented Jun 5, 2020

Motivation

Description of the changes

hvy Jun 5, 2020

Choose a reason for hiding this comment

ytsmiling Jun 7, 2020

Choose a reason for hiding this comment

hvy Jun 8, 2020

Choose a reason for hiding this comment

codecov-commenter commented Jun 5, 2020 • edited

Codecov Report

ytsmiling left a comment

Choose a reason for hiding this comment

hvy commented Jun 5, 2020 • edited

hvy commented Jun 5, 2020

hvy commented Jun 5, 2020

ytsmiling left a comment

Choose a reason for hiding this comment

ytsmiling Jun 7, 2020

Choose a reason for hiding this comment

hvy commented Jun 8, 2020

ytsmiling left a comment

Choose a reason for hiding this comment

toshihikoyanase left a comment

Choose a reason for hiding this comment

toshihikoyanase Jun 10, 2020

Choose a reason for hiding this comment

hvy Jun 10, 2020

Choose a reason for hiding this comment

hvy commented Jun 10, 2020

toshihikoyanase left a comment

Choose a reason for hiding this comment

`BaseStorage.set_trial_param` to return `None` instead of `bool`. #1327

`BaseStorage.set_trial_param` to return `None` instead of `bool`. #1327

codecov-commenter commented Jun 5, 2020 •

edited

hvy commented Jun 5, 2020 •

edited