Skip to content

Commit

Permalink
Merge pull request #885 from porink0424/feat/checkbox-to-ask-delete-a…
Browse files Browse the repository at this point in the history
…rtifacts

Add a checkbox to ask whether to delete artifacts associated to study/trial
  • Loading branch information
c-bata authored Jun 5, 2024
2 parents be91172 + 2b0fd04 commit a29ac8a
Show file tree
Hide file tree
Showing 7 changed files with 140 additions and 12 deletions.
5 changes: 4 additions & 1 deletion optuna_dashboard/_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,10 @@ def rename_study(study_id: int) -> dict[str, Any]:
@app.delete("/api/studies/<study_id:int>")
@json_api_view
def delete_study(study_id: int) -> dict[str, Any]:
if artifact_store is not None:
data = request.json or {}
remove_associated_artifacts = data.get("remove_associated_artifacts", True)

if artifact_store is not None and remove_associated_artifacts:
delete_all_artifacts(artifact_store, storage, study_id)

try:
Expand Down
4 changes: 2 additions & 2 deletions optuna_dashboard/ts/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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})`, {
Expand Down
5 changes: 4 additions & 1 deletion optuna_dashboard/ts/apiClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,10 @@ export abstract class APIClient {
studyName: string,
directions: Optuna.StudyDirection[]
): Promise<StudySummary>
abstract deleteStudy(studyId: number): Promise<void>
abstract deleteStudy(
studyId: number,
removeAssociatedArtifacts: boolean
): Promise<void>
abstract renameStudy(
studyId: number,
studyName: string
Expand Down
17 changes: 13 additions & 4 deletions optuna_dashboard/ts/axiosClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,19 @@ export class AxiosClient extends APIClient {
: undefined,
}
})
deleteStudy = (studyId: number): Promise<void> =>
this.axiosInstance.delete(`/api/studies/${studyId}`).then(() => {
return
})
deleteStudy = (
studyId: number,
removeAssociatedArtifacts: boolean
): Promise<void> =>
this.axiosInstance
.delete(`/api/studies/${studyId}`, {
data: {
remove_associated_artifacts: removeAssociatedArtifacts,
},
})
.then(() => {
return
})
renameStudy = (studyId: number, studyName: string): Promise<StudySummary> =>
this.axiosInstance
.post<RenameStudyResponse>(`/api/studies/${studyId}/rename`, {
Expand Down
13 changes: 12 additions & 1 deletion optuna_dashboard/ts/components/Artifact/DeleteArtifactDialog.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import {
Alert,
Button,
Dialog,
DialogActions,
DialogContent,
DialogContentText,
DialogTitle,
useTheme,
} from "@mui/material"
import React, { ReactNode, useState, FC } from "react"
import { Artifact } from "ts/types/optuna"
Expand Down Expand Up @@ -111,6 +113,7 @@ const DeleteDialog: FC<{
filename,
handleDeleteArtifact,
}) => {
const theme = useTheme()
return (
<Dialog
open={openDeleteArtifactDialog}
Expand All @@ -123,10 +126,18 @@ const DeleteDialog: FC<{
Delete artifact
</DialogTitle>
<DialogContent>
<DialogContentText>
<DialogContentText
sx={{
marginBottom: theme.spacing(2),
}}
>
Are you sure you want to delete an artifact ("
{filename}")?
</DialogContentText>
<Alert severity="warning">
If this artifact is linked to another study or trial, it will no
longer be accessible from that study or trial as well.
</Alert>
</DialogContent>
<DialogActions>
<Button onClick={handleCloseDeleteArtifactDialog} color="primary">
Expand Down
35 changes: 32 additions & 3 deletions optuna_dashboard/ts/components/DeleteStudyDialog.tsx
Original file line number Diff line number Diff line change
@@ -1,32 +1,40 @@
import {
Alert,
Button,
Checkbox,
Dialog,
DialogActions,
DialogContent,
DialogContentText,
DialogTitle,
FormControlLabel,
} from "@mui/material"
import React, { ReactNode, useState } from "react"
import { useRecoilValue } from "recoil"
import { actionCreator } from "../action"
import { artifactIsAvailable as artifactIsAvailableState } from "../state"

export const useDeleteStudyDialog = (): [
(studyId: number) => void,
() => ReactNode,
] => {
const action = actionCreator()
const artifactIsAvailable = useRecoilValue(artifactIsAvailableState)

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) => {
Expand All @@ -42,12 +50,33 @@ export const useDeleteStudyDialog = (): [
handleCloseDeleteStudyDialog()
}}
aria-labelledby="delete-study-dialog-title"
fullWidth
maxWidth="xs"
>
<DialogTitle id="delete-study-dialog-title">Delete study</DialogTitle>
<DialogContent>
<DialogContentText>
Are you sure you want to delete a study (id={deleteStudyID})?
</DialogContentText>
{artifactIsAvailable && (
<>
<FormControlLabel
label="Remove associated trial/study artifacts."
control={
<Checkbox
checked={removeAssociatedArtifacts}
onChange={() => setRemoveAssociatedArtifacts((cur) => !cur)}
/>
}
/>
{removeAssociatedArtifacts && (
<Alert severity="warning">
If artifacts are linked to another study or trial, they will
no longer be accessible from that study or trial as well.
</Alert>
)}
</>
)}
</DialogContent>
<DialogActions>
<Button onClick={handleCloseDeleteStudyDialog} color="primary">
Expand Down
73 changes: 73 additions & 0 deletions python_tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import json
import sys
import tempfile
from unittest import TestCase

import optuna
Expand Down Expand Up @@ -523,6 +524,78 @@ def test_delete_study(self) -> None:
self.assertEqual(status, 204)
self.assertEqual(len(get_all_study_summaries(storage)), 1)

@pytest.mark.skipif(
version.parse(optuna.__version__) < version.parse("3.4.0"),
reason="Needs optuna.artifacts",
)
def test_delete_study_with_removing_artifacts(self) -> None:
from optuna.artifacts import upload_artifact
from optuna.artifacts.exceptions import ArtifactNotFound

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",
body=json.dumps({"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)

@pytest.mark.skipif(
version.parse(optuna.__version__) < version.parse("3.4.0"),
reason="Needs optuna.artifacts",
)
def test_delete_study_without_removing_artifacts(self) -> None:
from optuna.artifacts import upload_artifact

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",
body=json.dumps({"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)
Expand Down

0 comments on commit a29ac8a

Please sign in to comment.