Fix request.values in `OptunaStorageProxyService` by hitsgub · Pull Request #6044 · optuna/optuna

hitsgub · 2025-04-14T10:25:50Z

Motivation

Recording None through a GrpcStorageProxy results in [] being recorded in backend DB.
Loading [] from the DB through GrpcStorageProxy results in None being loaded.
Saving and loading without GrpcStorageProxy does not change.
fix this so the value never changes

Description of the changes

First, add test to check to fail.
Next, fix bug.

c-bata · 2025-04-15T04:13:41Z

Let me mark this PR as a draft. Please mark as ready for review after CI passes.

hitsgub · 2025-04-15T09:14:06Z

Added tests for cases where set and get behave differently depending on whether a gRPC proxy is present.
The current implementation fails as expected.

=========================== short test summary info ============================
FAILED tests/storages_tests/test_storages.py::test_set_and_get_trial_state_values_through_grpc_proxy[None-inmemory] - assert [] == None
 +  where [] = FrozenTrial(number=1, state=TrialState.COMPLETE, values=[], datetime_start=datetime.datetime(2025, 4, 15, 9, 0, 45, 19..., 201376), params={}, user_attrs={}, system_attrs={}, intermediate_values={}, distributions={}, trial_id=1, value=None).values
 +    where FrozenTrial(number=1, state=TrialState.COMPLETE, values=[], datetime_start=datetime.datetime(2025, 4, 15, 9, 0, 45, 19..., 201376), params={}, user_attrs={}, system_attrs={}, intermediate_values={}, distributions={}, trial_id=1, value=None) = get_trial(1)
 +      where get_trial = <optuna.storages._in_memory.InMemoryStorage object at 0x7f71c7cb4b80>.get_trial
FAILED tests/storages_tests/test_storages.py::test_set_and_get_trial_state_values_through_grpc_proxy[None-journal] - assert [] == None
 +  where [] = FrozenTrial(number=1, state=TrialState.COMPLETE, values=[], datetime_start=datetime.datetime(2025, 4, 15, 9, 0, 46, 71..., 714433), params={}, user_attrs={}, system_attrs={}, intermediate_values={}, distributions={}, trial_id=1, value=None).values
 +    where FrozenTrial(number=1, state=TrialState.COMPLETE, values=[], datetime_start=datetime.datetime(2025, 4, 15, 9, 0, 46, 71..., 714433), params={}, user_attrs={}, system_attrs={}, intermediate_values={}, distributions={}, trial_id=1, value=None) = get_trial(1)
 +      where get_trial = <optuna.storages.journal._storage.JournalStorage object at 0x7f718f2c4880>.get_trial
FAILED tests/storages_tests/test_storages.py::test_set_and_get_trial_state_values_through_grpc_proxy[None-journal_redis] - assert [] == None
 +  where [] = FrozenTrial(number=1, state=TrialState.COMPLETE, values=[], datetime_start=datetime.datetime(2025, 4, 15, 9, 0, 46, 95..., 960024), params={}, user_attrs={}, system_attrs={}, intermediate_values={}, distributions={}, trial_id=1, value=None).values
 +    where FrozenTrial(number=1, state=TrialState.COMPLETE, values=[], datetime_start=datetime.datetime(2025, 4, 15, 9, 0, 46, 95..., 960024), params={}, user_attrs={}, system_attrs={}, intermediate_values={}, distributions={}, trial_id=1, value=None) = get_trial(1)
 +      where get_trial = <optuna.storages.journal._storage.JournalStorage object at 0x7f718f2ca370>.get_trial
= 3 failed, 4821 passed, 121 skipped, 36 deselected, 13 warnings in 744.66s (0:12:24) =
Error: Process completed with exit code 1.

c-bata · 2025-04-16T01:40:42Z

@not522 @sawa3030 Could you review this PR?

not522

Thank you for your PR!
It seems to be many changes for tests. Could you narrow down the changes to those related to bug fixes?

not522 · 2025-04-16T06:37:25Z

tests/storages_tests/test_storages.py

+
+@pytest.mark.parametrize("storage_mode_direct", STORAGE_MODES_DIRECT)
+@pytest.mark.parametrize("values", [None, [0.0]])
+def test_set_and_get_trial_state_values_through_grpc_proxy(


I think that this test should be in tests/storages_tests/test_grpc.py.

hitsgub · 2025-04-16T09:03:41Z

ok

tests/storages_tests/test_cached_storage.py .....                        [ 46%]
tests/storages_tests/test_callbacks.py ...                               [ 46%]
tests/storages_tests/test_grpc.py F..FF.........                         [ 46%]
tests/storages_tests/test_heartbeat.py ..........................        [ 47%]
tests/storages_tests/test_storages.py .................................. [ 47%]
........................................................................ [ 49%]
........................................................................ [ 50%]
..........ss.....ss..................................................... [ 52%]
........................................................................ [ 53%]
..............s.........                                                 [ 54%]

tests/storages_tests/test_grpc.py

Co-authored-by: Eri Sawada <125344906+sawa3030@users.noreply.github.com>

hitsgub · 2025-04-22T07:04:08Z

actions workflows have not been running...
I will close and reopen to try to run workflows

not522

LGTM!

c-bata

Thank you for your pull request. Let me leave some early feedback comments.

tests/storages_tests/test_grpc.py

c-bata · 2025-04-24T10:15:40Z

optuna/testing/storages.py

+    ) -> None:
        self.storage_specifier = storage_specifier
+        if base_storage is not None:
+            assert storage_specifier == "grpc_proxy"


My initial impression is that it could potentially be a bit confusing that storage_specifiers accepts "grpc_proxy", even though "grpc_rdb" and "grpc_journal_file" still exist. I have no idea how to mitigate this right now. Therefore, I'll comment later with a suggested solution.

I've been thinking about it, but I could not come up with a simple solution.

It seems that larger changes might be needed, which should not be addressed in this PR. Considering that this bug fix should be included in the next minor release, I will merge this PR first and then considering whether to submit a follow-up PR.

I apologize for the delayed review. I will proceed with merging this.

c-bata · 2025-04-25T02:36:27Z

optuna/testing/storages.py

+    ) -> None:
        self.storage_specifier = storage_specifier
+        if base_storage is not None:
+            assert storage_specifier == "grpc_proxy"


I've been thinking about it, but I could not come up with a simple solution.

It seems that larger changes might be needed, which should not be addressed in this PR. Considering that this bug fix should be included in the next minor release, I will merge this PR first and then considering whether to submit a follow-up PR.

I apologize for the delayed review. I will proceed with merging this.

c-bata · 2025-04-25T02:40:53Z

@sawa3030 please let me know once you have completed your review.

sawa3030 · 2025-04-25T05:02:35Z

Apologies for the delay. I am considering making only minimal changes to StorageSupplier. How about putting base_storage to kwargs?

class StorageSupplier:
    def __init__(
        self, storage_specifier: str, **kwargs: Any
    ) -> None:
        self.storage_specifier = storage_specifier
        self.extra_args = kwargs
        self.tempfile: IO[Any] | None = None
        self.server: grpc.Server | None = None
        self.thread: threading.Thread | None = None
        self.proxy: GrpcStorageProxy | None = None

    def __enter__(
        self,
    ) -> (
        optuna.storages.InMemoryStorage
        | optuna.storages._CachedStorage
        | optuna.storages.RDBStorage
        | optuna.storages.JournalStorage
        | optuna.storages.GrpcStorageProxy
    ):
        if self.storage_specifier == "grpc_proxy":
            base_storage = self.extra_args.get("base_storage")
            assert isinstance(base_storage, BaseStorage)
            return self._create_proxy(base_storage)
        elif self.storage_specifier == "inmemory":

sawa3030

LGTM!

hitsgub added 3 commits April 14, 2025 10:20

add test

067535e

fix test

d4e4fda

fix test

bb3b8c8

hitsgub force-pushed the fix-grpc-values branch from f362bea to bb3b8c8 Compare April 15, 2025 01:40

hitsgub added 3 commits April 15, 2025 02:40

fix test

0f77292

trigger

9e6ef4e

fix test

55337fe

c-bata marked this pull request as draft April 15, 2025 04:13

hitsgub added 3 commits April 15, 2025 06:12

fix test

0be52bf

Merge branch 'master' into fix-grpc-values

dee5911

debug

d6d65d5

c-bata added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Apr 15, 2025

hitsgub added 4 commits April 15, 2025 07:10

debug

77059cc

debug

99c0341

fix test

370f2e8

fix

1c6a67a

fix implementation

2715351

hitsgub marked this pull request as ready for review April 15, 2025 09:52

c-bata assigned not522 and c-bata Apr 16, 2025

not522 reviewed Apr 16, 2025

View reviewed changes

hitsgub added 2 commits April 16, 2025 08:38

reset

601998c

add test

0a3cf7f

fix implementation

5d38b93

hitsgub mentioned this pull request Apr 17, 2025

Introduce server-side TTL cache in GrpcStorageProxy #6023

Closed

sawa3030 reviewed Apr 22, 2025

View reviewed changes

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

hitsgub and others added 2 commits April 22, 2025 15:32

Update tests/storages_tests/test_grpc.py

743c548

Co-authored-by: Eri Sawada <125344906+sawa3030@users.noreply.github.com>

Merge branch 'master' into fix-grpc-values

7e1907c

hitsgub closed this Apr 22, 2025

hitsgub reopened this Apr 22, 2025

fix

cf9847a

hitsgub requested a review from not522 April 22, 2025 07:37

not522 approved these changes Apr 23, 2025

View reviewed changes

not522 removed their assignment Apr 23, 2025

c-bata added this to the v4.4.0 milestone Apr 24, 2025

c-bata reviewed Apr 24, 2025

View reviewed changes

c-bata approved these changes Apr 25, 2025

View reviewed changes

hitsgub added 2 commits April 25, 2025 09:06

move base_storage

2cbf513

fix

be52fc5

sawa3030 approved these changes Apr 28, 2025

View reviewed changes

HideakiImamura merged commit 3fd8e6f into optuna:master May 1, 2025
14 checks passed

Uh oh!

Conversation

hitsgub commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description of the changes

Uh oh!

c-bata commented Apr 15, 2025

Uh oh!

hitsgub commented Apr 15, 2025

Uh oh!

c-bata commented Apr 16, 2025

Uh oh!

not522 left a comment

Choose a reason for hiding this comment

Uh oh!

not522 Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

hitsgub commented Apr 16, 2025

Uh oh!

Uh oh!

hitsgub commented Apr 22, 2025

Uh oh!

not522 left a comment

Choose a reason for hiding this comment

Uh oh!

c-bata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

c-bata Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

c-bata Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

c-bata Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

c-bata commented Apr 25, 2025

Uh oh!

sawa3030 commented Apr 25, 2025

Uh oh!

sawa3030 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hitsgub commented Apr 14, 2025 •

edited

Loading