Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for the deletion of workflow reports #8156

Merged
merged 11 commits into from
Nov 6, 2024
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Added a button to the search popover in the skeleton and segment tab to select all matching non-group results. [#8123](https://github.com/scalableminds/webknossos/pull/8123)
- Unified wording in UI and code: “Magnification”/“mag” is now used in place of “Resolution“ most of the time, compare [https://docs.webknossos.org/webknossos/terminology.html](terminology document). [#8111](https://github.com/scalableminds/webknossos/pull/8111)
- Added support for adding remote OME-Zarr NGFF version 0.5 datasets. [#8122](https://github.com/scalableminds/webknossos/pull/8122)
- Workflow reports may be deleted by superusers. [#8156](https://github.com/scalableminds/webknossos/pull/8156)

### Changed
- Some mesh-related actions were disabled in proofreading-mode when using meshfiles that were created for a mapping rather than an oversegmentation. [#8091](https://github.com/scalableminds/webknossos/pull/8091)
Expand Down
13 changes: 13 additions & 0 deletions app/controllers/VoxelyticsController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package controllers
import com.scalableminds.util.time.Instant
import com.scalableminds.util.tools.{Fox, FoxImplicits}
import models.organization.OrganizationDAO
import models.user.UserService
import models.voxelytics._
import play.api.libs.json._
import play.api.mvc._
Expand All @@ -19,6 +20,7 @@ class VoxelyticsController @Inject()(
organizationDAO: OrganizationDAO,
voxelyticsDAO: VoxelyticsDAO,
voxelyticsService: VoxelyticsService,
userService: UserService,
lokiClient: LokiClient,
wkConf: WkConf,
sil: Silhouette[WkEnv])(implicit ec: ExecutionContext, bodyParsers: PlayBodyParsers)
Expand Down Expand Up @@ -158,6 +160,17 @@ class VoxelyticsController @Inject()(
} yield JsonOk(result)
}

def deleteWorkflow(workflowHash: String): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
_ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled"
_ <- userService.assertIsSuperUser(request.identity)
_ <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
_ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}")
_ <- voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
} yield Ok
}
Comment on lines +163 to +172
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add validation for running workflows and improve error handling

The implementation needs several improvements for robustness:

  1. As discussed in the PR objectives, we should prevent deletion of running workflows or at least warn users
  2. The deletion operation should be wrapped in error handling
  3. Consider using a transaction to ensure atomicity of the deletion operation

Here's a suggested implementation:

 def deleteWorkflow(workflowHash: String): Action[AnyContent] =
   sil.SecuredAction.async { implicit request =>
     for {
       _ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled"
       _ <- userService.assertIsSuperUser(request.identity)
-      _ <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
+      workflow <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
+      isRunning <- voxelyticsDAO.isWorkflowRunning(workflowHash)
+      _ <- bool2Fox(!isRunning) ?~> "voxelytics.cannotDeleteRunningWorkflow" ~> CONFLICT
       _ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}")
-      _ <- voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
+      result <- Fox.runAndHandleErrors {
+        voxelyticsDAO.inTransaction { implicit session =>
+          voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
+        }
+      }
+      _ = if (result.isLeft) {
+        logger.error(s"Failed to delete workflow $workflowHash: ${result.left.get}")
+      }
+      _ <- bool2Fox(result.isRight) ?~> "voxelytics.deletionFailed" ~> INTERNAL_SERVER_ERROR
     } yield Ok
   }

Committable suggestion skipped: line range outside the PR's diff.


def storeWorkflowEvents(workflowHash: String, runName: String): Action[List[WorkflowEvent]] =
sil.SecuredAction.async(validateJson[List[WorkflowEvent]]) { implicit request =>
def createWorkflowEvent(runId: ObjectId, events: List[WorkflowEvent]): Fox[Unit] =
Expand Down
15 changes: 15 additions & 0 deletions app/models/voxelytics/VoxelyticsDAO.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1135,4 +1135,19 @@ class VoxelyticsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContex
""".asUpdate)
} yield ()

def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash
AND (SELECT _organization FROM webknossos.users AS u WHERE u._id = _owner) = $organizationId;
""".asUpdate)
} yield ()

}
1 change: 1 addition & 0 deletions conf/webknossos.latest.routes
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ POST /verifyEmail
POST /voxelytics/workflows controllers.VoxelyticsController.storeWorkflow()
GET /voxelytics/workflows controllers.VoxelyticsController.listWorkflows()
GET /voxelytics/workflows/:workflowHash controllers.VoxelyticsController.getWorkflow(workflowHash: String, runId: Option[String])
DELETE /voxelytics/workflows/:workflowHash controllers.VoxelyticsController.deleteWorkflow(workflowHash: String)
POST /voxelytics/workflows/:workflowHash/events controllers.VoxelyticsController.storeWorkflowEvents(workflowHash: String, runName: String)
GET /voxelytics/workflows/:workflowHash/chunkStatistics controllers.VoxelyticsController.getChunkStatistics(workflowHash: String, runId: Option[String], taskName: String)
GET /voxelytics/workflows/:workflowHash/artifactChecksums controllers.VoxelyticsController.getArtifactChecksums(workflowHash: String, runId: Option[String], taskName: String, artifactName: Option[String])
Expand Down
6 changes: 6 additions & 0 deletions frontend/javascripts/admin/admin_rest_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2501,6 +2501,12 @@ export function getVoxelyticsArtifactChecksums(
);
}

export function deleteWorkflow(workflowHash: string): Promise<void> {
return Request.triggerRequest(`/api/voxelytics/workflows/${workflowHash}`, {
method: "DELETE",
});
}

// ### Help / Feedback userEmail
export function sendHelpEmail(message: string) {
return Request.receiveJSON(
Expand Down
32 changes: 31 additions & 1 deletion frontend/javascripts/admin/voxelytics/task_list_view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,12 @@ import TaskView from "./task_view";
import { formatLog } from "./log_tab";
import { addAfterPadding, addBeforePadding } from "./utils";
import { LOG_LEVELS } from "oxalis/constants";
import { getVoxelyticsLogs } from "admin/admin_rest_api";
import { getVoxelyticsLogs, deleteWorkflow } from "admin/admin_rest_api";
import ArtifactsDiskUsageList from "./artifacts_disk_usage_list";
import { notEmpty } from "libs/utils";
import type { ArrayElement } from "types/globals";
import { useSelector } from "react-redux";
import type { OxalisState } from "oxalis/store";

const { Search } = Input;

Expand Down Expand Up @@ -272,6 +274,8 @@ export default function TaskListView({
const highlightedTask = params.highlightedTask || "";
const location = useLocation();

const isCurrentUserSuperUser = useSelector((state: OxalisState) => state.activeUser?.isSuperUser);

const singleRunId = report.runs.length === 1 ? report.runs[0].id : runId;

useEffect(() => {
Expand Down Expand Up @@ -421,6 +425,26 @@ export default function TaskListView({
}
}

async function deleteWorkflowReport() {
await modal.confirm({
title: "Delete Workflow Report",
content:
"Are you sure you want to delete this workflow report? This can not be undone. Note that if the workflow is still running, this may cause it to fail.",
okText: "Delete",
okButtonProps: { danger: true },
onOk: async () => {
try {
await deleteWorkflow(report.workflow.hash);
history.push("/workflows");
message.success("Workflow report deleted.");
} catch (error) {
console.error(error);
message.error("Could not delete workflow report.");
}
},
});
}

const overflowMenu: MenuProps = {
items: [
{ key: "1", onClick: copyAllArtifactPaths, label: "Copy All Artifact Paths" },
Expand All @@ -442,6 +466,12 @@ export default function TaskListView({
{ key: "5", onClick: showArtifactsDiskUsageList, label: "Show Disk Usage of Artifacts" },
],
};
if (isCurrentUserSuperUser)
overflowMenu.items?.push({
key: "6",
onClick: deleteWorkflowReport,
label: "Delete Workflow Report",
});

type ItemType = ArrayElement<CollapseProps["items"]>;

Expand Down