Mismatched layout file terminates workflow with error when using 'visualizermapping'

[bruno-f-cruz]

Not fully sure how to reproduce this bug yet.
A combination of an older layout file with a recent workflow using visualizermapping seems to induce the following error when attempting to start the workflow:

The issue seems to be fixed by deleting the bonsai.layout file and reloading the workflow.
Deleting the node without reloading the workflow does not fix the issue either.

In the future it might be worth implementing a fail-safe routine to prevent these deadlocks. Acknowledging that the workflow cannot automatically recover the layout state, and simply reset it to allow the user to run the workflow at the cost of having to create a new layout from scratch.

[glopesdev]

@bruno-f-cruz looking further into this, this is probably a slightly more complicated case. There is already a check in place for failures of loading the .layout XML file but this is not what is happening here.

In this case this issue results from an incorrect mix-up of visualizers, where because of structural changes in the .bonsai file a visualizer gets assigned to an incompatible node type, e.g. TableLayoutPanelVisualizer gets assigned to an image node. This means that the visualizer code would get incompatible types when either Load or Show gets called, and at the moment this would only be detected at runtime.

Presenting a question modal dialog when starting the workflow is not ideal, and I think the correct way to resolve this would be at workflow loading time.

Doing this would require a slightly more complicated validation of the .layout file, where the type of each visualizer in the layout gets checked against the available visualizer types in each node. If a mismatch is detected, loading of the layout would be rejected, and we could then display the modal dialog giving the option to load without layout.

This check still wouldn't protect against all structural rearrangements, e.g. if the type of the visualizer just happens to be compatible with all the rearranged operator types, but at least it wouldn't crash with an error.

[glopesdev]

@bruno-f-cruz Given that this issue only happens when you start the workflow, we could also introduce a separate editor feature where you can simply reset the entire layout of a workflow, e.g. via a menu option. Would this be useful as an alternative?

[PathogenDavid]

Would this be useful as an alternative?

I'd worry that the majority of users wouldn't know to activate this feature without someone telling them to do it. Unless you know about Bonsai internals it's not really apparent that the exception is being caused by the layout.

(If people get used to just clicking it whenever they have a mysterious run error, it also makes it a little problematic if we remove it upon implementing a more robust layout system.)

As an aside I just noticed this exception is going through the fallback non-workflow exception case, which might be worth investigating in and of its self.

[glopesdev]

As an aside I just noticed this exception is going through the fallback non-workflow exception case

@PathogenDavid yup, I noticed that too, this is why it is actually popping up as a blocking dialog to users. If the visualizers were not getting launched at start-up I imagine they would still get a dialog when double-clicking the node, but it wouldn't stop the workflow from running.

That is another alternative we can go for in the meantime. The motivation for not crashing on visualizer errors when double-clicking comes from the fact that visualizers originally were used for inspecting what is happening with the workflow at runtime, and most likely one wouldn't want things to crash following a strictly visualizer error when you are just randomly poking around.

We could extend this even to visualizers that show on launch, so the workflow would still run but with affected visualizers closed. The only issue to consider is that visualizers have been more and more doubling down as GUI "frontends", and in this kind of applications they are required for operation, in which case you may indeed want the exception to prevent launch.

At some point we may want to draw a more clear separation between this usage of a "frontend" visualizer and a visualizer that is just there on-demand for you to poke at stuff. We could have an operator just for this that always forces the launch of a visualizer, but this is all a separate discussion.

[bruno-f-cruz]

Even if there is no provided functionality to resolve the deadlock, I think that at a bare minimum, the error message should provide information as to what the cause of the error might be. I agree with @PathogenDavid on the button argument, but I think that the same users who would use the button do not get what the current error message implies anyway.

Could we just provide users with additional information (be it in the error dialog or even in the console): "Bonsai could not load the default layout file for the current workflow. Deleting the layout file might solve the issue".

[glopesdev]

@bruno-f-cruz The problem is that we can't really be sure what the issue is. From the point of view of the editor, this is just an exception raised by the visualizer code, which could be a number of things, including a typo by the developer.

I will have a look if there are other checks we can make to detect this early and break out.

[bruno-f-cruz]

Ah I see. I thought the error could be scoped to a dependency in the layout file, but you are suggesting that even without a layout file the error may arise due to errors in the workflow file itself. That does indeed make things a bit harder...

[glopesdev]

Not even just the workflow file, but in the actual visualizer code. But I have an idea I want to try that might be able to catch this particular issue.

[glopesdev]

No time to pick this up yet, but for future reference, the idea was to replace the below line in SetVisualizerLayout:

bonsai/Bonsai.Editor/GraphView/WorkflowGraphView.cs

Line 711 in c5db2c0

else layoutSettings.Tag = node.Value;

with a more extended validation of whether the visualizer type stored in the .layout file can actually be applied to the corresponding node:

else
{
    if (!string.IsNullOrEmpty(layoutSettings.VisualizerTypeName))
    {
        var inspectBuilder = (InspectBuilder)node.Value;
        var visualizerElement = ExpressionBuilder.GetVisualizerElement(inspectBuilder);
        var visualizerTypes = typeVisualizerMap.GetTypeVisualizers(visualizerElement);
        if (!visualizerTypes.Any(type => type.FullName == layoutSettings.VisualizerTypeName))
            throw new InvalidOperationException("Attempt to map incompatible visualizer type. Visualizer layout may be corrupt.");
    }

    layoutSettings.Tag = node.Value;
}

To help further investigation, ExpressionBuilder.GetVisualizerElement(inspectBuilder) takes care of visualizer "propagation rules" and makes sure we hold the reference to the actual element that will be visualized when double-clicking the node.

typeVisualizerMap.GetTypeVisualizers(visualizerElement) can then be used to get the set of compatible type visualizers for that element. Unless I am missing some edge case, anything that comes from the .layout file should be in this list, if the assignment is to be compatible.

Again, this still does not resolve the situation where the .layout file was externally modified, and the incorrectly assigned visualizer just happens to accidentally be compatible with the new node, but at least this would allow us to detect any inconsistencies as soon as we open the workflow, and present an option to reset the layout.

There might also be another issue in that typeVisualizerMap I believe is filled asynchronously on editor launch, so it might not be fully populated by the time the workflow is open and this method gets called.

[glopesdev]

Reproducing this specific crash has probably changed since #1870 as the structure of visualizer layout assignments has significantly changed. It should now happen somewhere around:

bonsai/Bonsai.Editor/Layout/VisualizerLayoutMap.cs

Line 157 in d8e3bad

dialogSettings.VisualizerTypeName = layoutSettings.VisualizerTypeName;

This should be where the validation mentioned above would now be applied.

Parts of the discussion above are also related to #2137.