From bab2c87ef71a2cbaf6622116e0133734a243a021 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 17 Aug 2023 14:12:27 +0900 Subject: [PATCH 01/16] Display TrialArtifact only when trial is running --- optuna_dashboard/ts/components/TrialList.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/optuna_dashboard/ts/components/TrialList.tsx b/optuna_dashboard/ts/components/TrialList.tsx index eb700c138..487f124f8 100644 --- a/optuna_dashboard/ts/components/TrialList.tsx +++ b/optuna_dashboard/ts/components/TrialList.tsx @@ -153,6 +153,7 @@ const TrialListDetail: FC<{ const theme = useTheme() const action = actionCreator() const artifactEnabled = useRecoilValue(artifactIsAvailable) + const isRunningTrial = trial.state === "Running" || trial.state === "Waiting" const startMs = trial.datetime_start?.getTime() const completeMs = trial.datetime_complete?.getTime() @@ -319,7 +320,7 @@ const TrialListDetail: FC<{ value !== null ? renderInfo(key, value) : null )} - {artifactEnabled && } + {artifactEnabled && isRunningTrial && } ) } From b56db254f9dc74e68dce1de5426dd04b3807cadc Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 17 Aug 2023 14:33:07 +0900 Subject: [PATCH 02/16] Store metadeta in trial_system_attr --- optuna_dashboard/artifact/_backend.py | 34 ++++++++++++--------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index eea1e9885..f8f605697 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -102,8 +102,8 @@ def upload_artifact_api(study_id: int, trial_id: int) -> dict[str, Any]: "mimetype": mimetype or DEFAULT_MIME_TYPE, "encoding": encoding, } - attr_key = _artifact_prefix(trial_id=trial_id) + artifact_id - storage.set_study_system_attr(study_id, attr_key, json.dumps(artifact)) + attr_key = ARTIFACTS_ATTR_PREFIX + artifact_id + storage.set_trial_system_attr(trial_id, attr_key, json.dumps(artifact)) response.status = 201 trial = storage.get_trial(trial_id) @@ -112,7 +112,7 @@ def upload_artifact_api(study_id: int, trial_id: int) -> dict[str, Any]: return {"reason": "Invalid study_id or trial_id"} return { "artifact_id": artifact_id, - "artifacts": list_trial_artifacts(storage.get_study_system_attrs(study_id), trial), + "artifacts": list_trial_artifacts(storage.get_trial_system_attrs(trial_id), trial), } @app.delete("/api/artifacts///") @@ -123,8 +123,8 @@ def delete_artifact(study_id: int, trial_id: int, artifact_id: str) -> dict[str, return {"reason": "Cannot access to the artifacts."} artifact_store.remove(artifact_id) - attr_key = _artifact_prefix(trial_id) + artifact_id - storage.set_study_system_attr(study_id, attr_key, json.dumps(None)) + attr_key = ARTIFACTS_ATTR_PREFIX + artifact_id + storage.set_trial_system_attr(trial_id, attr_key, json.dumps(None)) response.status = 204 return {} @@ -178,24 +178,20 @@ def objective(trial: optuna.Trial) -> float: "encoding": encoding or guess_encoding, "filename": filename, } - attr_key = _artifact_prefix(trial_id=trial_id) + artifact_id - storage.set_study_system_attr(study_id, attr_key, json.dumps(artifact)) + attr_key = ARTIFACTS_ATTR_PREFIX + artifact_id + storage.set_trial_system_attr(trial_id, attr_key, json.dumps(artifact)) with open(file_path, "rb") as f: backend.write(artifact_id, f) return artifact_id -def _artifact_prefix(trial_id: int) -> str: - return ARTIFACTS_ATTR_PREFIX + f"{trial_id}:" - - def get_artifact_meta( storage: BaseStorage, study_id: int, trial_id: int, artifact_id: str ) -> Optional[ArtifactMeta]: - study_system_attr = storage.get_study_system_attrs(study_id) - attr_key = _artifact_prefix(trial_id=trial_id) + artifact_id - artifact_meta = study_system_attr.get(attr_key) + trial_system_attr = storage.get_trial_system_attrs(trial_id) + attr_key = ARTIFACTS_ATTR_PREFIX + artifact_id + artifact_meta = trial_system_attr.get(attr_key) if artifact_meta is not None: return json.loads(artifact_meta) @@ -209,9 +205,9 @@ def get_artifact_meta( def delete_all_artifacts(backend: ArtifactStore, storage: BaseStorage, study_id: int) -> None: artifact_metas = [] - study_system_attrs = storage.get_study_system_attrs(study_id) for trial in storage.get_all_trials(study_id): - trial_artifacts = list_trial_artifacts(study_system_attrs, trial) + trial_system_attrs = storage.get_trial_system_attrs(trial._trial_id) + trial_artifacts = list_trial_artifacts(trial_system_attrs, trial) artifact_metas.extend(trial_artifacts) for meta in artifact_metas: @@ -219,12 +215,12 @@ def delete_all_artifacts(backend: ArtifactStore, storage: BaseStorage, study_id: def list_trial_artifacts( - study_system_attrs: dict[str, Any], trial: FrozenTrial + trial_system_attrs: dict[str, Any], trial: FrozenTrial ) -> list[ArtifactMeta]: dashboard_artifact_metas = [ json.loads(value) - for key, value in study_system_attrs.items() - if key.startswith(_artifact_prefix(trial._trial_id)) + for key, value in trial_system_attrs.items() + if key.startswith(ARTIFACTS_ATTR_PREFIX) ] # See https://github.com/optuna/optuna/blob/f827582a8/optuna/artifacts/_upload.py#L16 From 2cf12457d5d38eafc471117c9b754503621871fb Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 17 Aug 2023 15:56:11 +0900 Subject: [PATCH 03/16] Remove unused argument --- optuna_dashboard/artifact/_backend.py | 5 ++--- python_tests/artifact/test_optuna_compatibility.py | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index f8f605697..a26e6af87 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -66,7 +66,7 @@ def proxy_artifact(study_id: int, trial_id: int, artifact_id: str) -> HTTPRespon if artifact_store is None: response.status = 400 # Bad Request return b"Cannot access to the artifacts." - artifact_dict = get_artifact_meta(storage, study_id, trial_id, artifact_id) + artifact_dict = get_artifact_meta(storage, trial_id, artifact_id) if artifact_dict is None: response.status = 404 return b"Not Found" @@ -169,7 +169,6 @@ def objective(trial: optuna.Trial) -> float: filename = os.path.basename(file_path) storage = trial.storage trial_id = trial._trial_id - study_id = trial.study._study_id artifact_id = str(uuid.uuid4()) guess_mimetype, guess_encoding = mimetypes.guess_type(filename) artifact: ArtifactMeta = { @@ -187,7 +186,7 @@ def objective(trial: optuna.Trial) -> float: def get_artifact_meta( - storage: BaseStorage, study_id: int, trial_id: int, artifact_id: str + storage: BaseStorage, trial_id: int, artifact_id: str ) -> Optional[ArtifactMeta]: trial_system_attr = storage.get_trial_system_attrs(trial_id) attr_key = ARTIFACTS_ATTR_PREFIX + artifact_id diff --git a/python_tests/artifact/test_optuna_compatibility.py b/python_tests/artifact/test_optuna_compatibility.py index d81e13977..52032ae1b 100644 --- a/python_tests/artifact/test_optuna_compatibility.py +++ b/python_tests/artifact/test_optuna_compatibility.py @@ -46,7 +46,6 @@ def test_list_optuna_trial_artifacts() -> None: artifact_meta = get_artifact_meta( storage=storage, - study_id=study._study_id, trial_id=trial._trial_id, artifact_id=artifact_id, ) From f00fd3b4e29e2d7773f973dd2c8fa23b794c2a51 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 17 Aug 2023 16:00:59 +0900 Subject: [PATCH 04/16] Check whether trial is finished or not when uploaded --- optuna_dashboard/artifact/_backend.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index a26e6af87..b8ad54d82 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -81,6 +81,14 @@ def proxy_artifact(study_id: int, trial_id: int, artifact_id: str) -> HTTPRespon @app.post("/api/artifacts//") @json_api_view def upload_artifact_api(study_id: int, trial_id: int) -> dict[str, Any]: + trial = storage.get_trial(trial_id) + if trial is None: + response.status = 400 + return {"reason": "Invalid study_id or trial_id"} + elif trial.state.is_finished(): + response.status = 400 + return {"reason": "The trial is already finished."} + # TODO(c-bata): Use optuna.artifacts.upload_artifact() if artifact_store is None: response.status = 400 # Bad Request @@ -106,10 +114,6 @@ def upload_artifact_api(study_id: int, trial_id: int) -> dict[str, Any]: storage.set_trial_system_attr(trial_id, attr_key, json.dumps(artifact)) response.status = 201 - trial = storage.get_trial(trial_id) - if trial is None: - response.status = 400 - return {"reason": "Invalid study_id or trial_id"} return { "artifact_id": artifact_id, "artifacts": list_trial_artifacts(storage.get_trial_system_attrs(trial_id), trial), From 10c1b76d155add8d9dea6ab1c4a285c54b375eb7 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 17 Aug 2023 17:05:46 +0900 Subject: [PATCH 05/16] Remove upload button only --- optuna_dashboard/ts/components/TrialList.tsx | 162 +++++++++---------- 1 file changed, 81 insertions(+), 81 deletions(-) diff --git a/optuna_dashboard/ts/components/TrialList.tsx b/optuna_dashboard/ts/components/TrialList.tsx index 487f124f8..69c7de7a7 100644 --- a/optuna_dashboard/ts/components/TrialList.tsx +++ b/optuna_dashboard/ts/components/TrialList.tsx @@ -1,26 +1,25 @@ -import React, { - ChangeEventHandler, - DragEventHandler, - FC, - MouseEventHandler, - ReactNode, - useMemo, - useRef, - useState, -} from "react" +import CheckBoxIcon from "@mui/icons-material/CheckBox" +import CheckBoxOutlineBlankIcon from "@mui/icons-material/CheckBoxOutlineBlank" +import DeleteIcon from "@mui/icons-material/Delete" +import DownloadIcon from "@mui/icons-material/Download" +import FilterListIcon from "@mui/icons-material/FilterList" +import FullscreenIcon from "@mui/icons-material/Fullscreen" +import InsertDriveFileIcon from "@mui/icons-material/InsertDriveFile" +import StopCircleIcon from "@mui/icons-material/StopCircle" +import UploadFileIcon from "@mui/icons-material/UploadFile" import { - Typography, Box, Button, - useTheme, - IconButton, - Menu, - MenuItem, Card, + CardActionArea, CardContent, CardMedia, - CardActionArea, + IconButton, + Menu, + MenuItem, Modal, + Typography, + useTheme, } from "@mui/material" import Chip from "@mui/material/Chip" import Divider from "@mui/material/Divider" @@ -29,25 +28,26 @@ import ListItem from "@mui/material/ListItem" import ListItemButton from "@mui/material/ListItemButton" import ListItemText from "@mui/material/ListItemText" import ListSubheader from "@mui/material/ListSubheader" -import FilterListIcon from "@mui/icons-material/FilterList" -import CheckBoxOutlineBlankIcon from "@mui/icons-material/CheckBoxOutlineBlank" -import CheckBoxIcon from "@mui/icons-material/CheckBox" -import UploadFileIcon from "@mui/icons-material/UploadFile" -import DownloadIcon from "@mui/icons-material/Download" -import DeleteIcon from "@mui/icons-material/Delete" -import FullscreenIcon from "@mui/icons-material/Fullscreen" -import InsertDriveFileIcon from "@mui/icons-material/InsertDriveFile" -import StopCircleIcon from "@mui/icons-material/StopCircle" +import React, { + ChangeEventHandler, + DragEventHandler, + FC, + MouseEventHandler, + ReactNode, + useMemo, + useRef, + useState, +} from "react" -import { TrialNote } from "./Note" -import { useNavigate, useLocation } from "react-router-dom" import ListItemIcon from "@mui/material/ListItemIcon" +import { useLocation, useNavigate } from "react-router-dom" import { useRecoilValue } from "recoil" -import { artifactIsAvailable } from "../state" import { actionCreator } from "../action" +import { artifactIsAvailable } from "../state" import { useDeleteArtifactDialog } from "./DeleteArtifactDialog" -import { TrialFormWidgets } from "./TrialFormWidgets" +import { TrialNote } from "./Note" import { ThreejsArtifactViewer } from "./ThreejsArtifactViewer" +import { TrialFormWidgets } from "./TrialFormWidgets" const states: TrialState[] = [ "Complete", @@ -153,7 +153,6 @@ const TrialListDetail: FC<{ const theme = useTheme() const action = actionCreator() const artifactEnabled = useRecoilValue(artifactIsAvailable) - const isRunningTrial = trial.state === "Running" || trial.state === "Waiting" const startMs = trial.datetime_start?.getTime() const completeMs = trial.datetime_complete?.getTime() @@ -320,7 +319,7 @@ const TrialListDetail: FC<{ value !== null ? renderInfo(key, value) : null )} - {artifactEnabled && isRunningTrial && } + {artifactEnabled && } ) } @@ -711,55 +710,56 @@ const TrialArtifact: FC<{ trial: Trial }> = ({ trial }) => { ) } })} - - - - - - Upload a New File - - Drag your file here or click to browse. - - - - + + + Upload a New File + + Drag your file here or click to browse. + + + + + ) : null} {renderDeleteArtifactDialog()} @@ -952,15 +952,15 @@ export const TrialList: FC<{ studyDetail: StudyDetail | null }> = ({ {selected.length === 0 ? null : selected.map((t) => ( - - ))} + + ))} From 961f9c8716383124be95a33326d9472b96f6edf3 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 17 Aug 2023 20:33:17 +0900 Subject: [PATCH 06/16] Revert change on get_artifact_meta() --- python_tests/artifact/test_optuna_compatibility.py | 1 + 1 file changed, 1 insertion(+) diff --git a/python_tests/artifact/test_optuna_compatibility.py b/python_tests/artifact/test_optuna_compatibility.py index 52032ae1b..d81e13977 100644 --- a/python_tests/artifact/test_optuna_compatibility.py +++ b/python_tests/artifact/test_optuna_compatibility.py @@ -46,6 +46,7 @@ def test_list_optuna_trial_artifacts() -> None: artifact_meta = get_artifact_meta( storage=storage, + study_id=study._study_id, trial_id=trial._trial_id, artifact_id=artifact_id, ) From 9e61d004aa418438e0df04c7414efea6a7977ec8 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 17 Aug 2023 23:00:50 +0900 Subject: [PATCH 07/16] Preserve backward compatibility when get and delete artifact --- optuna_dashboard/artifact/_backend.py | 47 ++++++++++++++++++--------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index b8ad54d82..b58c1daf8 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -40,7 +40,7 @@ }, ) - +OPTUNA_ARTIFACTS_ATTR_PREFIX = "artifacts:" ARTIFACTS_ATTR_PREFIX = "dashboard:artifacts:" DEFAULT_MIME_TYPE = "application/octet-stream" BaseRequest.MEMFILE_MAX = int( @@ -66,7 +66,7 @@ def proxy_artifact(study_id: int, trial_id: int, artifact_id: str) -> HTTPRespon if artifact_store is None: response.status = 400 # Bad Request return b"Cannot access to the artifacts." - artifact_dict = get_artifact_meta(storage, trial_id, artifact_id) + artifact_dict = get_artifact_meta(storage, study_id, trial_id, artifact_id) if artifact_dict is None: response.status = 404 return b"Not Found" @@ -127,8 +127,11 @@ def delete_artifact(study_id: int, trial_id: int, artifact_id: str) -> dict[str, return {"reason": "Cannot access to the artifacts."} artifact_store.remove(artifact_id) - attr_key = ARTIFACTS_ATTR_PREFIX + artifact_id - storage.set_trial_system_attr(trial_id, attr_key, json.dumps(None)) + # The metadata of the artifact is stored in one of the following three locations: + storage.set_study_system_attr(study_id, _artifact_prefix(trial_id) + artifact_id, json.dumps(None)) + storage.set_trial_system_attr(trial_id, ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None)) + storage.set_trial_system_attr(trial_id, OPTUNA_ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None)) + response.status = 204 return {} @@ -189,28 +192,38 @@ def objective(trial: optuna.Trial) -> float: return artifact_id +def _artifact_prefix(trial_id: int) -> str: + return ARTIFACTS_ATTR_PREFIX + f"{trial_id}:" + + def get_artifact_meta( - storage: BaseStorage, trial_id: int, artifact_id: str + storage: BaseStorage, study_id: int, trial_id: int, artifact_id: str ) -> Optional[ArtifactMeta]: - trial_system_attr = storage.get_trial_system_attrs(trial_id) - attr_key = ARTIFACTS_ATTR_PREFIX + artifact_id - artifact_meta = trial_system_attr.get(attr_key) + # Search study_system_attrs due to backward compatibility. + study_system_attrs = storage.get_study_system_attrs(study_id) + attr_key = _artifact_prefix(trial_id=trial_id) + artifact_id + artifact_meta = study_system_attrs.get(attr_key) if artifact_meta is not None: return json.loads(artifact_meta) + # Search trial_system_attrs. Note that artifacts uploaded via optuna.artifacts.upload_artifact + # have a different trial_system_attrs key prefix. # See https://github.com/optuna/optuna/blob/f827582a8/optuna/artifacts/_upload.py#L71 trial_system_attrs = storage.get_trial_system_attrs(trial_id) - value = trial_system_attrs.get("artifacts:" + artifact_id) + value = trial_system_attrs.get( + OPTUNA_ARTIFACTS_ATTR_PREFIX + artifact_id + ) or trial_system_attrs.get(ARTIFACTS_ATTR_PREFIX + artifact_id) if value is not None: return json.loads(value) + return None def delete_all_artifacts(backend: ArtifactStore, storage: BaseStorage, study_id: int) -> None: artifact_metas = [] + study_system_attrs = storage.get_study_system_attrs(study_id) for trial in storage.get_all_trials(study_id): - trial_system_attrs = storage.get_trial_system_attrs(trial._trial_id) - trial_artifacts = list_trial_artifacts(trial_system_attrs, trial) + trial_artifacts = list_trial_artifacts(study_system_attrs, trial) artifact_metas.extend(trial_artifacts) for meta in artifact_metas: @@ -218,20 +231,22 @@ def delete_all_artifacts(backend: ArtifactStore, storage: BaseStorage, study_id: def list_trial_artifacts( - trial_system_attrs: dict[str, Any], trial: FrozenTrial + study_system_attrs: dict[str, Any], trial: FrozenTrial ) -> list[ArtifactMeta]: + # Collect ArtifactMeta from study_system_attrs due to backward compatibility. dashboard_artifact_metas = [ json.loads(value) - for key, value in trial_system_attrs.items() - if key.startswith(ARTIFACTS_ATTR_PREFIX) + for key, value in study_system_attrs.items() + if key.startswith(_artifact_prefix(trial._trial_id)) ] + # Collect ArtifactMeta from trial_system_attrs. Note that artifacts uploaded via + # optuna.artifacts.upload_artifacts have a different trial_system_attrs key prefix. # See https://github.com/optuna/optuna/blob/f827582a8/optuna/artifacts/_upload.py#L16 optuna_artifact_metas = [ json.loads(value) for key, value in trial.system_attrs.items() - if key.startswith("artifacts:") + if key.startswith(OPTUNA_ARTIFACTS_ATTR_PREFIX) or key.startswith(ARTIFACTS_ATTR_PREFIX) ] - artifact_metas = dashboard_artifact_metas + optuna_artifact_metas return [a for a in artifact_metas if a is not None] From 5844697dbabc187df66809ee4485f1e09b8b9e17 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Fri, 18 Aug 2023 10:24:53 +0900 Subject: [PATCH 08/16] Apply black --- optuna_dashboard/artifact/_backend.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index b58c1daf8..4ed074907 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -128,9 +128,15 @@ def delete_artifact(study_id: int, trial_id: int, artifact_id: str) -> dict[str, artifact_store.remove(artifact_id) # The metadata of the artifact is stored in one of the following three locations: - storage.set_study_system_attr(study_id, _artifact_prefix(trial_id) + artifact_id, json.dumps(None)) - storage.set_trial_system_attr(trial_id, ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None)) - storage.set_trial_system_attr(trial_id, OPTUNA_ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None)) + storage.set_study_system_attr( + study_id, _artifact_prefix(trial_id) + artifact_id, json.dumps(None) + ) + storage.set_trial_system_attr( + trial_id, ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) + ) + storage.set_trial_system_attr( + trial_id, OPTUNA_ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) + ) response.status = 204 return {} @@ -215,7 +221,7 @@ def get_artifact_meta( ) or trial_system_attrs.get(ARTIFACTS_ATTR_PREFIX + artifact_id) if value is not None: return json.loads(value) - + return None From 571d1288b6cfcfeac45a388d09b59f1451d29268 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Fri, 18 Aug 2023 10:55:55 +0900 Subject: [PATCH 09/16] Apply fmt --- optuna_dashboard/ts/components/TrialList.tsx | 25 ++++++++++---------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/optuna_dashboard/ts/components/TrialList.tsx b/optuna_dashboard/ts/components/TrialList.tsx index 69c7de7a7..f2ff14466 100644 --- a/optuna_dashboard/ts/components/TrialList.tsx +++ b/optuna_dashboard/ts/components/TrialList.tsx @@ -710,7 +710,7 @@ const TrialArtifact: FC<{ trial: Trial }> = ({ trial }) => { ) } })} - {(trial.state === "Running" || trial.state === "Waiting") ? ( + {trial.state === "Running" || trial.state === "Waiting" ? ( = ({ trial }) => { minHeight: height, margin: theme.spacing(0, 1, 1, 0), border: dragOver - ? `3px dashed ${theme.palette.mode === "dark" ? "white" : "black" - }` + ? `3px dashed ${ + theme.palette.mode === "dark" ? "white" : "black" + }` : `1px solid ${theme.palette.divider}`, }} onDragOver={handleDragOver} @@ -952,15 +953,15 @@ export const TrialList: FC<{ studyDetail: StudyDetail | null }> = ({ {selected.length === 0 ? null : selected.map((t) => ( - - ))} + + ))} From 9760436222d9951acdf595de3572cfb693e1e2f4 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Fri, 18 Aug 2023 10:59:24 +0900 Subject: [PATCH 10/16] Revert auto format --- optuna_dashboard/ts/components/TrialList.tsx | 58 ++++++++++---------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/optuna_dashboard/ts/components/TrialList.tsx b/optuna_dashboard/ts/components/TrialList.tsx index f2ff14466..4a54331d3 100644 --- a/optuna_dashboard/ts/components/TrialList.tsx +++ b/optuna_dashboard/ts/components/TrialList.tsx @@ -1,25 +1,26 @@ -import CheckBoxIcon from "@mui/icons-material/CheckBox" -import CheckBoxOutlineBlankIcon from "@mui/icons-material/CheckBoxOutlineBlank" -import DeleteIcon from "@mui/icons-material/Delete" -import DownloadIcon from "@mui/icons-material/Download" -import FilterListIcon from "@mui/icons-material/FilterList" -import FullscreenIcon from "@mui/icons-material/Fullscreen" -import InsertDriveFileIcon from "@mui/icons-material/InsertDriveFile" -import StopCircleIcon from "@mui/icons-material/StopCircle" -import UploadFileIcon from "@mui/icons-material/UploadFile" +import React, { + ChangeEventHandler, + DragEventHandler, + FC, + MouseEventHandler, + ReactNode, + useMemo, + useRef, + useState, +} from "react" import { + Typography, Box, Button, - Card, - CardActionArea, - CardContent, - CardMedia, + useTheme, IconButton, Menu, MenuItem, + Card, + CardContent, + CardMedia, + CardActionArea, Modal, - Typography, - useTheme, } from "@mui/material" import Chip from "@mui/material/Chip" import Divider from "@mui/material/Divider" @@ -28,26 +29,25 @@ import ListItem from "@mui/material/ListItem" import ListItemButton from "@mui/material/ListItemButton" import ListItemText from "@mui/material/ListItemText" import ListSubheader from "@mui/material/ListSubheader" -import React, { - ChangeEventHandler, - DragEventHandler, - FC, - MouseEventHandler, - ReactNode, - useMemo, - useRef, - useState, -} from "react" +import FilterListIcon from "@mui/icons-material/FilterList" +import CheckBoxOutlineBlankIcon from "@mui/icons-material/CheckBoxOutlineBlank" +import CheckBoxIcon from "@mui/icons-material/CheckBox" +import UploadFileIcon from "@mui/icons-material/UploadFile" +import DownloadIcon from "@mui/icons-material/Download" +import DeleteIcon from "@mui/icons-material/Delete" +import FullscreenIcon from "@mui/icons-material/Fullscreen" +import InsertDriveFileIcon from "@mui/icons-material/InsertDriveFile" +import StopCircleIcon from "@mui/icons-material/StopCircle" +import { TrialNote } from "./Note" +import { useNavigate, useLocation } from "react-router-dom" import ListItemIcon from "@mui/material/ListItemIcon" -import { useLocation, useNavigate } from "react-router-dom" import { useRecoilValue } from "recoil" -import { actionCreator } from "../action" import { artifactIsAvailable } from "../state" +import { actionCreator } from "../action" import { useDeleteArtifactDialog } from "./DeleteArtifactDialog" -import { TrialNote } from "./Note" -import { ThreejsArtifactViewer } from "./ThreejsArtifactViewer" import { TrialFormWidgets } from "./TrialFormWidgets" +import { ThreejsArtifactViewer } from "./ThreejsArtifactViewer" const states: TrialState[] = [ "Complete", From 408cf00d4a69f29d8a2b5c73f2f2b0b53ba05c8d Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Sat, 19 Aug 2023 10:55:57 +0900 Subject: [PATCH 11/16] Make ARTIFACTS_ATTR_PREFIX default consist with optuna --- optuna_dashboard/artifact/_backend.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index 4ed074907..5f0ba2610 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -40,8 +40,8 @@ }, ) -OPTUNA_ARTIFACTS_ATTR_PREFIX = "artifacts:" -ARTIFACTS_ATTR_PREFIX = "dashboard:artifacts:" +ARTIFACTS_ATTR_PREFIX = "artifacts:" +DASHBOARD_ARTIFACTS_ATTR_PREFIX = "dashboard:artifacts:" DEFAULT_MIME_TYPE = "application/octet-stream" BaseRequest.MEMFILE_MAX = int( os.environ.get("OPTUNA_DASHBOARD_MEMFILE_MAX", 1024 * 1024 * 128) @@ -132,10 +132,10 @@ def delete_artifact(study_id: int, trial_id: int, artifact_id: str) -> dict[str, study_id, _artifact_prefix(trial_id) + artifact_id, json.dumps(None) ) storage.set_trial_system_attr( - trial_id, ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) + trial_id, DASHBOARD_ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) ) storage.set_trial_system_attr( - trial_id, OPTUNA_ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) + trial_id, ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) ) response.status = 204 @@ -199,7 +199,7 @@ def objective(trial: optuna.Trial) -> float: def _artifact_prefix(trial_id: int) -> str: - return ARTIFACTS_ATTR_PREFIX + f"{trial_id}:" + return DASHBOARD_ARTIFACTS_ATTR_PREFIX + f"{trial_id}:" def get_artifact_meta( @@ -217,7 +217,7 @@ def get_artifact_meta( # See https://github.com/optuna/optuna/blob/f827582a8/optuna/artifacts/_upload.py#L71 trial_system_attrs = storage.get_trial_system_attrs(trial_id) value = trial_system_attrs.get( - OPTUNA_ARTIFACTS_ATTR_PREFIX + artifact_id + DASHBOARD_ARTIFACTS_ATTR_PREFIX + artifact_id ) or trial_system_attrs.get(ARTIFACTS_ATTR_PREFIX + artifact_id) if value is not None: return json.loads(value) @@ -252,7 +252,7 @@ def list_trial_artifacts( optuna_artifact_metas = [ json.loads(value) for key, value in trial.system_attrs.items() - if key.startswith(OPTUNA_ARTIFACTS_ATTR_PREFIX) or key.startswith(ARTIFACTS_ATTR_PREFIX) + if key.startswith(ARTIFACTS_ATTR_PREFIX) or key.startswith(DASHBOARD_ARTIFACTS_ATTR_PREFIX) ] artifact_metas = dashboard_artifact_metas + optuna_artifact_metas return [a for a in artifact_metas if a is not None] From dae5f8f6e74363d5dddff395bb1a4b846f64e201 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Sat, 19 Aug 2023 16:42:15 +0900 Subject: [PATCH 12/16] Remove unnecessary search in trial_system_attr --- optuna_dashboard/artifact/_backend.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index 5f0ba2610..7a9f12422 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -131,9 +131,6 @@ def delete_artifact(study_id: int, trial_id: int, artifact_id: str) -> dict[str, storage.set_study_system_attr( study_id, _artifact_prefix(trial_id) + artifact_id, json.dumps(None) ) - storage.set_trial_system_attr( - trial_id, DASHBOARD_ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) - ) storage.set_trial_system_attr( trial_id, ARTIFACTS_ATTR_PREFIX + artifact_id, json.dumps(None) ) @@ -216,9 +213,7 @@ def get_artifact_meta( # have a different trial_system_attrs key prefix. # See https://github.com/optuna/optuna/blob/f827582a8/optuna/artifacts/_upload.py#L71 trial_system_attrs = storage.get_trial_system_attrs(trial_id) - value = trial_system_attrs.get( - DASHBOARD_ARTIFACTS_ATTR_PREFIX + artifact_id - ) or trial_system_attrs.get(ARTIFACTS_ATTR_PREFIX + artifact_id) + value = trial_system_attrs.get(ARTIFACTS_ATTR_PREFIX + artifact_id) if value is not None: return json.loads(value) @@ -252,7 +247,7 @@ def list_trial_artifacts( optuna_artifact_metas = [ json.loads(value) for key, value in trial.system_attrs.items() - if key.startswith(ARTIFACTS_ATTR_PREFIX) or key.startswith(DASHBOARD_ARTIFACTS_ATTR_PREFIX) + if key.startswith(ARTIFACTS_ATTR_PREFIX) ] artifact_metas = dashboard_artifact_metas + optuna_artifact_metas return [a for a in artifact_metas if a is not None] From 98c85451242a2592bf20153b0c141b6776554ee3 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Sat, 19 Aug 2023 17:00:14 +0900 Subject: [PATCH 13/16] Update comment --- optuna_dashboard/artifact/_backend.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index 7a9f12422..9ef79dad1 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -127,7 +127,7 @@ def delete_artifact(study_id: int, trial_id: int, artifact_id: str) -> dict[str, return {"reason": "Cannot access to the artifacts."} artifact_store.remove(artifact_id) - # The metadata of the artifact is stored in one of the following three locations: + # The artifact's metadata is stored in one of the following two locations: storage.set_study_system_attr( study_id, _artifact_prefix(trial_id) + artifact_id, json.dumps(None) ) From 182663158510741acb6bb0c5b660c28156a0a9ae Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Mon, 21 Aug 2023 12:32:45 +0900 Subject: [PATCH 14/16] Add unit tests --- python_tests/artifact/test_backend.py | 100 ++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 python_tests/artifact/test_backend.py diff --git a/python_tests/artifact/test_backend.py b/python_tests/artifact/test_backend.py new file mode 100644 index 000000000..d427d5d05 --- /dev/null +++ b/python_tests/artifact/test_backend.py @@ -0,0 +1,100 @@ +from unittest.mock import MagicMock + +from optuna_dashboard.artifact import _backend +import pytest + + +def test_get_artifact_path() -> None: + study = MagicMock(_study_id=0) + trial = MagicMock(_trial_id=0, study=study) + assert _backend.get_artifact_path(trial=trial, artifact_id="id0") == "/artifacts/0/0/id0" + + +def test_artifact_prefix() -> None: + actual = _backend._artifact_prefix(trial_id=0) + assert actual == "dashboard:artifacts:0:" + + +@pytest.fixture() +def init_storage_with_artifact_meta() -> MagicMock: + storage = MagicMock() + + get_study_system_attrs = ( + lambda study_id: { + "dashboard:artifacts:0:id0": '{"artifact_id": "id0", "filename": "foo.txt"}', + "dashboard:artifacts:0:id1": '{"artifact_id": "id1", "filename": "bar.txt"}', + "baz": "baz", + } + if study_id == 0 + else {} + ) + storage.get_study_system_attrs.side_effect = get_study_system_attrs + + trial0_system_attrs = { + "artifacts:id2": '{"artifact_id": "id2", "filename": "baz.txt"}', + } + trial1_system_attrs = { + "artifacts:id3": '{"artifact_id": "id3", "filename": "qux.txt"}', + } + get_trial_system_attrs = ( + lambda trial_id: trial0_system_attrs + if trial_id == 0 + else trial1_system_attrs + if trial_id == 1 + else {} + ) + storage.get_trial_system_attrs.side_effect = get_trial_system_attrs + + get_all_trials = ( + lambda study_id: [ + MagicMock( + _trial_id=0, study=MagicMock(_study_id=study_id), system_attrs=trial0_system_attrs + ), + MagicMock( + _trial_id=1, study=MagicMock(_study_id=study_id), system_attrs=trial1_system_attrs + ), + ] + if study_id == 0 + else [] + ) + storage.get_all_trials.side_effect = get_all_trials + + return storage + + +def test_get_artifact_meta(init_storage_with_artifact_meta: MagicMock) -> None: + storage = init_storage_with_artifact_meta + + actual = _backend.get_artifact_meta(storage, study_id=0, trial_id=0, artifact_id="id0") + assert actual == {"artifact_id": "id0", "filename": "foo.txt"} + + actual = _backend.get_artifact_meta(storage, study_id=0, trial_id=1, artifact_id="id3") + assert actual == {"artifact_id": "id3", "filename": "qux.txt"} + + actual = _backend.get_artifact_meta(storage, study_id=0, trial_id=0, artifact_id="id4") + assert actual is None + + +def test_delete_all_artifacts(init_storage_with_artifact_meta: MagicMock) -> None: + backend = MagicMock() + storage = init_storage_with_artifact_meta + _backend.delete_all_artifacts(backend, storage, study_id=0) + + assert backend.remove.call_args_list == [ + (("id0",),), + (("id1",),), + (("id2",),), + (("id3",),), + ] + + +def test_list_trial_artifacts(init_storage_with_artifact_meta: MagicMock) -> None: + storage = init_storage_with_artifact_meta + trial = MagicMock(_trial_id=0, system_attrs=storage.get_trial_system_attrs(0)) + + actual = _backend.list_trial_artifacts(storage.get_study_system_attrs(0), trial) + assert actual == [ + {"artifact_id": "id0", "filename": "foo.txt"}, + {"artifact_id": "id1", "filename": "bar.txt"}, + {"artifact_id": "id2", "filename": "baz.txt"}, + ] From 6322e76def3c6f326c0ab8fdeba08ed6931cd123 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Thu, 24 Aug 2023 13:05:43 +0900 Subject: [PATCH 15/16] Revert argument for list_trial_artifacts() in upload_artifact_api() Co-authored-by: c-bata --- optuna_dashboard/artifact/_backend.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/optuna_dashboard/artifact/_backend.py b/optuna_dashboard/artifact/_backend.py index 9ef79dad1..b83a8a0cf 100644 --- a/optuna_dashboard/artifact/_backend.py +++ b/optuna_dashboard/artifact/_backend.py @@ -116,7 +116,7 @@ def upload_artifact_api(study_id: int, trial_id: int) -> dict[str, Any]: return { "artifact_id": artifact_id, - "artifacts": list_trial_artifacts(storage.get_trial_system_attrs(trial_id), trial), + "artifacts": list_trial_artifacts(storage.get_study_system_attrs(study_id), trial), } @app.delete("/api/artifacts///") From af7abc913dd2f8a1dc97ce655f2bf6ee564c0352 Mon Sep 17 00:00:00 2001 From: Shinichi Hemmi <50256998+Alnusjaponica@users.noreply.github.com> Date: Fri, 25 Aug 2023 13:53:54 +0900 Subject: [PATCH 16/16] Replace MagicMock with actual storage --- python_tests/artifact/test_backend.py | 60 ++++++++++----------------- 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/python_tests/artifact/test_backend.py b/python_tests/artifact/test_backend.py index d427d5d05..e2d0f1526 100644 --- a/python_tests/artifact/test_backend.py +++ b/python_tests/artifact/test_backend.py @@ -1,5 +1,6 @@ from unittest.mock import MagicMock +from optuna.storages import BaseStorage from optuna_dashboard.artifact import _backend import pytest @@ -16,48 +17,29 @@ def test_artifact_prefix() -> None: @pytest.fixture() -def init_storage_with_artifact_meta() -> MagicMock: - storage = MagicMock() - - get_study_system_attrs = ( - lambda study_id: { - "dashboard:artifacts:0:id0": '{"artifact_id": "id0", "filename": "foo.txt"}', - "dashboard:artifacts:0:id1": '{"artifact_id": "id1", "filename": "bar.txt"}', - "baz": "baz", - } - if study_id == 0 - else {} - ) - storage.get_study_system_attrs.side_effect = get_study_system_attrs - - trial0_system_attrs = { - "artifacts:id2": '{"artifact_id": "id2", "filename": "baz.txt"}', +def init_storage_with_artifact_meta() -> BaseStorage: + from optuna import create_study + from optuna.storages import InMemoryStorage + + storage = InMemoryStorage() + study = create_study(storage=storage) + + study_system_attrs = { + "dashboard:artifacts:0:id0": '{"artifact_id": "id0", "filename": "foo.txt"}', + "dashboard:artifacts:0:id1": '{"artifact_id": "id1", "filename": "bar.txt"}', + "baz": "baz", } - trial1_system_attrs = { + for key, value in study_system_attrs.items(): + study.set_system_attr(key, value) + + trial_system_attrs = { + "artifacts:id2": '{"artifact_id": "id2", "filename": "baz.txt"}', "artifacts:id3": '{"artifact_id": "id3", "filename": "qux.txt"}', } - get_trial_system_attrs = ( - lambda trial_id: trial0_system_attrs - if trial_id == 0 - else trial1_system_attrs - if trial_id == 1 - else {} - ) - storage.get_trial_system_attrs.side_effect = get_trial_system_attrs - - get_all_trials = ( - lambda study_id: [ - MagicMock( - _trial_id=0, study=MagicMock(_study_id=study_id), system_attrs=trial0_system_attrs - ), - MagicMock( - _trial_id=1, study=MagicMock(_study_id=study_id), system_attrs=trial1_system_attrs - ), - ] - if study_id == 0 - else [] - ) - storage.get_all_trials.side_effect = get_all_trials + for key, value in trial_system_attrs.items(): + trial = study.ask() + trial.set_system_attr(key, value) + study.tell(trial, 0.0) return storage