Option to invert color layer when starting neuron inferral #8796
Conversation
📝 WalkthroughWalkthroughAdds an optional invertColorLayer flag across backend routes/controllers and frontend APIs/forms, forwards "invert_color_layer" into inference job command arguments, propagates the flag from per-layer dataset configuration, and records the change in the unreleased changelog. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@fm3 I noticed you made the |
|
Existing jobs in the database from before this change will not have this parameter. So at least the job list view should be able to handle it not being present (not sure if anything needs to be changed for that). The worker defaults to false if it doesn’t receive the parameter. But I agree that all newly created jobs should have it. |
Right, thanks for pointing that out! From what I can tell from testing and the code, this is not affected by changes in this PR, because in the job list view, the type |
|
as for the testing, do you want to do that yourself (I think you have the setup with the worker, right?) or do you want me to setup the worker myself? |
|
Works for me 🎉 |
fm3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
conf/webknossos.latest.routes (1)
272-273: Consider parity: should mitochondria inference also honor invertColorLayer?If predicting on inverted data is meaningful for mitochondria, consider adding the same optional flag for consistency. If not applicable to that model, feel free to ignore.
If desired, apply this change:
-POST /jobs/run/inferMitochondria/:datasetId controllers.JobController.runInferMitochondriaJob(datasetId: ObjectId, layerName: String, bbox: String, newDatasetName: String) +POST /jobs/run/inferMitochondria/:datasetId controllers.JobController.runInferMitochondriaJob(datasetId: ObjectId, layerName: String, bbox: String, newDatasetName: String, invertColorLayer: Option[Boolean])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/AiModelController.scala(2 hunks)app/controllers/JobController.scala(4 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/api/jobs.ts(3 hunks)frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx(9 hunks)unreleased_changes/8796.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-13T09:06:12.362Z
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx:530-535
Timestamp: 2025-01-13T09:06:12.362Z
Learning: In the webknossos codebase, the evaluation settings for split-merger evaluation in neuron inference tasks (`useSparseTracing`, `maxEdgeLength`, `sparseTubeThresholdInNm`, `minimumMergerPathLengthInNm`) are designed to be optional parameters.
Applied to files:
frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (26)
unreleased_changes/8796.md (1)
1-2: LGTM!Clear and concise changelog entry that accurately describes the user-facing impact of the change.
app/controllers/JobController.scala (4)
205-209: LGTM!The function signature correctly adds the optional
invertColorLayerparameter to therunInferNucleiJobmethod.
226-227: LGTM!The
invertColorLayerparameter is correctly forwarded to the job command arguments asinvert_color_layer.
235-245: LGTM!The function signature correctly adds the optional
invertColorLayerparameter to therunInferNeuronsJobmethod.
275-275: LGTM!The
invertColorLayerparameter is correctly forwarded to the job command arguments asinvert_color_layer.app/controllers/AiModelController.scala (2)
51-52: LGTM!The
RunInferenceParameterscase class correctly adds the optionalinvertColorLayerfield.
200-201: LGTM!The
invertColorLayerparameter is correctly forwarded to the job command arguments asinvert_color_layer.frontend/javascripts/viewer/view/action-bar/starting_job_modals.tsx (10)
621-626: LGTM!The
SplitMergerEvaluationSettingstype is well-structured with all fields correctly defined as optional, which aligns with the requirement from the retrieved learnings.
1028-1029: LGTM!The datasetConfiguration is correctly retrieved to access layer-specific settings.
1050-1056: LGTM!The isInverted value is correctly retrieved from the layer configuration and passed to the startNucleiInferralJob function.
1083-1084: LGTM!The datasetConfiguration is correctly retrieved to access layer-specific settings for neuron segmentation.
1120-1121: LGTM!The datasetLayerConfiguration is correctly retrieved from the datasetConfiguration using the colorLayer name.
1128-1129: LGTM!The isInverted value is correctly passed to the startNeuronInferralJob function.
1162-1165: LGTM!The isInverted value and splitMergerEvaluationSettings are correctly passed to the startNeuronInferralJob function when split merger evaluation is enabled.
1240-1241: LGTM!The datasetConfiguration is correctly retrieved to access layer-specific settings for custom AI model inference.
1279-1280: LGTM!The layerConfiguration is correctly retrieved from the datasetConfiguration using the colorLayer name.
1289-1289: LGTM!The isInverted value is correctly passed as invertColorLayer to the runNeuronInferenceWithAiModelJob function.
frontend/javascripts/admin/api/jobs.ts (7)
14-14: LGTM!The import correctly brings in the SplitMergerEvaluationSettings type from the starting_job_modals module.
191-196: LGTM!The function signature correctly adds the invertColorLayer parameter to startNucleiInferralJob.
197-206: LGTM!The URLSearchParams implementation correctly builds the query string with the required parameters, including the conditional append of invertColorLayer.
209-218: LGTM!The function signature correctly adds invertColorLayer and replaces individual evaluation parameters with the SplitMergerEvaluationSettings type, improving the API design.
225-248: LGTM!The implementation correctly destructures the splitMergerEvaluationSettings object and conditionally appends the evaluation parameters to the URL when split merger evaluation is enabled.
249-249: LGTM!The invertColorLayer parameter is correctly appended to the URL parameters.
398-398: LGTM!The RunInferenceParameters type correctly includes the optional invertColorLayer field.
conf/webknossos.latest.routes (2)
272-273: invertColorLayer added to inference routes: correct and backward-compatibleAppending
invertColorLayer: Option[Boolean]to both routes is aligned with the PR’s intent, preserves existing callers (optional), and keeps naming consistent with other flags. No concerns from the routes side.
272-273: All inference flows correctly include invertColorLayer end-to-endVerified that:
- Controller signatures in
JobController.scalaend withinvertColorLayer: Option[Boolean]for bothrunInferNucleiJobandrunInferNeuronsJob.- Routes (
conf/webknossos.latest.routeslines 272–273) match these signatures.- Admin frontend (
frontend/javascripts/admin/api/jobs.ts) appendsinvertColorLayerviaURLSearchParamsfor/inferNucleiand/inferNeurons.- Custom-model flow (
RunInferenceParametersinAiModelController.scalaandrunNeuronInferenceWithAiModelJob) includesinvertColorLayer?: booleanin JSON, and the backend maps it to"invert_color_layer".Everything is aligned—no further changes needed.
|
@fm3 I just cleaned up the code a bit. This shouldnt have changed anything I think, but can you make sure it hasnt? The request dont seem to have changed I think |
|
Yep, still works for me :) |
|
Two comments:
|
|
I agree, this is not ideal. However, I think this variant would already help for the moment. If I see it correctly, the worker job train_model does not yet take this parameter, so we can’t quickly add it here. Also, train model can run on multiple annotations, so we’d have to propagate this for each annotation individually. If we redesign the whole thing and let the libs query the view configuration, this could possibly lead to some problems too
An alternative might be to send the whole current view configuration json with the job parameters? What do you think @hotzenklotz ? |
|
My comments were not meant to block this PR. I am just stating the issues I am seeing. Your points with regards to the view configuration are valid. That being said, the raised issues exist regardless of whether the frontend or the libs decided to provide the view configuration. In both cases, we have to make a decision. |
|
Frontend LGTM |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/custom_ai_model_inference_form.tsx (1)
58-69: Guard against missing layer config; default invert to falseAccessing
datasetConfiguration.layers[colorLayer.name]can throw if the layer is missing. Defaulting preserves backward compatibility (previously “not inverted”).- const layerConfiguration = datasetConfiguration.layers[colorLayer.name]; + const isInverted = + datasetConfiguration?.layers?.[colorLayer.name]?.isInverted ?? false; @@ - invertColorLayer: layerConfiguration.isInverted, + invertColorLayer: isInverted,
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (1)
25-147: Reduce duplication: factor out a helper to read per-layer inversionThree forms now replicate the same pattern. Consider a small util to keep things DRY.
I can prepare a tiny helper like:
export const getLayerIsInverted = (cfg: DatasetConfiguration, layerName: string) => cfg?.layers?.[layerName]?.isInverted ?? false;and replace the inlined lookups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/controllers/AiModelController.scala(2 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/api/jobs.ts(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/custom_ai_model_inference_form.tsx(4 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/AiModelController.scala
- conf/webknossos.latest.routes
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx (2)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/admin/api/jobs.ts (1)
startNucleiInferralJob(191-205)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/custom_ai_model_inference_form.tsx (1)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (2)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/admin/api/jobs.ts (1)
startNeuronInferralJob(207-253)
frontend/javascripts/admin/api/jobs.ts (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/components/collapsible_split_merger_evaluation_settings.tsx (1)
SplitMergerEvaluationSettings(4-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (9)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/custom_ai_model_inference_form.tsx (2)
22-22: LGTM: selecting datasetConfiguration from stateSelection looks correct and consistent with other forms.
80-81: LGTM: dependency list updatedIncluding
datasetConfigurationensures recomputation on config changes.frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx (2)
16-16: LGTM: selecting datasetConfiguration from state
50-51: LGTM: dependency list includes datasetConfigurationfrontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (2)
29-30: LGTM: selecting datasetConfiguration from state
99-108: Ensure annotationId presence when split-merger evaluation is enabled
startNeuronInferralJobthrows ifannotationIdis missing. We check skeletons, but notannotationIdexplicitly.Add a quick guard before the final call:
+ if (annotationId == null) { + Toast.error("Missing annotationId for split/merger evaluation."); + return; + }frontend/javascripts/admin/api/jobs.ts (3)
418-419: LGTM: invertColorLayer added to BaseModelInferenceParametersConsistent with FE forms and backend JSON payload.
191-205: All call sites correctly pass the newinvertColorLayerflag
I ran a project-wide search forstartNucleiInferralJob(and confirmed the only invocation infrontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsxat line 48 now passeslayerConfiguration.isInvertedas the fourth argument. No other usages exist, so the code change is fully covered.
217-253: Resolved: Backend parameter names match frontend usageI’ve verified that the Play route and controller signature in conf/webknossos.latest.routes and app/controllers/JobController.scala declare exactly the same query parameters—
evalUseSparseTracing,evalMaxEdgeLength,evalSparseTubeThresholdNm, andevalMinMergerPathLengthNm—that the frontend is appending. No changes are needed here.
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx
Show resolved
Hide resolved
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (1)
68-77: Guard against undefined config and pass a single, normalized boolean to both code pathsAccessing datasetConfiguration.layers[colorLayer.name] and then .isInverted can throw if any link is missing; also compute once and reuse so both branches stay consistent.
Apply:
- const layerConfiguration = datasetConfiguration.layers[colorLayer.name]; + const isInverted = + datasetConfiguration?.layers?.[colorLayer.name]?.isInverted ?? false; @@ - layerConfiguration.isInverted, + isInverted, @@ - layerConfiguration.isInverted, + isInverted,Also applies to: 99-108
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (1)
68-68: Prefer the currently visible inversion state over persisted defaults (if available)If colorLayer exposes the live isInverted flag from the viewer, prefer that and fall back to datasetConfiguration; this matches the PR goal of using “currently visible state.”
Minimal tweak:
- const isInverted = - datasetConfiguration?.layers?.[colorLayer.name]?.isInverted ?? false; + const isInverted = + (colorLayer as any)?.isInverted ?? + datasetConfiguration?.layers?.[colorLayer.name]?.isInverted ?? + false;Please verify that selectedLayer/colorLayer actually carries isInverted; if not, keep the first variant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
frontend/javascripts/admin/api/jobs.ts(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx(3 hunks)frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/nuclei_detection_form.tsx
- frontend/javascripts/admin/api/jobs.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (2)
frontend/javascripts/libs/react_hooks.ts (1)
useWkSelector(292-294)frontend/javascripts/admin/api/jobs.ts (1)
startNeuronInferralJob(207-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/view/action-bar/ai_job_modals/forms/neuron_segmentation_form.tsx (2)
29-29: Selector addition looks fineUsing datasetConfiguration here is reasonable; you also added it to the dependency list below, avoiding stale captures.
110-117: Good dependency coverageIncluding datasetConfiguration in jobApiCall’s deps is correct; it ensures inversion changes retrigger the callback.
3 participants
If the color layer is currently displayed with inverted intensities, also invert it for the neuron inferral.
Steps to test:
(I tested this locally)
yarn enable-jobsinvert_color_layer: trueinPredictTaskTODOs:
Issues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)