core-bioimage-io-python icon indicating copy to clipboard operation
core-bioimage-io-python copied to clipboard

Better default io names in build_model()

Open FynnBe opened this issue 1 year ago • 12 comments

... Does it check for the names of the input&output layers when writing the rdf.yaml?

Originally posted by @esgomezm in https://github.com/bioimage-io/collection-bioimage-io/issues/571#issuecomment-1485578142

We could inspect the (TF) weights for better tensor_inputs and tensor_outputs default names in build_model()

FynnBe avatar Mar 28 '23 07:03 FynnBe

input/output layer names are important (https://github.com/bioimage-io/collection-bioimage-io/issues/571#issuecomment-1486480847) , but not always very informative or dependent on the architecture "conv2d_19".

I wonder if we shouldn't add them as fields to the corresponding weights entries (tensorflow_saved_model_bundle, keras_hdf5?, tensorflow_js?). This would be easy to test and could be generated by bioimageio.core for older format versions lacking this information. updating models with this information would be easy. @carlosuc3m @esgomezm @constantinpape should we separate this as a technical detail from the existing input/output names?

FynnBe avatar Mar 28 '23 09:03 FynnBe

It could be but I'm not sure how to avoid a misunderstanding between the current input and output fields

esgomezm avatar Mar 28 '23 09:03 esgomezm

I think I would prefer to have it in separate fields for the respective weight formats. But I don't have a strong opinion here, as I am not actively using any of these weight formats, so feel free to implement whichever solution works best for you.

constantinpape avatar Mar 28 '23 09:03 constantinpape

To illustrate the separation of the fields:

inputs: [{name: raw_input, ...}]
outputs: [{name: prediction, ...}]
weights:
  tensorflow_saved_model_bundle:
    input_layer_names: [input_1]
    output_layer_names: [conv2d_19]

edit: changed input_layer_name to input_layer_names list

developers using specific weights would find the layer names right there. higher-level names for model users (using our libraries) can abstract away the layer names, which are weight format dependent...

FynnBe avatar Mar 28 '23 09:03 FynnBe

why dont' we like to name the inputs and outputs with names like conv2d_19 or input_1?

carlosuc3m avatar Mar 28 '23 14:03 carlosuc3m

I always thought of the input and output tensor names as content descriptions, raw_input, em_volume, or even something like left_camera and right_camera for a two camera setup... i.e. identifiers you would use in code for those tensors. while input_1 is sufficiently generic to be anything, conf2d_19 is clearly the name of a convolutional layer --- no good name for the output tensor in my opinion. Add any of the optional postprocessing steps and the name is quite misleading. (note that we have general issues with the tensor referencing pre- and post any pre- or postprocessing step.)

A bit of a vague point, but it feels like mixing implementation detail with the abstraction layer defined by the RDF. And along those lines: In the model RDF all weight format specific details are in their respective weights entry. What if ONNX layers will also need names?

FynnBe avatar Mar 28 '23 18:03 FynnBe

one more scenario to illustrate the "mix" (yes, it's a bit of a contructed issue 😇 ): what if a model has pytorch an onnx weights with arbitrary names. In a new model version converted tf weights are added. This would entail that inputs and outputs need to be renamed, which is weird.

FynnBe avatar Mar 28 '23 18:03 FynnBe

That is a good point, but then I whink that we need a way to know which of the inputs of the model correspond to each of the inputs specified in the yaml

carlosuc3m avatar Apr 05 '23 12:04 carlosuc3m

That is a good point, but then I whink that we need a way to know which of the inputs of the model correspond to each of the inputs specified in the yaml

can't we simply list the network input names in the order of the model inputs? could you give a counter-example? or how could we define this instead? if we define it per weight format we can adjust for each format if necessary...

FynnBe avatar Apr 07 '23 08:04 FynnBe

can't we simply list the network input names in the order of the model inputs? could you give a counter-example? or how could we define this instead? if we define it per weight format we can adjust for each format if necessary...

As long as the order is kept the same I think that your suggestion will be enough yes @FynnBe . Is this already implemented in the specs?

carlosuc3m avatar May 22 '23 17:05 carlosuc3m

As long as the order is kept the same I think that your suggestion will be enough yes @FynnBe . Is this already implemented in the specs?

Hi @carlosuc3m , no it's not implemented yet, but I can make a draft today (edit: given that I find where those requirements are documented). You mention in https://github.com/bioimage-io/collection-bioimage-io/issues/540#issuecomment-1557647179 that ONNX actually requires specific input names, as well. Is this documented somewhere? Can one not simply provide the input tensors as positional arguments?

Same for TensorFlow: I'd like to make sure that we don't have the option of providing input tensors positionally. Where does this break exactly? (@esgomezm you encountered this (https://github.com/bioimage-io/collection-bioimage-io/issues/571#issuecomment-1486480847)... is this documented somewhere? ) For TensorFlow Javascript I only found

...
      const image = util.textToImageArray(imageText, this.imageSize);
      const predictOut = this.model.predict(image);

from https://www.tensorflow.org/js/tutorials/conversion/pretrained_model#explore_the_code which seems to get away without any tensor names. Is this specific to TensorFlow 1?

FynnBe avatar May 23 '23 07:05 FynnBe

@carlosuc3m in https://github.com/bioimage-io/collection-bioimage-io/issues/609#issuecomment-1610156170 how do you know the onnx names that you would need? Can we solve this with inspecting the onnx weights or finding another way to provide positional arguments? Adding the weights specific tensor name fields as mandatory fields feels a bit of an overkill, espcially because running, e.g. the onnx model in Python does not require those tensor names.

FynnBe avatar Aug 15 '23 12:08 FynnBe