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. [#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")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_ = logger.info(s"Deleting workflow with hash $workflowHash")
_ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}")

_ <- voxelyticsDAO.deleteWorkflow(workflowHash)
} yield Ok
}
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

The PR objectives mention concerns about deleting running workflows. Consider adding a check to prevent deletion of active workflows.

 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")
       _ <- voxelyticsDAO.deleteWorkflow(workflowHash)
     } yield Ok
   }

Committable suggestion was skipped due to low confidence.


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
13 changes: 13 additions & 0 deletions app/models/voxelytics/VoxelyticsDAO.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1135,4 +1135,17 @@ class VoxelyticsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContex
""".asUpdate)
} yield ()

def deleteWorkflow(hash: String): Fox[Unit] =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def deleteWorkflow(hash: String): Fox[Unit] =
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =

for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
WHERE hash = $hash;
WHERE hash = $hash;
AND _organization = $organizationId

""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowhash = NULL
WHERE _voxelytics_workflowhash = $hash;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SET _voxelytics_workflowhash = NULL
WHERE _voxelytics_workflowhash = $hash;
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;

""".asUpdate)
} yield ()

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider wrapping the delete operations in a transaction.

The workflow deletion involves multiple database operations that should be atomic to maintain data consistency. If the first operation succeeds but the second fails, it could leave the database in an inconsistent state.

Consider wrapping both operations in a transaction:

def deleteWorkflow(hash: String): Fox[Unit] =
  for {
-   _ <- run(q"""
-                 DELETE FROM webknossos.voxelytics_workflows
-                 WHERE hash = $hash;
-                 """.asUpdate)
-   _ <- run(q"""
-                 UPDATE webknossos.jobs
-                 SET _voxelytics_workflowhash = NULL
-                 WHERE _voxelytics_workflowhash = $hash;
-       """.asUpdate)
+   _ <- sqlClient.withTransaction { implicit client =>
+     for {
+       _ <- run(q"""
+         DELETE FROM webknossos.voxelytics_workflows
+         WHERE hash = $hash;
+         """.asUpdate)
+       _ <- run(q"""
+         UPDATE webknossos.jobs
+         SET _voxelytics_workflowhash = NULL
+         WHERE _voxelytics_workflowhash = $hash;
+         """.asUpdate)
+     } yield ()
+   }
  } yield ()

Committable suggestion was skipped due to low confidence.

}
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?\nNote that if the workflow is still running, this may cause it to fail.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Are you sure you want to delete this workflow report?\nNote that if the workflow is still running, this may cause it to fail.",
"Are you sure you want to delete this workflow report? This can not be undone.\nNote 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