Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discard attribute changed callbacks during graph loading #2598

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

yann-lty
Copy link
Member

@yann-lty yann-lty commented Nov 14, 2024

Description

Ensure attribute changed callbacks are not triggered on graph loading.

When loading a graph, we expect nodes to be deserialized in the exact state they were saved.
Attribute callbacks may modify attribute values and impact the UID of the nodes.
This was leading to UID compatibility conflicts on file opening, for Graph containing Nodes using such mechanisms.

Features list

  • Add "strictCompatibility" mode for graph loading
  • Discard attribute callbacks when graph is being loaded

Implementation remarks

While this behavior was covered by a test, it turned out to be a false positive.
The error was hidden behind the fact that the tested Node was actually UID conflicting due to the callback, and swapped to a Compatibility Node. Therefore, attribute values were restored to the serialized ones, with expected values.
The introduction of the "strictCompatibility" mode allows to test this behavior more efficiently.

@yann-lty yann-lty force-pushed the fix/attributeCallbackOnGraphLoad branch 2 times, most recently from 9acd7f8 to db5dc3b Compare November 14, 2024 18:08
Provide a way to control how to handle node compatibility issues
when loading a Graph with the `strictCompatibility` parameter.
Introduce a new error type, GraphCompatibilityError, to be raised
when loading a Graph with compatibility issues with strictCompatibility enabled.
Update tests that should fail if nodes are loaded as
CompatibilityNodes (resulting in false positives).
@yann-lty yann-lty force-pushed the fix/attributeCallbackOnGraphLoad branch from db5dc3b to 61d3590 Compare November 14, 2024 18:11
@cbentejac cbentejac added this to the Meshroom 2024.1.0 milestone Nov 15, 2024
Copy link
Contributor

@waaake waaake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a test around this, the current changes did solve part of the problem which was loading a graph with no onAttrChanged callbacks triggered and no unwanted CompatibilityIssues raised.

My tests involved having a ChoiceParam Combo Box getting values populated in an onAttrChanged function,
and when the graph is saved and loaded back, the last value on the ChoiceParam is retained, but is shown in Red or invalid in the GUI, as the value is no longer part of the default values on the ChoiceParam which are loaded when the node gets deserialised.

Heart of the issue comes from the fact, we're not serialising the current set of values on the param in the graph file to be able to load them back upon loading.

Guessing this issue came into being with the introduction of allowing dynamic values on the ChoiceParam, as now the choiceParam can get updated values during any method calls.

Thinking about the best way would be to serialise values from such params.

meshroom/core/graph.py Outdated Show resolved Hide resolved
@waaake
Copy link
Contributor

waaake commented Nov 15, 2024

To be able to reproduce what I did, please use the nodes from https://gist.github.com/waaake/d96a5ae6bd8f00c4a7ee0f99cd6a9242

Steps:

  1. Create FirstNode.
  2. Create Second Node.
  3. Connect FirstNode.x -> SecondNode.y.
  4. SecondNode.choice should now have the values as ["A", "B", "C", "D"] with the current value set as "D".
  5. Change the value of this parameter to "B".
  6. Save the graph.
  7. Load the graph back.
  8. Now the value on the choice parameter is set as "B" but marked invalid with a Red colour.
  9. If we click on the choice parameter, the values are no longer present causing the current value on the param to be deemed as invalid.

@yann-lty
Copy link
Member Author

@waaake thanks for the feedback.
This PR is indeed only addressing the compatibility issues happening when attribute callbacks are triggered while loading the graph, which currently prevents the correct re-opening of scenes for computation.
The dynamic values in ChoiceParam not being serialized is surely a problem too, but its resolution can be decoupled from this one in another PR.

@waaake
Copy link
Contributor

waaake commented Nov 18, 2024

@waaake thanks for the feedback. This PR is indeed only addressing the compatibility issues happening when attribute callbacks are triggered while loading the graph, which currently prevents the correct re-opening of scenes for computation. The dynamic values in ChoiceParam not being serialized is surely a problem too, but its resolution can be decoupled from this one in another PR.

Thanks @yann-lty for clarifying the intent of the current fix. I think it's working really well in terms of the fix we're trying to provide urgently and we should be good to get this merged and later discuss around the topic of serialising the contents of the Node. 🙂

@cbentejac cbentejac merged commit 6692915 into develop Nov 18, 2024
3 checks passed
@cbentejac cbentejac deleted the fix/attributeCallbackOnGraphLoad branch November 18, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants