From a6c7bfd4377827312d00db4d3748381b95e3ced6 Mon Sep 17 00:00:00 2001 From: porink0424 Date: Fri, 31 May 2024 15:08:05 +0900 Subject: [PATCH 1/9] Add remove_associated_artifacts query param into delete_study api --- optuna_dashboard/_app.py | 5 +++- python_tests/test_api.py | 62 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/optuna_dashboard/_app.py b/optuna_dashboard/_app.py index c2e5099af..a60a5d819 100644 --- a/optuna_dashboard/_app.py +++ b/optuna_dashboard/_app.py @@ -180,7 +180,10 @@ def rename_study(study_id: int) -> dict[str, Any]: @app.delete("/api/studies/") @json_api_view def delete_study(study_id: int) -> dict[str, Any]: - if artifact_store is not None: + remove_associated_artifacts = ( + True if request.params.get("remove_associated_artifacts") == "true" else False + ) + if artifact_store is not None and remove_associated_artifacts: delete_all_artifacts(artifact_store, storage, study_id) try: diff --git a/python_tests/test_api.py b/python_tests/test_api.py index d644635fd..129bf0ae2 100644 --- a/python_tests/test_api.py +++ b/python_tests/test_api.py @@ -2,10 +2,13 @@ import json import sys +import tempfile from unittest import TestCase import optuna from optuna import get_all_study_summaries +from optuna.artifacts import upload_artifact +from optuna.artifacts.exceptions import ArtifactNotFound from optuna.study import StudyDirection from optuna_dashboard._app import create_app from optuna_dashboard._app import create_new_study @@ -523,6 +526,65 @@ def test_delete_study(self) -> None: self.assertEqual(status, 204) self.assertEqual(len(get_all_study_summaries(storage)), 1) + def test_delete_study_with_removing_artifacts(self) -> None: + storage = optuna.storages.InMemoryStorage() + study = optuna.create_study(storage=storage) + with tempfile.TemporaryDirectory() as tmpdir_name: + artifact_store = optuna.artifacts.FileSystemArtifactStore(base_path=tmpdir_name) + with tempfile.NamedTemporaryFile() as f: + f.write(b"dummy") + f.flush() + artifact_id = upload_artifact(study, f.name, artifact_store) + + app = create_app(storage, artifact_store) + + with artifact_store.open_reader(artifact_id) as reader: + self.assertEqual(reader.read(), b"dummy") + + status, _, _ = send_request( + app, + f"/api/studies/{study._study_id}", + "DELETE", + queries={"remove_associated_artifacts": "true"}, + content_type="application/json", + ) + self.assertEqual(status, 204) + + with self.assertRaises(ArtifactNotFound): + with artifact_store.open_reader(artifact_id) as reader: + reader.read() + + self.assertEqual(len(get_all_study_summaries(storage)), 0) + + def test_delete_study_without_removing_artifacts(self) -> None: + storage = optuna.storages.InMemoryStorage() + study = optuna.create_study(storage=storage) + with tempfile.TemporaryDirectory() as tmpdir_name: + artifact_store = optuna.artifacts.FileSystemArtifactStore(base_path=tmpdir_name) + with tempfile.NamedTemporaryFile() as f: + f.write(b"dummy") + f.flush() + artifact_id = upload_artifact(study, f.name, artifact_store) + + app = create_app(storage, artifact_store) + + with artifact_store.open_reader(artifact_id) as reader: + self.assertEqual(reader.read(), b"dummy") + + status, _, _ = send_request( + app, + f"/api/studies/{study._study_id}", + "DELETE", + queries={"remove_associated_artifacts": "false"}, + content_type="application/json", + ) + self.assertEqual(status, 204) + + with artifact_store.open_reader(artifact_id) as reader: + self.assertEqual(reader.read(), b"dummy") + + self.assertEqual(len(get_all_study_summaries(storage)), 0) + def test_delete_study_not_found(self) -> None: storage = optuna.storages.InMemoryStorage() app = create_app(storage) From a6f608310f5df40ac0f5f9aa82a8fc74150bb826 Mon Sep 17 00:00:00 2001 From: porink0424 Date: Fri, 31 May 2024 15:10:35 +0900 Subject: [PATCH 2/9] Add removeAssociatedArtifact arg --- optuna_dashboard/ts/action.ts | 4 ++-- optuna_dashboard/ts/apiClient.ts | 5 ++++- optuna_dashboard/ts/axiosClient.ts | 15 +++++++++++---- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/optuna_dashboard/ts/action.ts b/optuna_dashboard/ts/action.ts index 091e7339b..f3109f0a6 100644 --- a/optuna_dashboard/ts/action.ts +++ b/optuna_dashboard/ts/action.ts @@ -284,9 +284,9 @@ export const actionCreator = () => { }) } - const deleteStudy = (studyId: number) => { + const deleteStudy = (studyId: number, removeAssociatedArtifacts: boolean) => { apiClient - .deleteStudy(studyId) + .deleteStudy(studyId, removeAssociatedArtifacts) .then(() => { setStudySummaries(studySummaries.filter((s) => s.study_id !== studyId)) enqueueSnackbar(`Success to delete a study (id=${studyId})`, { diff --git a/optuna_dashboard/ts/apiClient.ts b/optuna_dashboard/ts/apiClient.ts index 038ea9f73..f1a7ad8c5 100644 --- a/optuna_dashboard/ts/apiClient.ts +++ b/optuna_dashboard/ts/apiClient.ts @@ -185,7 +185,10 @@ export abstract class APIClient { studyName: string, directions: Optuna.StudyDirection[] ): Promise - abstract deleteStudy(studyId: number): Promise + abstract deleteStudy( + studyId: number, + removeAssociatedArtifacts: boolean + ): Promise abstract renameStudy( studyId: number, studyName: string diff --git a/optuna_dashboard/ts/axiosClient.ts b/optuna_dashboard/ts/axiosClient.ts index 1c52c42f1..7588ad80d 100644 --- a/optuna_dashboard/ts/axiosClient.ts +++ b/optuna_dashboard/ts/axiosClient.ts @@ -116,10 +116,17 @@ export class AxiosClient extends APIClient { : undefined, } }) - deleteStudy = (studyId: number): Promise => - this.axiosInstance.delete(`/api/studies/${studyId}`).then(() => { - return - }) + deleteStudy = ( + studyId: number, + removeAssociatedArtifacts: boolean + ): Promise => + this.axiosInstance + .delete( + `/api/studies/${studyId}?remove_associated_artifacts=${removeAssociatedArtifacts}` + ) + .then(() => { + return + }) renameStudy = (studyId: number, studyName: string): Promise => this.axiosInstance .post(`/api/studies/${studyId}/rename`, { From 6ec2de7e414f5cbcb6fd2180b74fc679bdd40e4e Mon Sep 17 00:00:00 2001 From: porink0424 Date: Fri, 31 May 2024 15:18:24 +0900 Subject: [PATCH 3/9] Impl checkbox for selecting whether to remove associated artifacts --- .../ts/components/DeleteStudyDialog.tsx | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/optuna_dashboard/ts/components/DeleteStudyDialog.tsx b/optuna_dashboard/ts/components/DeleteStudyDialog.tsx index 14312e068..d4e4dc9b9 100644 --- a/optuna_dashboard/ts/components/DeleteStudyDialog.tsx +++ b/optuna_dashboard/ts/components/DeleteStudyDialog.tsx @@ -1,10 +1,13 @@ import { + Alert, Button, + Checkbox, Dialog, DialogActions, DialogContent, DialogContentText, DialogTitle, + FormControlLabel, } from "@mui/material" import React, { ReactNode, useState } from "react" import { actionCreator } from "../action" @@ -17,16 +20,18 @@ export const useDeleteStudyDialog = (): [ const [openDeleteStudyDialog, setOpenDeleteStudyDialog] = useState(false) const [deleteStudyID, setDeleteStudyID] = useState(-1) + const [removeAssociatedArtifacts, setRemoveAssociatedArtifacts] = + useState(false) const handleCloseDeleteStudyDialog = () => { setOpenDeleteStudyDialog(false) setDeleteStudyID(-1) + setRemoveAssociatedArtifacts(false) } const handleDeleteStudy = () => { - action.deleteStudy(deleteStudyID) - setOpenDeleteStudyDialog(false) - setDeleteStudyID(-1) + action.deleteStudy(deleteStudyID, removeAssociatedArtifacts) + handleCloseDeleteStudyDialog() } const openDialog = (studyId: number) => { @@ -42,12 +47,29 @@ export const useDeleteStudyDialog = (): [ handleCloseDeleteStudyDialog() }} aria-labelledby="delete-study-dialog-title" + fullWidth + maxWidth="xs" > Delete study Are you sure you want to delete a study (id={deleteStudyID})? + setRemoveAssociatedArtifacts((cur) => !cur)} + /> + } + /> + {removeAssociatedArtifacts && ( + + If articles are linked to another study or trial, they will no + longer be accessible from that study or trial as well. + + )}