Use bioimageio.core.build_spec to generate stardist export #171
Conversation
|
Thanks a lot @constantinpape ! Just a small comment now:
Yes, I agree - having is as an optional dependency and testing on import makes sense imho |
I made it optional now, but install it in the CI so that we can see whether tests are passing here. |
|
@uschmidt83 I implemented some of the changes we discussed in the meeting now. I.e. the Edit: for some context: |
We never work directly with tensorflow model, hence this doesn't exist in the repo. I managed to get it to work though, see the attached notebook. |
|
Thanks for the notebook @uschmidt83, it is super helpful for debugging. I can run prediction with the exported model in our bioimageio python library now; however the shapes of the expected and resulting output tensor do not match:
The input shape is (512, 512). Are you downsampling the input before prediction and then upsampling it again after running prediction? |
We build a new Keras model with concatenated outputs and such that the output size matches the input size, even if the model output is subsampled (
(This wasn't a permalink and is now pointing somewhere else.)
The output size of our models is smaller by a factor of |
|
Thanks for the clarification @uschmidt83. The issue was that I compared the predictions from the keras model (which are downsampled by the factor I have fixed that now and can run prediction and get outputs of the same shape. However, the results don't quite match numerically. My current guess is that this is due to differences between the percentile based normalization here and in |
|
I had a closer look now and indeed the differences are due to different behaviour of the normalization. |
|
This is working locally now. (Needs the changes in bioimage-io/core-bioimage-io-python#142) |
|
Hi there! I'll try to follow part of the discussion we got by email:
For the losses etc. I might be missing something but this is the way I see it: The models shared in the BMZ are always attached to a consumer (Ilastik, StarDist etc.), rather than models that you download and directly use in python by something like As suspected then, we don't export the losses to avoid any problem with the validator. We create a new model without compilation (currently the notebook is concatenating the channels, but I will correct that): from keras.models import Model
inputs = stardist_model.keras_model.inputs # model is the trained stardist model
outputs = stardist_model.keras_model.outputs
new_model = Model(inputs = inputs, outputs = outputs)This is the model that we export as keras and tf models.
As you wish. I'm exporting the config.json entirely so it can be read in python, but this will always depend on StarDist requirements.
I think, but correct me if I'm wrong, that exporting the model as I wrote before, does not include the downsampling part. In the case of deepImageJ, we store the grid value and use it as part of the pre/post-processing in the macro file to up/down-sample the image whenever is needed. |
|
Thanks for your comments @esgomezm.
I don't really agree with this. Ideally models should be cross-compatible and run in as many consumers as possible.
I think we can just do both in the beginning: include the content of the
I think the way you export it includes the downsampling part, because this is done by the model itself. I have also uploaded a model produced with the export function from this PR: https://oc.embl.de/index.php/s/riZiXn5GeYi8C9j |
Sure, my fault, I was not very specific. Models should be cross-compatible for inference, which is the only functionality we support at the moment and for which loss functions or the optimizers are dispensable. Training a StarDist model is not supported by any of the consumers (except, of course, StarDist). I'm assuming that the main reason to keep the loss functions is to enable further fine-tunning and training, but that, from the BMZ site, shouldn't be promoted except if you do it in StarDist (because we do not support cross-compatible training). When I said that the model is supposed to be loaded in StarDist, I was referring to the training. It's the same as with all the ZCDL4M notebooks. None of them is exported with their own loss functions. Otherwise, they cause many compatibility problems while the loss function is not used, except in the notebook. In the limit we could also say something similar with the post-processing as it's not really covered by our specifications but still we're trying to link as much consumers as we want, always knowing which are the limitations on each.
Ok, thank you! I was not using that function but just exporting the model directly with TF saved model functions. |
|
Thanks for the clarification @esgomezm. Regarding the inference/training part we fully agree and I think there was just a misunderstanding: Regarding the downsampling: I think this is something we would need to discuss with @uschmidt83 and @maweigert; I currently don't really understand when it is used and when not. |
Thanks, I updated this locally.
Do you get I think that's not the reason, rather TensorFlow is in a bad state after running |
StarDist produces downsampled outputs (parameter Oh, and we also concatenate the two StarDist outputs (prob and dist) for the Fiji plugin. Does that help? |
Can you share a code snippet with me? The TensorFlow API changed many times, and we might be using an old way of doing this in StarDist/CSBDeep. |
I'm testing this release right now on Ubuntu 20.04 (without GPU acceleration). I still see the same behavior when adding a model (dialog quickly appears and disappears), but the model seems to be actually installed this time. It was quite confusing to me that there was no user feedback whether a model was successfully installed or not. Unfortunately, I get an error when I open "DeepImageJ Run", select my newly-installed model, and then click on "Test model". First, nothing happens but one CPU core is used 100%. After a couple of minutes, the Fiji Console pops up with the following error message: Expand to show long error messageAm I doing something wrong? Can someone else test this? I've uploaded the model here (link expires in 7 days). |
|
Hello @uschmidt83. As you mentioned the test model button is not working correctly. I will fix that over the weekend. |
|
Hello again @uschmidt83 , |
|
Thanks for looking into this @carlosuc3m. @uschmidt83 I think we can assume it works now in deepImageJ. Should we go ahead and merge it? (Also there are still some failing tests, but this seems to be unrelated to the changes here.) |
Ah, I didn't expect that DeepImageJ works like this. Wouldn't it be more intuitive to have separate menu items for model testing and running?
I can confirm that model testing and running on a different opened image works for me now.
There's one issue I don't understand yet. Why did we bother to put the When I did run the postprocessing macro from the Script Editor (opened manually from the
Yes, those are unrelated. The bioimage.io tests are only active in Github action tests with |
Great. thanks for checking!
We want to support this in deepImageJ but are currently a bit out of manpower to implement it. There's probably a relatively simple solution. But also with the given solution, the script is at least packaged with the model, so that it can be applied manually.
@esgomezm @carlosuc3m do you have any idea how to fix this? |
Why don't you run the postprocessing from deepimagej? The deepImageJ model should run this script as the postprocessing. If it doesn't do so is probably because it's not specified in the config:
deepimagej:
prediction:
preprocess:
- spec: ij.IJ::runMacroFile
kwargs: per_sample_scale_range.ijm
postprocess:
- spec: ij.IJ::runMacroFile
kwargs: StarDist_Post-processing.ijm
The script splits the output of the network into two images and renames them. Maybe the renaming in the macro is written before the split? I can take a look at it and update the script if you send the model again. |
Hey @esgomezm, we decided not to include it in post-processing directly for now because it will introduce issues with the test output (which does not apply post-processing). We can't tackle this part right now, so it would be good to move ahead with the current solution, where the postprocessing script is attached and can then be manually applied by the user.
Yes, it would be great if you could have a look into this. The most recent model is available here: https://oc.embl.de/index.php/s/Dkt2agj75vlW1Ju |
No, it's not listed as
I don't think that's it, because it does work the second time i run it. Here's the macro: stardist/stardist/bioimageio_utils.py Lines 10 to 52 in 58ca216 |
Exactly, it's for CI reasons (otherwise this model would break the upcoming deepImageJ CI). See also my comment above. |
|
On second thought, we could also just include it in the post-processing and live with a fact that it will fail in the deepImageJ CI; I don't really know what the current status of setting it up is right now and if we will have it in time for the paper. What's the status of this, @oeway @carlosuc3m? |
|
Ok, this came up in the meeting again today and we decided to stick with the current solution in order to have a cross-compatible model that passes all checks. @uschmidt83
Could you maybe make a few screenshots or a video to show exactly what is not working? |
|
Hi @uschmidt83, Also, please let us know what the exact issue with the ImageJ script is, see #171 (comment). Thanks! |
I don't understand, why did you make a new PR to update your own PR here?
I tried to reproduce this today, but wasn't able to. I once got a different error message, but it did work all the other times. |
To have a clean diff that you can look at; the PR here is getting a bit convoluted.
Ok, just let me know if you have any questions regarding the changes or if this is good to go from your side.
👍 |
Just make the changes in a single commit, that's easy enough to review and edit. Thanks. |
Update bioimageio export function to be compatible with new bioimagei…
Ok, I merged it; let me know if this is ok with you or if you see any necessary changes (commit 7d7e4f8). |
|
The changes make sense, thanks @uschmidt83. |
|
Thanks for merging this @uschmidt83! |
This PR uses
bioimageio.core.build_modelto generate a bioimageio compatible model. Superseeds #149.The function runs through, but test still fails because the test input / output are saved with the wrong number of dimensions.
Some other points:
bioimageio.coreas hard dependency; instead it could also be added as optional dependencies and import could be nested intry: ... except ImportError.config:stardist. This way the model can properly checked withbioimageio.coreand instance segmentation can be applied as custom postprocessing by consumers that support it.Needs bioimage-io/core-bioimage-io-python#138.
cc @uschmidt83 @maweigert