genericworkflownodes / GenericKnimeNodes

Base package for GenericKnimeNodes
https://github.com/genericworkflownodes/GenericKnimeNodes
Other
15 stars 16 forks source link

Optional Output ports #129

Closed jpfeuffer closed 7 years ago

jpfeuffer commented 8 years ago

Some tools with optional output ports, perform different (and/or additional) tasks, when a filename is given as this optional parameter. Since KNIME always sets the filename of all outports to a temporary/internal file, the optional tasks of these tools are always executed. Since the nodes do not know about their "connectivity", inferring if this output is actually wanted can not be done in this way. A solution where we add an additional boolean parameter to the tool itself might be a lot of work. Nonetheless, we could try in the GKN to add such a bool param, whenever an optional outport is present. I am not sure yet, how this unused port will be handled in this case.

jpfeuffer commented 8 years ago

I started on an implementation on this and it seems to work out. Nonetheless, I wanted to ask @aiche for some input before I invest more time into this. My plan was:

I am not sure, if this whole problem is too OpenMS specific but I think several tools handle optional outputs by including/omitting specifying the optional output filename (and not via an additional boolean parameter).

jpfeuffer commented 8 years ago

Another suggestion by @timosachsenberg was to use the OutputTypes register card and a None or Inactive selection.

aiche commented 8 years ago

I see two main challenges here.

First one is the UI part, of how to communicate the optional output files. The idea of @timosachsenberg would be an option. Alternatively you could embed this in an own tab on the dialog.

The second problem is too properly communicate the fact, that no file will be generated to the following nodes. I would guess that you will need to embed this information into the PortObjectSpec. But I'm not sure here, it could also be sufficient to simply send null instead of a valid PortObjectSpec. But I guess, this would lead to an invalid configure result.

@jpfeuffer we can also discuss this in more detail via telephone.

jpfeuffer commented 8 years ago

See PR #133 . I did not include code for a new "ActivityChooser" tab yet and instead included the options into the MimeTypeChooser as proposed by Timo. However a new one might indeed be cleaner. It requires some more effort though. I used the (newish) InactivePort... specs to mark the Ports as deactivated in the GUI. This also results in deactivation of the whole downstream branch if you actually connect something to this port. See the IF-Switch Node by KNIME. With this PR we can discuss the progress of this issue a bit better.

jpfeuffer commented 8 years ago

There still might be some issues. Do not close. 1) Although the ports are 'X'ed out after configuration in my KNIME SDK 3.1, this is not the case in the production KNIME 3.2.1. In this version, the ports only get 'X'ed out visually after execution. (I have to check with the same version and ask if there is a difference between SDK and production, which I doubt). This is not a blocker but Xs at config time would be nicer. 2) I have experienced cases, where the tools (despite Inactive outputs) still try to execute the optional behaviour. I have to check if it's the C++ tools fault or if the params given by KNIME are not yet fully correct.

jpfeuffer commented 7 years ago

Okay, I could not reproduce 2 anymore. After #137 and #140 .

  1. still holds but I am contact with the KNIME guys.
jpfeuffer commented 7 years ago

I think this is basically fixed. There is another PR waiting to remodel the representation but I haven't had problems in a while. Will close it for now.

  1. Was a problem with an older plugin version loaded in my SDK.