From ba82fadad9d735684c9cf3879951407a835c6599 Mon Sep 17 00:00:00 2001 From: moririn2528 Date: Tue, 12 Sep 2023 17:43:23 +0900 Subject: [PATCH] fix by review --- optuna_dashboard/_app.py | 15 ++++- optuna_dashboard/_preferential_history.py | 32 ++++++++-- optuna_dashboard/_serializer.py | 20 ++++-- .../preferential/_system_attrs.py | 4 +- python_tests/test_api.py | 48 ++++++++++---- python_tests/test_preferential_history.py | 64 +++++++++++++------ 6 files changed, 132 insertions(+), 51 deletions(-) diff --git a/optuna_dashboard/_app.py b/optuna_dashboard/_app.py index a3ab595a..a020c8b8 100644 --- a/optuna_dashboard/_app.py +++ b/optuna_dashboard/_app.py @@ -28,6 +28,7 @@ from ._cached_extra_study_property import get_cached_extra_study_property from ._custom_plot_data import get_plotly_graph_objects from ._importance import get_param_importance_from_trials_cache from ._pareto_front import get_pareto_front_trials +from ._preferential_history import HistoryIdError from ._preferential_history import NewHistory from ._preferential_history import remove_history from ._preferential_history import report_history @@ -311,14 +312,24 @@ def create_app( @app.delete("/api/studies//preference/") @json_api_view def remove_preference(study_id: int, history_id: str) -> dict[str, Any]: - remove_history(study_id, storage, history_id) + try: + remove_history(study_id, storage, history_id) + except HistoryIdError: + response.status = 404 + return {"reason": f"history_id={history_id} is not found"} + response.status = 204 return {} @app.post("/api/studies//preference/") @json_api_view def restore_preference(study_id: int, history_id: str) -> dict[str, Any]: - restore_history(study_id, storage, history_id) + try: + restore_history(study_id, storage, history_id) + except HistoryIdError: + response.status = 404 + return {"reason": f"history_id={history_id} is not found"} + response.status = 204 return {} diff --git a/optuna_dashboard/_preferential_history.py b/optuna_dashboard/_preferential_history.py index 3d912190..9726d27c 100644 --- a/optuna_dashboard/_preferential_history.py +++ b/optuna_dashboard/_preferential_history.py @@ -30,6 +30,17 @@ if TYPE_CHECKING: }, ) History = ChooseWorstHistory + SerializedHistory = TypedDict( + "SerializedHistory", + { + "history": History, + "is_removed": bool, + }, + ) + + +class HistoryIdError(Exception): + pass @dataclass @@ -80,13 +91,20 @@ def report_history( return id -def remove_history(study_id: int, storage: BaseStorage, id: str) -> None: - storage.set_study_system_attr(study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + id, []) - - -def restore_history(study_id: int, storage: BaseStorage, id: str) -> None: +def remove_history(study_id: int, storage: BaseStorage, history_id: str) -> None: system_attrs = storage.get_study_system_attrs(study_id) - history: History = json.loads(system_attrs.get(_SYSTEM_ATTR_PREFIX_HISTORY + id, "")) + history_key = _SYSTEM_ATTR_PREFIX_HISTORY + history_id + if history_key not in system_attrs: + raise HistoryIdError + storage.set_study_system_attr(study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history_id, []) + + +def restore_history(study_id: int, storage: BaseStorage, history_id: str) -> None: + system_attrs = storage.get_study_system_attrs(study_id) + history_key = _SYSTEM_ATTR_PREFIX_HISTORY + history_id + if history_key not in system_attrs: + raise HistoryIdError + history: History = json.loads(system_attrs.get(history_key, "")) storage.set_study_system_attr( - study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history["id"], history["preferences"] + study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history_id, history["preferences"] ) diff --git a/optuna_dashboard/_serializer.py b/optuna_dashboard/_serializer.py index 8388c736..001f689a 100644 --- a/optuna_dashboard/_serializer.py +++ b/optuna_dashboard/_serializer.py @@ -26,6 +26,9 @@ if TYPE_CHECKING: from typing import Literal from typing import TypedDict + from ._preferential_history import History + from ._preferential_history import SerializedHistory + Attribute = TypedDict( "Attribute", { @@ -172,24 +175,29 @@ def serialize_study_detail( def serialize_preference_history( system_attrs: dict[str, Any], -) -> list[dict[str, Any]]: - histories: list[dict[str, Any]] = [] +) -> list[SerializedHistory]: + histories: list[SerializedHistory] = [] for k, v in system_attrs.items(): if not k.startswith(_SYSTEM_ATTR_PREFIX_HISTORY): continue choice: dict[str, Any] = json.loads(v) if choice["mode"] == "ChooseWorst": - history = { + history: History = { "mode": "ChooseWorst", "id": choice["id"], "timestamp": choice["timestamp"], "candidates": choice["candidates"], "clicked": choice["clicked"], - "is_removed": is_preference_removed(system_attrs, choice["id"]), + "preferences": choice["preferences"], } - histories.append(history) + histories.append( + { + "history": history, + "is_removed": is_preference_removed(system_attrs, choice["id"]), + } + ) - histories.sort(key=lambda c: datetime.fromisoformat(c["timestamp"])) + histories.sort(key=lambda c: datetime.fromisoformat(c["history"]["timestamp"])) return histories diff --git a/optuna_dashboard/preferential/_system_attrs.py b/optuna_dashboard/preferential/_system_attrs.py index 33d56f30..438b411c 100644 --- a/optuna_dashboard/preferential/_system_attrs.py +++ b/optuna_dashboard/preferential/_system_attrs.py @@ -44,8 +44,8 @@ def get_preferences(study_system_attrs: dict[str, Any]) -> list[tuple[int, int]] return preferences -def is_preference_removed(study_system_attrs: dict[str, Any], id: str) -> bool: - key = _SYSTEM_ATTR_PREFIX_PREFERENCE + id +def is_preference_removed(study_system_attrs: dict[str, Any], preference_id: str) -> bool: + key = _SYSTEM_ATTR_PREFIX_PREFERENCE + preference_id preference = study_system_attrs.get(key, []) return len(preference) == 0 diff --git a/python_tests/test_api.py b/python_tests/test_api.py index 6075ffa5..afe5a31c 100644 --- a/python_tests/test_api.py +++ b/python_tests/test_api.py @@ -8,6 +8,9 @@ from optuna import get_all_study_summaries from optuna.study import StudyDirection from optuna_dashboard._app import create_app from optuna_dashboard._app import create_new_study +from optuna_dashboard._preferential_history import NewHistory +from optuna_dashboard._preferential_history import remove_history +from optuna_dashboard._preferential_history import report_history from optuna_dashboard._serializer import serialize_preference_history from optuna_dashboard.preferential import create_study @@ -204,7 +207,7 @@ class APITestCase(TestCase): assert len(best_trials) == 1 assert best_trials[0].number == 2 - def test_undo_redo_history(self) -> None: + def test_remove_history(self) -> None: storage = optuna.storages.InMemoryStorage() study = create_study(storage=storage, n_generate=3) for _ in range(3): @@ -212,25 +215,19 @@ class APITestCase(TestCase): app = create_app(storage) study_id = study._study._study_id - status, _, _ = send_request( - app, - f"/api/studies/{study_id}/preference", - "POST", - body=json.dumps( - { - "mode": "ChooseWorst", - "candidates": [0, 1, 2], - "clicked": 2, - } + history_id = report_history( + study_id, + storage, + NewHistory( + mode="ChooseWorst", + candidates=[0, 1, 2], + clicked=2, ), - content_type="application/json", ) - self.assertEqual(status, 204) histories = serialize_preference_history(storage.get_study_system_attrs(study_id)) assert len(histories) == 1 assert not histories[0]["is_removed"] - history_id = histories[0]["id"] status, _, _ = send_request( app, f"/api/studies/{study_id}/preference/{history_id}", @@ -243,6 +240,29 @@ class APITestCase(TestCase): assert histories[0]["is_removed"] assert len(study.get_preferences()) == 0 + def test_restore_history(self) -> None: + storage = optuna.storages.InMemoryStorage() + study = create_study(storage=storage, n_generate=3) + for _ in range(3): + study.ask() + + app = create_app(storage) + study_id = study._study._study_id + history_id = report_history( + study_id, + storage, + NewHistory( + mode="ChooseWorst", + candidates=[0, 1, 2], + clicked=2, + ), + ) + remove_history(study_id, storage, history_id) + histories = serialize_preference_history(storage.get_study_system_attrs(study_id)) + assert len(histories) == 1 + assert histories[0]["is_removed"] + assert len(study.get_preferences()) == 0 + status, _, _ = send_request( app, f"/api/studies/{study_id}/preference/{history_id}", diff --git a/python_tests/test_preferential_history.py b/python_tests/test_preferential_history.py index c64f448c..3d1a7d70 100644 --- a/python_tests/test_preferential_history.py +++ b/python_tests/test_preferential_history.py @@ -4,6 +4,7 @@ import json from typing import Callable from typing import TYPE_CHECKING +from optuna.storages import BaseStorage from optuna_dashboard._preferential_history import _SYSTEM_ATTR_PREFIX_HISTORY from optuna_dashboard._preferential_history import NewHistory from optuna_dashboard._preferential_history import remove_history @@ -44,17 +45,17 @@ def test_report_and_get_choices(storage_supplier: Callable[[], StorageSupplier]) history = serialize_preference_history(storage.get_study_system_attrs(study_id)) sys_attrs = storage.get_study_system_attrs(study_id) assert len(history) == 2 - assert history[0]["candidates"] == [0, 1, 2] - assert history[0]["clicked"] == 1 - preferences = sys_attrs[_SYSTEM_ATTR_PREFIX_PREFERENCE + history[0]["id"]] + assert history[0]["history"]["candidates"] == [0, 1, 2] + assert history[0]["history"]["clicked"] == 1 + preferences = sys_attrs[_SYSTEM_ATTR_PREFIX_PREFERENCE + history[0]["history"]["id"]] assert len(preferences) == 2 for i, (best, worst) in enumerate([(0, 1), (2, 1)]): assert len(preferences[i]) == 2 assert preferences[i][0] == best assert preferences[i][1] == worst - assert history[1]["candidates"] == [0, 2, 3, 4] - assert history[1]["clicked"] == 0 - preferences = sys_attrs[_SYSTEM_ATTR_PREFIX_PREFERENCE + history[1]["id"]] + assert history[1]["history"]["candidates"] == [0, 2, 3, 4] + assert history[1]["history"]["clicked"] == 0 + preferences = sys_attrs[_SYSTEM_ATTR_PREFIX_PREFERENCE + history[1]["history"]["id"]] assert len(preferences) == 3 for i, (best, worst) in enumerate([(2, 0), (3, 0), (4, 0)]): assert len(preferences[i]) == 2 @@ -62,42 +63,65 @@ def test_report_and_get_choices(storage_supplier: Callable[[], StorageSupplier]) assert preferences[i][1] == worst +def get_preferences_history( + study_id: int, + storage: BaseStorage, + history_id: str, +) -> tuple[list[tuple[int, int]], History]: + system_attrs = storage.get_study_system_attrs(study_id) + history: History = json.loads(system_attrs.get(_SYSTEM_ATTR_PREFIX_HISTORY + history_id, "")) + preference: list[tuple[int, int]] = system_attrs.get( + _SYSTEM_ATTR_PREFIX_PREFERENCE + history_id, [] + ) + return preference, history + + @parametrize_storages -def test_undo_redo_history(storage_supplier: Callable[[], StorageSupplier]) -> None: +def test_remove_history(storage_supplier: Callable[[], StorageSupplier]) -> None: with storage_supplier() as storage: study = create_study(storage=storage, n_generate=5) for _ in range(5): trial = study.ask() trial.suggest_float("x", 0, 1) - study_id = study._study._study_id - def get_preferences_history(id: str) -> tuple[list[tuple[int, int]], History]: - system_attrs = storage.get_study_system_attrs(study_id) - history: History = json.loads(system_attrs.get(_SYSTEM_ATTR_PREFIX_HISTORY + id, "")) - preference: list[tuple[int, int]] = system_attrs.get( - _SYSTEM_ATTR_PREFIX_PREFERENCE + id, [] - ) - return preference, history - history_id = report_history( study_id=study_id, storage=storage, input_data=NewHistory(mode="ChooseWorst", candidates=[0, 1, 2], clicked=1), ) remove_history(study_id, storage, history_id) - preference, history = get_preferences_history(history_id) + preference, history = get_preferences_history(study_id, storage, history_id) assert history["mode"] == "ChooseWorst" assert history["candidates"] == [0, 1, 2] assert history["clicked"] == 1 assert len(preference) == 0 remove_history(study_id, storage, history_id) - preference, history = get_preferences_history(history_id) + preference, history = get_preferences_history(study_id, storage, history_id) + assert len(preference) == 0 + + +@parametrize_storages +def test_restore_history(storage_supplier: Callable[[], StorageSupplier]) -> None: + with storage_supplier() as storage: + study = create_study(storage=storage, n_generate=5) + for _ in range(5): + trial = study.ask() + trial.suggest_float("x", 0, 1) + study_id = study._study._study_id + + history_id = report_history( + study_id=study_id, + storage=storage, + input_data=NewHistory(mode="ChooseWorst", candidates=[0, 1, 2], clicked=1), + ) + remove_history(study_id, storage, history_id) + preference, history = get_preferences_history(study_id, storage, history_id) assert len(preference) == 0 restore_history(study_id, storage, history_id) - preference, history = get_preferences_history(history_id) + preference, history = get_preferences_history(study_id, storage, history_id) assert history["mode"] == "ChooseWorst" assert history["candidates"] == [0, 1, 2] assert history["clicked"] == 1 @@ -108,5 +132,5 @@ def test_undo_redo_history(storage_supplier: Callable[[], StorageSupplier]) -> N assert preference[i][1] == worst restore_history(study_id, storage, history_id) - preference, history = get_preferences_history(history_id) + preference, history = get_preferences_history(study_id, storage, history_id) assert len(preference) == 2