Skip to content

Route to register aiModel #8127

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Route to register aiModel #8127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1c23368
Route to register aiModel
fm3 Oct 16, 2024
e6cc396
update schema with new model categories. skip id taken check for the …
fm3 Oct 16, 2024
1633aa3
Merge branch 'master' into register-ai-model
fm3 Oct 16, 2024
4aa7896
bump schema version
fm3 Oct 16, 2024
95bab3f
add uniqueness checks for name + id. add sql evolution for enum values
fm3 Oct 17, 2024
579f3d0
no transaction block in evolution
fm3 Oct 17, 2024
d41e7b1
Merge branch 'master' into register-ai-model
fm3 Oct 22, 2024
3eb81d8
Merge branch 'master' into register-ai-model
fm3 Oct 28, 2024
4987297
Merge branch 'master' into register-ai-model
fm3 Oct 28, 2024
d29f0d6
delete incompatible models in reversion
fm3 Oct 28, 2024
03a4ea3
Merge branch 'master' into register-ai-model
fm3 Oct 28, 2024
f76ec20
Merge branch 'master' into register-ai-model
fm3 Oct 29, 2024
6ae4355
Merge branch 'master' into register-ai-model
fm3 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions MIGRATIONS.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ User-facing changes are documented in the [changelog](CHANGELOG.released.md).

- [121-worker-name.sql](conf/evolutions/121-worker-name.sql)
- [122-resolution-to-mag.sql](conf/evolutions/122-resolution-to-mag.sql)
- [123-more-model-categories.sql](conf/evolutions/123-more-model-categories.sql)
32 changes: 32 additions & 0 deletions app/controllers/AiModelController.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ object UpdateAiModelParameters {
implicit val jsonFormat: OFormat[UpdateAiModelParameters] = Json.format[UpdateAiModelParameters]
}

case class RegisterAiModelParameters(id: ObjectId, // must be a valid MongoDB ObjectId
dataStoreName: String,
name: String,
comment: Option[String],
category: Option[AiModelCategory])

object RegisterAiModelParameters {
implicit val jsonFormat: OFormat[RegisterAiModelParameters] = Json.format[RegisterAiModelParameters]
}

class AiModelController @Inject()(
aiModelDAO: AiModelDAO,
aiModelService: AiModelService,
Expand Down Expand Up @@ -209,6 +219,28 @@ class AiModelController @Inject()(
} yield Ok(jsResult)
}

def registerAiModel: Action[RegisterAiModelParameters] =
sil.SecuredAction.async(validateJson[RegisterAiModelParameters]) { implicit request =>
for {
_ <- userService.assertIsSuperUser(request.identity)
_ <- dataStoreDAO.findOneByName(request.body.dataStoreName) ?~> "dataStore.notFound"
_ <- aiModelDAO.findOne(request.body.id).reverse ?~> "aiModel.id.taken"
_ <- aiModelDAO.findOneByName(request.body.name).reverse ?~> "aiModel.name.taken"
_ <- aiModelDAO.insertOne(
AiModel(
request.body.id,
_organization = request.identity._organization,
request.body.dataStoreName,
request.identity._id,
None,
List.empty,
request.body.name,
request.body.comment,
request.body.category
))
} yield Ok
}
Comment on lines +241 to +242
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Return the created model in the response.

For consistency with other controller methods, return the created model instead of an empty Ok.

-      } yield Ok
+      newModel <- aiModelDAO.findOne(request.body.id) ?~> "aiModel.notFound"
+      jsResult <- aiModelService.publicWrites(newModel)
+      } yield Ok(jsResult)

Committable suggestion was skipped due to low confidence.

Comment on lines +222 to +242
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing validations and improve error handling.

The method should validate:

  1. That dataStoreName is either "localhost" or "webknossos.org"
  2. That category, when provided, is a valid EM type
  3. Input field sizes to prevent DoS
 def registerAiModel: Action[RegisterAiModelParameters] =
   sil.SecuredAction.async(validateJson[RegisterAiModelParameters]) { implicit request =>
     for {
       _ <- userService.assertIsSuperUser(request.identity)
+      _ <- bool2Fox(Set("localhost", "webknossos.org").contains(request.body.dataStoreName)) ?~> "dataStore.invalid"
+      _ <- request.body.category.traverse(cat => bool2Fox(AiModelCategory.isValidEMType(cat)) ?~> "category.invalid")
       _ <- dataStoreDAO.findOneByName(request.body.dataStoreName) ?~> "dataStore.notFound"

Committable suggestion was skipped due to low confidence.


def deleteAiModel(aiModelId: String): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
Expand Down
7 changes: 7 additions & 0 deletions app/models/aimodels/AiModel.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,11 @@ class AiModelDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext)
q"UPDATE webknossos.aiModels SET name = ${a.name}, comment = ${a.comment}, modified = ${a.modified} WHERE _id = ${a._id}".asUpdate)
} yield ()

def findOneByName(name: String)(implicit ctx: DBAccessContext): Fox[AiModel] =
for {
accessQuery <- readAccessQuery
r <- run(q"SELECT $columns FROM $existingCollectionName WHERE name = $name AND $accessQuery".as[AimodelsRow])
parsed <- parseFirst(r, name)
} yield parsed

}
2 changes: 1 addition & 1 deletion app/models/aimodels/AiModelCategory.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ import com.scalableminds.util.enumeration.ExtendedEnumeration

object AiModelCategory extends ExtendedEnumeration {
type AiModelCategory = Value
val em_neurons, em_nuclei = Value
val em_neurons, em_nuclei, em_synapses, em_neuron_types, em_cell_organelles = Value
}
2 changes: 1 addition & 1 deletion app/utils/sql/SQLDAO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ abstract class SQLDAO[C, R, X <: AbstractTable[R]] @Inject()(sqlClient: SqlClien
case Some(r) =>
parse(r) ?~> ("sql: could not parse database row for object" + id)
case _ =>
Fox.failure("sql: could not find object " + id)
Fox.empty
Copy link
Member Author

Choose a reason for hiding this comment

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

We used Fox.empty already in many overwriting findOnes, but not this one in the superclass. I don’t think it will hurt, though, and this way I can distinguish between error and empty downstream.

}.flatten

@nowarn // suppress warning about unused implicit ctx, as it is used in subclasses
Expand Down
11 changes: 11 additions & 0 deletions conf/evolutions/123-more-model-categories.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

-- no transaction here, since ALTER TYPE ... ADD cannot run inside a transaction block

do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 122, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Strengthen the schema version assertion.

While the version check is good, it could be more robust by handling NULL values and providing a more descriptive error message.

-do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 122, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;
+do $$ 
+declare
+  current_version int;
+begin
+  SELECT schemaVersion INTO current_version FROM webknossos.releaseInformation;
+  ASSERT current_version IS NOT NULL AND current_version = 122,
+    'Expected schema version 122, but found ' || COALESCE(current_version::text, 'NULL');
+end; $$ LANGUAGE plpgsql;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 122, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;
do $$
declare
current_version int;
begin
SELECT schemaVersion INTO current_version FROM webknossos.releaseInformation;
ASSERT current_version IS NOT NULL AND current_version = 122,
'Expected schema version 122, but found ' || COALESCE(current_version::text, 'NULL');
end; $$ LANGUAGE plpgsql;


ALTER TYPE webknossos.AI_MODEL_CATEGORY ADD VALUE 'em_synapses';
ALTER TYPE webknossos.AI_MODEL_CATEGORY ADD VALUE 'em_neuron_types';
ALTER TYPE webknossos.AI_MODEL_CATEGORY ADD VALUE 'em_cell_organelles';
Comment on lines +6 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

These enum values already exist in schema.sql - migration will fail

The values 'em_synapses', 'em_neuron_types', and 'em_cell_organelles' are already defined in tools/postgres/schema.sql as part of the initial enum definition. Attempting to add these values again through the migration will cause an error since PostgreSQL doesn't allow duplicate enum values.

  • Remove these ALTER TYPE statements as these values are already part of the enum type definition in tools/postgres/schema.sql
🔗 Analysis chain

Verify enum values don't already exist and consider rollback strategy.

The ALTER TYPE commands look correct, but we should verify these values don't already exist to prevent errors. Also, consider documenting the rollback strategy since enum values cannot be easily removed.

Consider adding a comment block documenting the rollback strategy, as removing enum values requires careful handling:

-- Rollback strategy:
-- 1. Create a new enum type without these values
-- 2. Update all columns to use the new type
-- 3. Drop the old type
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if any of the new enum values already exist in the database

# Test: Search for existing enum definitions or usage
rg -i "em_synapses|em_neuron_types|em_cell_organelles" --type sql

# Test: Look for any previous migrations that might have added these values
fd -e sql | xargs grep -l "ALTER TYPE.*AI_MODEL_CATEGORY.*ADD VALUE"

Length of output: 688


UPDATE webknossos.releaseInformation SET schemaVersion = 123;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add verification for schema version update.

The schema version update should verify that exactly one row was updated to ensure the migration completed successfully.

-UPDATE webknossos.releaseInformation SET schemaVersion = 123;
+DO $$
+DECLARE
+  rows_affected int;
+BEGIN
+  WITH updated AS (
+    UPDATE webknossos.releaseInformation 
+    SET schemaVersion = 123 
+    WHERE schemaVersion = 122
+    RETURNING *
+  )
+  SELECT COUNT(*) INTO rows_affected FROM updated;
+  
+  ASSERT rows_affected = 1, 
+    'Expected to update exactly one row, but updated ' || rows_affected || ' rows';
+END $$;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
UPDATE webknossos.releaseInformation SET schemaVersion = 123;
DO $$
DECLARE
rows_affected int;
BEGIN
WITH updated AS (
UPDATE webknossos.releaseInformation
SET schemaVersion = 123
WHERE schemaVersion = 122
RETURNING *
)
SELECT COUNT(*) INTO rows_affected FROM updated;
ASSERT rows_affected = 1,
'Expected to update exactly one row, but updated ' || rows_affected || ' rows';
END $$;


11 changes: 11 additions & 0 deletions conf/evolutions/reversions/123-more-model-categories.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
START TRANSACTION;

do $$ begin ASSERT (select schemaVersion from webknossos.releaseInformation) = 123, 'Previous schema version mismatch'; end; $$ LANGUAGE plpgsql;

-- removing enum values is not supported in postgres, see https://www.postgresql.org/docs/current/datatype-enum.html#DATATYPE-ENUM-IMPLEMENTATION-DETAILS

UPDATE webknossos.aiModels SET isDeleted = TRUE WHERE category IN ('em_synapses', 'em_neuron_types', 'em_cell_organelles');

UPDATE webknossos.releaseInformation SET schemaVersion = 122;

COMMIT TRANSACTION;
1 change: 1 addition & 0 deletions conf/webknossos.latest.routes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ POST /aiModels/inferences/runInference
GET /aiModels/inferences/:id controllers.AiModelController.readAiInferenceInfo(id: String)
GET /aiModels/inferences controllers.AiModelController.listAiInferences
GET /aiModels controllers.AiModelController.listAiModels
POST /aiModels/register controllers.AiModelController.registerAiModel
GET /aiModels/:id controllers.AiModelController.readAiModelInfo(id: String)
PUT /aiModels/:id controllers.AiModelController.updateAiModelInfo(id: String)
DELETE /aiModels/:id controllers.AiModelController.deleteAiModel(id: String)
Expand Down
4 changes: 2 additions & 2 deletions tools/postgres/schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ CREATE TABLE webknossos.releaseInformation (
schemaVersion BIGINT NOT NULL
);

INSERT INTO webknossos.releaseInformation(schemaVersion) values(122);
INSERT INTO webknossos.releaseInformation(schemaVersion) values(123);
COMMIT TRANSACTION;


Expand Down Expand Up @@ -546,7 +546,7 @@ CREATE TABLE webknossos.emailVerificationKeys(
isUsed BOOLEAN NOT NULL DEFAULT false
);

CREATE TYPE webknossos.AI_MODEL_CATEGORY AS ENUM ('em_neurons', 'em_nuclei');
CREATE TYPE webknossos.AI_MODEL_CATEGORY AS ENUM ('em_neurons', 'em_nuclei', 'em_synapses', 'em_neuron_types', 'em_cell_organelles');

CREATE TABLE webknossos.aiModels(
_id CHAR(24) PRIMARY KEY,
Expand Down