From b0ed82af7e779d4235e80babde6d0c28bc41c9b0 Mon Sep 17 00:00:00 2001 From: moririn2528 Date: Mon, 11 Sep 2023 16:31:39 +0900 Subject: [PATCH] fix by review --- optuna_dashboard/_app.py | 22 +++++++------- optuna_dashboard/_preferential_history.py | 29 ++++++++++--------- optuna_dashboard/_serializer.py | 4 +-- .../preferential/_system_attrs.py | 4 +-- python_tests/test_api.py | 14 ++------- python_tests/test_preferential_history.py | 11 +++---- 6 files changed, 38 insertions(+), 46 deletions(-) diff --git a/optuna_dashboard/_app.py b/optuna_dashboard/_app.py index 65bb9f34..f497145e 100644 --- a/optuna_dashboard/_app.py +++ b/optuna_dashboard/_app.py @@ -29,8 +29,9 @@ 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 NewHistory +from ._preferential_history import remove_history from ._preferential_history import report_history -from ._preferential_history import switching_history +from ._preferential_history import restore_history from ._rdb_migration import register_rdb_migration_route from ._serializer import serialize_study_detail from ._serializer import serialize_study_summary @@ -307,18 +308,17 @@ def create_app( response.status = 204 return {} - @app.put("/api/studies//preference/") + @app.delete("/api/studies//preference/") @json_api_view - def switch_preference(study_id: int, history_uuid: str) -> dict[str, Any]: - try: - enable = request.json.get("enable", None) - if enable is None or not isinstance(enable, bool): - raise ValueError - except ValueError: - response.status = 400 - return {"reason": "Invalid request."} - switching_history(study_id, storage, history_uuid, enable) + def remove_preference(study_id: int, history_uuid: str) -> dict[str, Any]: + remove_history(study_id, storage, history_uuid) + response.status = 204 + return {} + @app.post("/api/studies//preference/") + @json_api_view + def restore_preference(study_id: int, history_uuid: str) -> dict[str, Any]: + restore_history(study_id, storage, history_uuid) response.status = 204 return {} diff --git a/optuna_dashboard/_preferential_history.py b/optuna_dashboard/_preferential_history.py index a8fe2148..60fe6cb0 100644 --- a/optuna_dashboard/_preferential_history.py +++ b/optuna_dashboard/_preferential_history.py @@ -82,19 +82,20 @@ def report_history( return history_id -def switching_history(study_id: int, storage: BaseStorage, uuid: str, enable: bool) -> None: +def remove_history(study_id: int, storage: BaseStorage, uuid: str) -> None: system_attrs = storage.get_study_system_attrs(study_id) history: History = json.loads(system_attrs.get(_SYSTEM_ATTR_PREFIX_HISTORY + uuid, "")) - if enable: - preferences = [ - (best, history["clicked"]) - for best in history["candidates"] - if best != history["clicked"] - ] - storage.set_study_system_attr( - study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history["preference_id"], preferences - ) - else: # disable - storage.set_study_system_attr( - study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history["preference_id"], [] - ) + storage.set_study_system_attr( + study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history["preference_id"], [] + ) + + +def restore_history(study_id: int, storage: BaseStorage, uuid: str) -> None: + system_attrs = storage.get_study_system_attrs(study_id) + history: History = json.loads(system_attrs.get(_SYSTEM_ATTR_PREFIX_HISTORY + uuid, "")) + preferences = [ + (best, history["clicked"]) for best in history["candidates"] if best != history["clicked"] + ] + storage.set_study_system_attr( + study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history["preference_id"], preferences + ) diff --git a/optuna_dashboard/_serializer.py b/optuna_dashboard/_serializer.py index 35b9d6a7..9ce77fb3 100644 --- a/optuna_dashboard/_serializer.py +++ b/optuna_dashboard/_serializer.py @@ -18,7 +18,7 @@ from ._named_objectives import get_objective_names from ._preferential_history import _SYSTEM_ATTR_PREFIX_HISTORY from .artifact._backend import list_trial_artifacts from .preferential._study import _SYSTEM_ATTR_PREFERENTIAL_STUDY -from .preferential._system_attrs import is_preference_valid +from .preferential._system_attrs import is_preference_removed if TYPE_CHECKING: @@ -184,7 +184,7 @@ def serialize_preference_history( "timestamp": choice["timestamp"], "candidates": choice["candidates"], "clicked": choice["clicked"], - "enabled": is_preference_valid(system_attrs, choice["preference_id"]), + "is_removed": is_preference_removed(system_attrs, choice["preference_id"]), } histories.append(history) diff --git a/optuna_dashboard/preferential/_system_attrs.py b/optuna_dashboard/preferential/_system_attrs.py index 5ce83742..82ba61fa 100644 --- a/optuna_dashboard/preferential/_system_attrs.py +++ b/optuna_dashboard/preferential/_system_attrs.py @@ -44,10 +44,10 @@ def get_preferences(study_system_attrs: dict[str, Any]) -> list[tuple[int, int]] return preferences -def is_preference_valid(study_system_attrs: dict[str, Any], uuid: str) -> bool: +def is_preference_removed(study_system_attrs: dict[str, Any], uuid: str) -> bool: key = _SYSTEM_ATTR_PREFIX_PREFERENCE + uuid preference = study_system_attrs.get(key, []) - return len(preference) > 0 + return len(preference) == 0 def report_skip( diff --git a/python_tests/test_api.py b/python_tests/test_api.py index 22c60578..205bf966 100644 --- a/python_tests/test_api.py +++ b/python_tests/test_api.py @@ -234,12 +234,7 @@ class APITestCase(TestCase): status, _, _ = send_request( app, f"/api/studies/{study_id}/preference/{history_id}", - "PUT", - body=json.dumps( - { - "enable": False, - } - ), + "DELETE", content_type="application/json", ) self.assertEqual(status, 204) @@ -251,12 +246,7 @@ class APITestCase(TestCase): status, _, _ = send_request( app, f"/api/studies/{study_id}/preference/{history_id}", - "PUT", - body=json.dumps( - { - "enable": True, - } - ), + "POST", content_type="application/json", ) self.assertEqual(status, 204) diff --git a/python_tests/test_preferential_history.py b/python_tests/test_preferential_history.py index 75e622db..3ba54fd4 100644 --- a/python_tests/test_preferential_history.py +++ b/python_tests/test_preferential_history.py @@ -6,8 +6,9 @@ from typing import TYPE_CHECKING 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 from optuna_dashboard._preferential_history import report_history -from optuna_dashboard._preferential_history import switching_history +from optuna_dashboard._preferential_history import restore_history from optuna_dashboard._serializer import serialize_preference_history from optuna_dashboard.preferential import create_study from optuna_dashboard.preferential._system_attrs import _SYSTEM_ATTR_PREFIX_PREFERENCE @@ -86,18 +87,18 @@ def test_undo_redo_history(storage_supplier: Callable[[], StorageSupplier]) -> N storage=storage, input_data=NewHistory(mode="ChooseWorst", candidates=[0, 1, 2], clicked=1), ) - switching_history(study_id, storage, history_id, False) + remove_history(study_id, storage, history_id) preference, history = get_preferences_history(history_id) assert history["mode"] == "ChooseWorst" assert history["candidates"] == [0, 1, 2] assert history["clicked"] == 1 assert len(preference) == 0 - switching_history(study_id, storage, history_id, False) + remove_history(study_id, storage, history_id) preference, history = get_preferences_history(history_id) assert len(preference) == 0 - switching_history(study_id, storage, history_id, True) + restore_history(study_id, storage, history_id) preference, history = get_preferences_history(history_id) assert history["mode"] == "ChooseWorst" assert history["candidates"] == [0, 1, 2] @@ -108,6 +109,6 @@ def test_undo_redo_history(storage_supplier: Callable[[], StorageSupplier]) -> N assert preference[i][0] == best assert preference[i][1] == worst - switching_history(study_id, storage, history_id, True) + restore_history(study_id, storage, history_id) preference, history = get_preferences_history(history_id) assert len(preference) == 2