From f9aa926a97e1f1481cc36f8169bd0f862b9c1106 Mon Sep 17 00:00:00 2001 From: moririn2528 Date: Tue, 12 Sep 2023 11:41:18 +0900 Subject: [PATCH] fix by review --- optuna_dashboard/_app.py | 12 ++-- optuna_dashboard/_preferential_history.py | 40 +++++------ optuna_dashboard/_serializer.py | 3 +- .../preferential/_system_attrs.py | 4 +- optuna_dashboard/ts/action.ts | 23 ++++--- optuna_dashboard/ts/apiClient.ts | 23 ++++--- .../ts/components/PreferenceHistory.tsx | 44 ++++++++----- .../ts/components/PreferentialTrials.tsx | 66 +++++++++---------- optuna_dashboard/ts/types/index.d.ts | 3 +- python_tests/test_api.py | 6 +- python_tests/test_preferential_history.py | 12 ++-- 11 files changed, 125 insertions(+), 111 deletions(-) diff --git a/optuna_dashboard/_app.py b/optuna_dashboard/_app.py index f497145e..a3ab595a 100644 --- a/optuna_dashboard/_app.py +++ b/optuna_dashboard/_app.py @@ -308,17 +308,17 @@ def create_app( response.status = 204 return {} - @app.delete("/api/studies//preference/") + @app.delete("/api/studies//preference/") @json_api_view - def remove_preference(study_id: int, history_uuid: str) -> dict[str, Any]: - remove_history(study_id, storage, history_uuid) + def remove_preference(study_id: int, history_id: str) -> dict[str, Any]: + remove_history(study_id, storage, history_id) response.status = 204 return {} - @app.post("/api/studies//preference/") + @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) + def restore_preference(study_id: int, history_id: str) -> dict[str, Any]: + restore_history(study_id, storage, history_id) response.status = 204 return {} diff --git a/optuna_dashboard/_preferential_history.py b/optuna_dashboard/_preferential_history.py index 60fe6cb0..c8acf969 100644 --- a/optuna_dashboard/_preferential_history.py +++ b/optuna_dashboard/_preferential_history.py @@ -24,10 +24,10 @@ if TYPE_CHECKING: { "mode": FeedbackMode, "id": str, - "preference_id": str, "timestamp": str, "candidates": list[int], "clicked": int, + "preferences": list[tuple[int, int]], }, ) History = ChooseWorstHistory @@ -49,53 +49,45 @@ def report_history( # TODO(moririn): Use TypeGuard after adding other history types. if input_data.mode == "ChooseWorst": preferences = [ - (best, input_data.clicked) - for best in input_data.candidates - if best != input_data.clicked + (better, input_data.clicked) + for better in input_data.candidates + if better != input_data.clicked ] else: assert False, f"Unknown data: {input_data}" - preference_id = report_preferences( + id = report_preferences( study_id=study_id, storage=storage, preferences=preferences, ) - history_id = str(uuid.uuid4()) if input_data.mode == "ChooseWorst": history: ChooseWorstHistory = { "mode": "ChooseWorst", - "id": history_id, - "preference_id": preference_id, + "id": id, "timestamp": datetime.now().isoformat(), "candidates": input_data.candidates, "clicked": input_data.clicked, + "preferences": preferences, } - key = _SYSTEM_ATTR_PREFIX_HISTORY + history_id + key = _SYSTEM_ATTR_PREFIX_HISTORY + id storage.set_study_system_attr( study_id=study_id, key=key, value=json.dumps(history), ) - return history_id + return id -def remove_history(study_id: int, storage: BaseStorage, uuid: str) -> None: +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: system_attrs = storage.get_study_system_attrs(study_id) - history: History = json.loads(system_attrs.get(_SYSTEM_ATTR_PREFIX_HISTORY + uuid, "")) + history: History = json.loads(system_attrs.get(_SYSTEM_ATTR_PREFIX_HISTORY + 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 + study_id, _SYSTEM_ATTR_PREFIX_PREFERENCE + history["id"], history["preferences"] ) diff --git a/optuna_dashboard/_serializer.py b/optuna_dashboard/_serializer.py index 9ce77fb3..d4ff5608 100644 --- a/optuna_dashboard/_serializer.py +++ b/optuna_dashboard/_serializer.py @@ -180,11 +180,10 @@ def serialize_preference_history( history = { "mode": "ChooseWorst", "id": choice["id"], - "preference_id": choice["preference_id"], "timestamp": choice["timestamp"], "candidates": choice["candidates"], "clicked": choice["clicked"], - "is_removed": is_preference_removed(system_attrs, choice["preference_id"]), + "is_removed": is_preference_removed(system_attrs, choice["id"]), } histories.append(history) diff --git a/optuna_dashboard/preferential/_system_attrs.py b/optuna_dashboard/preferential/_system_attrs.py index 82ba61fa..33d56f30 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], uuid: str) -> bool: - key = _SYSTEM_ATTR_PREFIX_PREFERENCE + uuid +def is_preference_removed(study_system_attrs: dict[str, Any], id: str) -> bool: + key = _SYSTEM_ATTR_PREFIX_PREFERENCE + id preference = study_system_attrs.get(key, []) return len(preference) == 0 diff --git a/optuna_dashboard/ts/action.ts b/optuna_dashboard/ts/action.ts index f42a17bb..5b66c58f 100644 --- a/optuna_dashboard/ts/action.ts +++ b/optuna_dashboard/ts/action.ts @@ -16,7 +16,8 @@ import { deleteArtifactAPI, reportPreferenceAPI, skipPreferentialTrialAPI, - switchPreferentialHistoryAPI, + removePreferentialHistoryAPI, + restorePreferentialHistoryAPI, } from "./apiClient" import { graphVisibilityState, @@ -610,12 +611,17 @@ export const actionCreator = () => { }) } - const switchPreferentialHistory = ( - studyId: number, - historyUuid: string, - enable: boolean - ) => { - switchPreferentialHistoryAPI(studyId, historyUuid, enable).catch((err) => { + const removePreferentialHistory = (studyId: number, historyUuid: string) => { + removePreferentialHistoryAPI(studyId, historyUuid).catch((err) => { + const reason = err.response?.data.reason + enqueueSnackbar(`Failed to switch history. Reason: ${reason}`, { + variant: "error", + }) + console.log(err) + }) + } + const restorePreferentialHistory = (studyId: number, historyUuid: string) => { + restorePreferentialHistoryAPI(studyId, historyUuid).catch((err) => { const reason = err.response?.data.reason enqueueSnackbar(`Failed to switch history. Reason: ${reason}`, { variant: "error", @@ -645,7 +651,8 @@ export const actionCreator = () => { saveTrialUserAttrs, updatePreference, skipPreferentialTrial, - switchPreferentialHistory, + removePreferentialHistory, + restorePreferentialHistory, } } diff --git a/optuna_dashboard/ts/apiClient.ts b/optuna_dashboard/ts/apiClient.ts index 5c9355b7..ecc4b7c2 100644 --- a/optuna_dashboard/ts/apiClient.ts +++ b/optuna_dashboard/ts/apiClient.ts @@ -57,12 +57,11 @@ const convertTrialResponse = (res: TrialResponse): Trial => { interface PreferenceHistoryResponce { id: string - preference_id: string candidates: number[] clicked: number mode: PreferenceFeedbackMode timestamp: string - enabled: boolean + is_removed: boolean } const convertPreferenceHistory = ( @@ -70,12 +69,11 @@ const convertPreferenceHistory = ( ): PreferenceHistory => { return { id: res.id, - preference_id: res.preference_id, candidates: res.candidates, clicked: res.clicked, feedback_mode: res.mode, timestamp: new Date(res.timestamp), - enabled: res.enabled, + isRemoved: res.is_removed, } } @@ -369,15 +367,22 @@ export const skipPreferentialTrialAPI = ( }) } -export const switchPreferentialHistoryAPI = ( +export const removePreferentialHistoryAPI = ( studyId: number, - historyUuid: string, - enable: boolean + historyUuid: string ): Promise => { return axiosInstance - .put(`/api/studies/${studyId}/preference/${historyUuid}`, { - enable: enable, + .delete(`/api/studies/${studyId}/preference/${historyUuid}`) + .then(() => { + return }) +} +export const restorePreferentialHistoryAPI = ( + studyId: number, + historyUuid: string +): Promise => { + return axiosInstance + .post(`/api/studies/${studyId}/preference/${historyUuid}`) .then(() => { return }) diff --git a/optuna_dashboard/ts/components/PreferenceHistory.tsx b/optuna_dashboard/ts/components/PreferenceHistory.tsx index 0535dc2e..81f106bd 100644 --- a/optuna_dashboard/ts/components/PreferenceHistory.tsx +++ b/optuna_dashboard/ts/components/PreferenceHistory.tsx @@ -142,14 +142,10 @@ const ChoiceTrials: FC<{ trials: Trial[] study_id: number }> = ({ choice, trials, study_id }) => { - const [enabled, setEnabled] = useState(choice.enabled) + const [isRemoved, setRemoved] = useState(choice.isRemoved) const theme = useTheme() const worst_trials = new Set([choice.clicked]) const action = actionCreator() - const handleSwitch = () => { - setEnabled(!enabled) - action.switchPreferentialHistory(study_id, choice.id, !enabled) - } return ( {formatDate(choice.timestamp)} - - {choice.enabled ? : } - + {choice.isRemoved ? ( + { + setRemoved(false) + action.restorePreferentialHistory(study_id, choice.id) + }} + sx={{ + margin: `auto ${theme.spacing(2)}`, + }} + > + + + ) : ( + { + setRemoved(true) + action.removePreferentialHistory(study_id, choice.id) + }} + sx={{ + margin: `auto ${theme.spacing(2)}`, + }} + > + + + )} {choice.candidates.map((trial_num, index) => ( diff --git a/optuna_dashboard/ts/components/PreferentialTrials.tsx b/optuna_dashboard/ts/components/PreferentialTrials.tsx index ab7f04ea..f079a80d 100644 --- a/optuna_dashboard/ts/components/PreferentialTrials.tsx +++ b/optuna_dashboard/ts/components/PreferentialTrials.tsx @@ -174,8 +174,8 @@ const PreferentialTrial: FC<{ } type DisplayTrials = { - numbers: number[] - last_number: number + display: number[] + clicked: number[] } export const PreferentialTrials: FC<{ studyDetail: StudyDetail | null }> = ({ @@ -193,51 +193,55 @@ export const PreferentialTrials: FC<{ studyDetail: StudyDetail | null }> = ({ const activeTrials = runningTrials.concat(studyDetail.best_trials) const [displayTrials, setDisplayTrials] = useState({ - numbers: activeTrials.map((t) => t.number), - last_number: Math.max(...activeTrials.map((t) => t.number), -1), + display: [], + clicked: [], }) const new_trails = activeTrials.filter( (t) => - displayTrials.last_number < t.number && - displayTrials.numbers.find((n) => n === t.number) === undefined + !displayTrials.display.includes(t.number) && + !displayTrials.clicked.includes(t.number) ) if (new_trails.length > 0) { - setDisplayTrials((display) => { - const numbers = [...display.numbers] + setDisplayTrials((prev) => { + const display = [...prev.display] + const clicked = [...prev.clicked] new_trails.map((t) => { - const index = numbers.findIndex((n) => n === -1) + const index = display.findIndex((n) => n === -1) if (index === -1) { - numbers.push(t.number) + display.push(t.number) + clicked.push(-1) } else { - numbers[index] = t.number + display[index] = t.number } }) return { - numbers: numbers, - last_number: Math.max(...numbers, -1), + display: display, + clicked: clicked, } }) } const hideTrial = (num: number) => { - setDisplayTrials((display) => { - const index = display.numbers.findIndex((n) => n === num) + setDisplayTrials((prev) => { + const index = prev.display.findIndex((n) => n === num) if (index === -1) { - return display + return prev } - const numbers = [...displayTrials.numbers] - numbers[index] = -1 + const display = [...prev.display] + const clicked = [...prev.clicked] + display[index] = -1 + clicked[index] = num return { - numbers: numbers, - last_number: display.last_number, + display: display, + clicked: clicked, } }) } - const latestHistoryId = studyDetail?.preference_history - ?.filter((h) => h.enabled) - .pop()?.id + const latestHistoryId = + studyDetail?.preference_history?.filter((h) => !h.isRemoved).pop()?.id ?? + null if (undoHistoryId !== null && undoHistoryId !== latestHistoryId) { - setUndoHistoryId(null) + setUndoHistoryId(latestHistoryId) } return ( @@ -253,17 +257,13 @@ export const PreferentialTrials: FC<{ studyDetail: StudyDetail | null }> = ({ Which trial is the worst? { - if (latestHistoryId === undefined) { + if (latestHistoryId === null) { return } setUndoHistoryId(latestHistoryId) - action.switchPreferentialHistory( - studyDetail.id, - latestHistoryId, - false - ) + action.removePreferentialHistory(studyDetail.id, latestHistoryId) }} sx={{ margin: "auto 0 auto auto", @@ -273,11 +273,11 @@ export const PreferentialTrials: FC<{ studyDetail: StudyDetail | null }> = ({ - {displayTrials.numbers.map((t, index) => ( + {displayTrials.display.map((t, index) => ( trial.number === t)} - candidates={displayTrials.numbers.filter((n) => n !== -1)} + candidates={displayTrials.display.filter((n) => n !== -1)} hideTrial={() => { hideTrial(t) }} diff --git a/optuna_dashboard/ts/types/index.d.ts b/optuna_dashboard/ts/types/index.d.ts index fd9ca113..014d2dfd 100644 --- a/optuna_dashboard/ts/types/index.d.ts +++ b/optuna_dashboard/ts/types/index.d.ts @@ -217,10 +217,9 @@ type StudyParamImportance = { type PreferenceHistory = { id: string - preference_id: string candidates: number[] clicked: number feedback_mode: PreferenceFeedbackMode timestamp: Date - enabled: boolean + isRemoved: boolean } diff --git a/python_tests/test_api.py b/python_tests/test_api.py index 205bf966..6075ffa5 100644 --- a/python_tests/test_api.py +++ b/python_tests/test_api.py @@ -228,7 +228,7 @@ class APITestCase(TestCase): self.assertEqual(status, 204) histories = serialize_preference_history(storage.get_study_system_attrs(study_id)) assert len(histories) == 1 - assert histories[0]["enabled"] + assert not histories[0]["is_removed"] history_id = histories[0]["id"] status, _, _ = send_request( @@ -240,7 +240,7 @@ class APITestCase(TestCase): self.assertEqual(status, 204) histories = serialize_preference_history(storage.get_study_system_attrs(study_id)) assert len(histories) == 1 - assert not histories[0]["enabled"] + assert histories[0]["is_removed"] assert len(study.get_preferences()) == 0 status, _, _ = send_request( @@ -252,7 +252,7 @@ class APITestCase(TestCase): self.assertEqual(status, 204) histories = serialize_preference_history(storage.get_study_system_attrs(study_id)) assert len(histories) == 1 - assert histories[0]["enabled"] + assert not histories[0]["is_removed"] preferences = study.get_preferences() preferences.sort(key=lambda x: (x[0].number, x[1].number)) assert len(preferences) == 2 diff --git a/python_tests/test_preferential_history.py b/python_tests/test_preferential_history.py index 3ba54fd4..c64f448c 100644 --- a/python_tests/test_preferential_history.py +++ b/python_tests/test_preferential_history.py @@ -46,7 +46,7 @@ def test_report_and_get_choices(storage_supplier: Callable[[], StorageSupplier]) 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]["preference_id"]] + preferences = sys_attrs[_SYSTEM_ATTR_PREFIX_PREFERENCE + history[0]["id"]] assert len(preferences) == 2 for i, (best, worst) in enumerate([(0, 1), (2, 1)]): assert len(preferences[i]) == 2 @@ -54,7 +54,7 @@ def test_report_and_get_choices(storage_supplier: Callable[[], StorageSupplier]) 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]["preference_id"]] + preferences = sys_attrs[_SYSTEM_ATTR_PREFIX_PREFERENCE + history[1]["id"]] assert len(preferences) == 3 for i, (best, worst) in enumerate([(2, 0), (3, 0), (4, 0)]): assert len(preferences[i]) == 2 @@ -72,13 +72,11 @@ def test_undo_redo_history(storage_supplier: Callable[[], StorageSupplier]) -> N study_id = study._study._study_id - def get_preferences_history(history_id: str) -> tuple[list[tuple[int, int]], History]: + 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 + history_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 + history["preference_id"], [] + _SYSTEM_ATTR_PREFIX_PREFERENCE + id, [] ) return preference, history