Add frontend and backend part for job to materialize a volume annotation #6086

MichaelBuessemeyer · 2022-02-28T21:10:19Z

This PR adds the frontend and backend part for the job that applies the results of a merger mode tracing to a segmentation layer.

Open TODOs:

Properly support and test the support for tracings with multiple segmentation layers.
Add a preview image for the apply merger mode job
Test selecting from multiple segmentation layers and whether the worker also chooses the correct segmentation layer and applies the segment merging to it.
See my mentions below :)

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

As this pr is closely connected to the counter part pr in the worker's repo, the testing instructions can be taken from the wk-worker pr as they would be the same for this pr.

Issues:

fixes Allow applying result of merger mode tracing #4603

(Please delete unneeded items, merge only when none are left open)

Updated (unreleased) changelog
~~[ ] Updated (unreleased) migration guide if applicable~~
Updated documentation if applicable -> Add a small sentence + image on where to trigger the job
~~[ ] Adapted wk-connect if datastore API changes~~
~~[ ] Adapted wk-libs python client if relevant API parts change~~
~~[ ] Needs datastore update after deployment~~
Ready for review

MichaelBuessemeyer · 2022-02-28T21:11:11Z

app/models/job/Job.scala

@@ -72,7 +72,7 @@ case class Job(
          }
        case "export_tiff" =>
          Some(s"$dataStorePublicUrl/data/exports/${_id.id}/download")
-        case "infer_nuclei" | "infer_neurons" =>
+        case "infer_nuclei" | "infer_neurons" | "apply_merger_mode" =>


I am not sure, whether this is everything I need to do here for the new job. I did not test the jobs list view and therefore this is likely untested.

MichaelBuessemeyer

These are all the comments that seemed helpful to me. If you got questions about the code, just let me know.

frontend/javascripts/oxalis/view/action-bar/toolbar_view.js

MichaelBuessemeyer · 2022-02-28T21:18:24Z

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.js

 function StartingJobModal(props: StartingJobModalProps) {
  const isBoundingBoxConfigurable = props.isBoundingBoxConfigurable || false;
+  const chooseSegmentationLayer = props.chooseSegmentationLayer || false;


The main changes here and below are that I enabled StartingJobModal to not only select color layers but also segmentation layers when chooseSegmentationLayer=true is passed to the component. The changes are mainly removing "color" from that variable names as the layers the component now works with might also be of kind segmentation.

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.js

…-apply-merger-mode-job

make start job modals a form

Add potential volume annotation layer to params of merger mode job all potential annotation cases

…-apply-merger-mode-job

…t as the other job starting modals - Add volume annotation layer name to flood fill job to make it work when there are multiple annotation layers

philippotto

Great stuff! Didn't test yet, but the code looks good overall 👍 Mainly left feedback for naming and maybe usability.

conf/application.conf

frontend/javascripts/admin/admin_rest_api.js

frontend/javascripts/oxalis/view/action-bar/toolbar_view.js

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.js

Co-authored-by: Philipp Otto <[email protected]>

…-apply-merger-mode-job

…webknossos into add-apply-merger-mode-job

philippotto · 2022-04-06T09:51:27Z

Great stuff! Testing went mostly smooth, but I noticed a few things:

unfortunately, the downsampled segmentation layer did not match the existing color layer mags. this caused the This dataset contains multiple layers which differ in their resolution. Please convert the layers to make their resolutions match. error for me.
- I ran the job on l4 sample where the color layer has [1, 1, 1], [2, 2, 1], [4, 4, 2], [8, 8, 4], [16, 16, 8], but the new seg layer got [1, 1, 1], [2, 2, 1], [4, 4, 1], ....
- I see two options for fixing this:
  - call downsample_mag_list and pass the mags which existed for the old segmentation layer
  - or use this code from vx: https://github.com/scalableminds/voxelytics/blob/024839faa2d564f48d3dbdabf1a648389e77486c/voxelytics/commons/downsampling_helper.py#L152-L152 (ideally, this should be moved into wk-libs)
- the first option is easier. the second would be nicer for the long-term. but I'd rather recommend to use the first one.
no "View" link in the job page (I think you need to adapt the renderActions method)
the color layer could not be rendered correctly (see https://github.com/scalableminds/webknossos-worker/pull/95/files/742c9273deb45836e0709f16f81a33138d16e911#r843730822)
I'd change the icon for the "apply merger mode" button to <ExportOutlined />

MichaelBuessemeyer · 2022-04-25T09:44:30Z

@fm3 I think I need some help in this pr because I changed the long-running jobs routes a little. Do you think bumping the API version is needed here or can we somehow circumvent this?

Also: wklibs does not contain any methods that work with the backend long-running jobs routes, or does it? IF it does, wklibs might also need some slight adjustments.

Adapted wk-libs python client if relevant API parts change

MichaelBuessemeyer · 2022-04-25T09:51:13Z

TODOs

Check other long-running jobs and make them "overwrite proof" in case multiple jobs are running at the same time which have the same output dataset name given by the user
- Only the inferral jobs and the new merger mode job got the new property new_dataset_name. For both, I ensured before creating the output dataset to check whether the name is still available. If the name is not available anymore an index number is added to the name to avoid name collisions.

fm3 · 2022-04-25T09:51:17Z

As far as I know the /jobs/run routes are currently only called by the javascript front-end. Because of this, I’d say we do not need to provide backwards compatibility / bump the api version or adapt the python client

- and apply some feedback

…r mode job modal

…-apply-merger-mode-job

MichaelBuessemeyer

@daniel-wer I just added comments to the code parts I'd like some feedback on before merging. Could you please do that 🙏 🥨 ?

MichaelBuessemeyer · 2022-05-05T09:03:05Z

frontend/javascripts/admin/job/job_list_view.tsx

+        <span>
+          Materialize annotation for {job.layerName ? ` layer ${job.layerName} of ` : " "}
+          <Link to={`/datasets/${job.organizationName}/${job.datasetName}/view`}>
+            {job.datasetName}
+          </Link>
+          {job.mergeSegments
+            ? ". This includes merging the segments that were merged via merger mode."
+            : null}
+        </span>


Feedback about this part / formulation would be nice.

And whether it is readable enough or should be refactored in a certain way.

MichaelBuessemeyer · 2022-05-05T10:24:03Z

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

+export function OutputSegmentationLayerNameFormItem({
+  hasOutputSegmentationLayer,
+  layers,
+  additionalAllowedNames,
+}: OutputSegmentationLayerNameProps) {
+  return (
+    <Form.Item
+      label="Name of output segmentation layer"
+      name="outputSegmentationLayerName"
+      rules={[
+        { required: hasOutputSegmentationLayer },
+        {
+          min: 3,
+        },
+        {
+          pattern: /[0-9a-zA-Z_-]+$/,
+        },
+        {
+          validator: async (_rule, newOutputLayerName) => {
+            if (
+              layers.some((layer) => layer.name === newOutputLayerName) &&
+              !additionalAllowedNames.includes(newOutputLayerName)
+            ) {
+              const reason =
+                "This name is already used by another segmentation layer of this dataset.";
+              return Promise.reject(reason);
+            } else {
+              return Promise.resolve();
+            }
+          },
+        },
+      ]}
+      hidden={!hasOutputSegmentationLayer}
+    >
+      <Input />
+    </Form.Item>
+  );
+}
+


This new Form.Item is new and wasn't reviewed by @philippotto. Could you please do that @daniel-wer?

Could you explain the rationale behind additionalAllowedNames? I didn't understand why this is needed from looking at the code.

additionalAllowedNames is a list of names that are allowed to enter although the dataset already has a layer with that name. In the modal this is used to allow the user to let the output segmentation layer have the exact same name as the input segmentation layer. This is possible because the output segmentation layer replaces the input segmentation layer in the output dataset. Therefore non _corrected or something like this at the end of the output layer name is needed.

Does this behaviour make sense to you?

Ah yes that makes sense. One could also remove the extra parameter additionalAllowedNames and filter the layers before handing them as a prop to OutputSegmentationLayerNameFormItem (to filter out the segmentation layer that is replaced). A small comment could be added at the filtering spot to explain why this is done.
I feel like this would be cleaner and easier to understand, what do you think?

what do you think?

Sounds good to me :)

MichaelBuessemeyer · 2022-05-05T10:25:59Z

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

+  initialOutputSegmentationLayerName = `${
+    initialOutputSegmentationLayerName || "segmentation"
+  }_corrected`;
+  // TODO: Other jobs also have an output segmentation layer. The names for these jobs should also be configurable.


This TODO needs to be kept, as the new OutputSegmentationLayerNameFormItem should also be used for the inferral jobs. I did not do it in this pr to avoid making this pr even bigger.

please open a follow-up issue for this :)

issue: #6197

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

daniel-wer

Please check my comments. The biggest issue in my opinion is that the code assumes that there always is a fallback layer. In my understanding, that's not the case, annotations can also be created without a fallback layer 🤔

daniel-wer · 2022-05-05T11:43:21Z

frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx

@@ -457,6 +473,9 @@ class DatasetSettings extends React.PureComponent<DatasetSettingsProps, State> {
          <div className="flex-item">
            {hasHistogram && !isDisabled ? this.getEditMinMaxButton(layerName, isInEditMode) : null}
          </div>
+          <div className="flex-item">
+            {isVolumeTracing && !isDisabled ? this.getMergeWithFallbackLayerButton(layer) : null}


Not every volume annotation has a fallback layer. Shouldn't this be checked before showing this button?

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

…ayer without a fallback layer - also incoperate other feedback

MichaelBuessemeyer · 2022-05-05T13:24:33Z

@daniel-wer I incorporated your feedback, could you please check the changes 💇 ?

MichaelBuessemeyer · 2022-05-05T13:53:00Z

I also just changed the docs a little. Do you think this is enough documentation for the new job @daniel-wer @fm3?

daniel-wer

Please also see my comment regarding additionalAllowedNames.

docs/volume_annotation.md

Co-authored-by: Daniel <[email protected]>

- pass unallowed layer names directly to form item

MichaelBuessemeyer · 2022-05-05T16:25:03Z

@daniel-wer I finished your last feedback, could you please have a look again? Maybe Monday so this pr can be merged on that day?

philippotto

See my few nitpicks, but other than that everything looks great :)

docs/volume_annotation.md

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

philippotto · 2022-05-09T09:44:17Z

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

+  initialOutputSegmentationLayerName = `${
+    initialOutputSegmentationLayerName || "segmentation"
+  }_corrected`;
+  // TODO: Other jobs also have an output segmentation layer. The names for these jobs should also be configurable.


please open a follow-up issue for this :)

philippotto · 2022-05-09T09:46:43Z

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

+      // Filtering out the layer that is currently selected as this name is allowed
+      // because the output layer overwrites the selected layer.


Suggested change

// Filtering out the layer that is currently selected as this name is allowed

// because the output layer overwrites the selected layer.

// Filtering out the layer that is currently selected as this name is not allowed

// because the output layer would overwrite the selected layer.

if I understood it correctly

No sorry, that is exactly not the case here.
All layer names are forbidden !except! the name of the selected volume layer, because this volume layer will don't exist in the output dataset, but it will be replaced with the modified version of this layer. The modified version then will have the name given by the OutputSegmentationLayerNameFormItem.
The list of these names is passed to OutputSegmentationLayerNameFormItem so that this form item can check against these names. In the case of a match, the form item is not submittable / in an error state to prevent the job running in the backend from having problems because of duplicate layer names.

As you misunderstood this comment, do you have a different way to express this, that might be easier to understand / not misunderstand?

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

Co-authored-by: Philipp Otto <[email protected]>

…-apply-merger-mode-job

…webknossos into add-apply-merger-mode-job

philippotto · 2022-05-09T11:23:15Z

frontend/javascripts/oxalis/view/right-border-tabs/starting_job_modals.tsx

+      // Filtering out the layer that is currently selected as this name is allowed
+      // because the output layer overwrites the selected layer.


Suggested change

// Filtering out the layer that is currently selected as this name is allowed

// because the output layer overwrites the selected layer.

// Existing layer names may not be used for the output layer. The only exception is the name of the currently selected layer.

// If that layer name is chosen, the original layer data won't exist in the output dataset, since it will be replaced by the new output.

Maybe like this? Needs prettier-formatting, though. You could also keep the comment as is. My interpretation didn't really make sense, which is why I'd hope I won't question the veracity of the comment in the future 😆

// If that layer name is chosen, the original layer data won't exist in the output dataset, since it will be replaced by the new output.

Technically speaking, this is not correct. The selected layer will never directly exist in the output dataset, if the job has an output layer. Therefore, this is even the case if the layer name is not chosen. But your 3rd sentence reads like it is only the case when the layer name is chosen.

I rephrased your suggestion a little and made it more expressive:

// is the name of the currently selected layer. This layer is the only one not // copied over from the original dataset to the output dataset. // Therefore, this name is available as the name for the output layer name. // That is why that layer is filtered out here.

Makes sense 👍

add frontend and backend part for job to apply merger mode tracing

435f2f1

MichaelBuessemeyer commented Feb 28, 2022

View reviewed changes

renaming variable

ee1f591

MichaelBuessemeyer commented Feb 28, 2022

View reviewed changes

MichaelBuessemeyer added 3 commits March 21, 2022 16:40

Merge branch 'master' of github.com:scalableminds/webknossos into add…

4f7dcb0

…-apply-merger-mode-job

add annotation type as job parameter and make layer names more readable

db2998a

fix layer selection for merger mode job modal

719a98a

MichaelBuessemeyer mentioned this pull request Mar 30, 2022

Unify code of globalize floodfill modal and the starting jobs modals #6126

Closed

MichaelBuessemeyer added 4 commits March 31, 2022 11:32

add job output dataset name to start job modals

c2190cd

make start job modals a form

add output dataset name to nuclei & neuron & merger mode job params

a9fb85e

Add potential volume annotation layer to params of merger mode job all potential annotation cases

Merge branch 'master' of github.com:scalableminds/webknossos into add…

7f045f5

…-apply-merger-mode-job

refactor starting modal for flood fills to use the same root componen…

d0be7b9

…t as the other job starting modals - Add volume annotation layer name to flood fill job to make it work when there are multiple annotation layers

MichaelBuessemeyer changed the title ~~[WIP] Add frontend and backend part for job to apply merger mode tracing~~ Add frontend and backend part for job to apply merger mode tracing Mar 31, 2022

MichaelBuessemeyer self-assigned this Apr 4, 2022

MichaelBuessemeyer added frontend new feature backend labels Apr 4, 2022

Merge branch 'master' into add-apply-merger-mode-job

60a5acf

philippotto reviewed Apr 4, 2022

View reviewed changes

MichaelBuessemeyer and others added 5 commits April 5, 2022 12:32

Apply suggestions from code review

c252d40

Co-authored-by: Philipp Otto <[email protected]>

Merge branch 'master' of github.com:scalableminds/webknossos into add…

7512f3a

…-apply-merger-mode-job

apply pr feedback

0aa7e31

Merge branch 'add-apply-merger-mode-job' of github.com:scalableminds/…

9d16876

…webknossos into add-apply-merger-mode-job

disable jobs

7688824

MichaelBuessemeyer added 2 commits April 25, 2022 13:03

update docs

28a66f9

apply pr feedback

b81be12

MichaelBuessemeyer and others added 8 commits April 27, 2022 15:10

only show merger mode job button whe jobs are enabled

7601d6f

- and apply some feedback

Merge branch 'master' into add-apply-merger-mode-job

ef24b0a

add output segmentation layer name as user input field to apply merge…

5d3a7e9

…r mode job modal

add segmentation output layer name to apply merger mode job api call

04e4f96

Merge branch 'master' of github.com:scalableminds/webknossos into add…

2d740f7

…-apply-merger-mode-job

make merger job available for volume annotation without skeletons

740efd6

Merge branch 'master' of github.com:scalableminds/webknossos into add…

4ec2dc9

…-apply-merger-mode-job

fix returnLink of materialize job

a810b43

MichaelBuessemeyer changed the title ~~Add frontend and backend part for job to apply merger mode tracing~~ Add frontend and backend part for job to materialize a volume annotation May 5, 2022

MichaelBuessemeyer commented May 5, 2022

View reviewed changes

daniel-wer reviewed May 5, 2022

View reviewed changes

make materialize volume anotation job better compatible with volume l…

0bbbe0d

…ayer without a fallback layer - also incoperate other feedback

update doc images and add merge with fallback layer section to docs

310375f

daniel-wer reviewed May 5, 2022

View reviewed changes

docs/volume_annotation.md Outdated Show resolved Hide resolved

docs/volume_annotation.md Outdated Show resolved Hide resolved

docs/volume_annotation.md Outdated Show resolved Hide resolved

MichaelBuessemeyer and others added 2 commits May 5, 2022 16:56

Apply suggestions from code review

9f6a3e7

Co-authored-by: Daniel <[email protected]>

apply review feedback

c02083c

- pass unallowed layer names directly to form item

Merge branch 'master' into add-apply-merger-mode-job

edff465

philippotto approved these changes May 9, 2022

View reviewed changes

MichaelBuessemeyer and others added 5 commits May 9, 2022 12:22

Update docs/volume_annotation.md

b43250b

Co-authored-by: Philipp Otto <[email protected]>

Merge branch 'master' into add-apply-merger-mode-job

4d9ca99

apply pr feedback

4af34cf

Merge branch 'master' of github.com:scalableminds/webknossos into add…

dad50a8

…-apply-merger-mode-job

Merge branch 'add-apply-merger-mode-job' of github.com:scalableminds/…

b581c4b

…webknossos into add-apply-merger-mode-job

philippotto reviewed May 9, 2022

View reviewed changes

rephrase commennt to make it clearer

89bb93f

MichaelBuessemeyer merged commit 95ec9e1 into master May 9, 2022

MichaelBuessemeyer deleted the add-apply-merger-mode-job branch May 9, 2022 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add frontend and backend part for job to materialize a volume annotation #6086

Add frontend and backend part for job to materialize a volume annotation #6086

MichaelBuessemeyer commented Feb 28, 2022 •

edited

Loading

MichaelBuessemeyer Feb 28, 2022

MichaelBuessemeyer left a comment

MichaelBuessemeyer Feb 28, 2022

philippotto left a comment

philippotto commented Apr 6, 2022 •

edited by MichaelBuessemeyer

Loading

MichaelBuessemeyer commented Apr 25, 2022

MichaelBuessemeyer commented Apr 25, 2022 •

edited

Loading

fm3 commented Apr 25, 2022

MichaelBuessemeyer left a comment

MichaelBuessemeyer May 5, 2022

MichaelBuessemeyer May 5, 2022

daniel-wer May 5, 2022

MichaelBuessemeyer May 5, 2022

daniel-wer May 5, 2022

MichaelBuessemeyer May 5, 2022

MichaelBuessemeyer May 5, 2022

philippotto May 9, 2022

MichaelBuessemeyer May 9, 2022

daniel-wer left a comment

daniel-wer May 5, 2022

MichaelBuessemeyer commented May 5, 2022

MichaelBuessemeyer commented May 5, 2022

daniel-wer left a comment

MichaelBuessemeyer commented May 5, 2022

philippotto left a comment

philippotto May 9, 2022

philippotto May 9, 2022

MichaelBuessemeyer May 9, 2022 •

edited

Loading

MichaelBuessemeyer May 9, 2022

philippotto May 9, 2022

MichaelBuessemeyer May 9, 2022

philippotto May 9, 2022

		// Filtering out the layer that is currently selected as this name is allowed
		// because the output layer overwrites the selected layer.

Add frontend and backend part for job to materialize a volume annotation #6086

Add frontend and backend part for job to materialize a volume annotation #6086

Conversation

MichaelBuessemeyer commented Feb 28, 2022 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

Choose a reason for hiding this comment

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

philippotto commented Apr 6, 2022 • edited by MichaelBuessemeyer Loading

MichaelBuessemeyer commented Apr 25, 2022

MichaelBuessemeyer commented Apr 25, 2022 • edited Loading

fm3 commented Apr 25, 2022

MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-wer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented May 5, 2022

MichaelBuessemeyer commented May 5, 2022

daniel-wer left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented May 5, 2022

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Feb 28, 2022 •

edited

Loading

philippotto commented Apr 6, 2022 •

edited by MichaelBuessemeyer

Loading

MichaelBuessemeyer commented Apr 25, 2022 •

edited

Loading

MichaelBuessemeyer May 9, 2022 •

edited

Loading