Skip to content

[core] node: Prevent using default output value if it is already defined#3126

Draft
cbentejac wants to merge 2 commits into
developfrom
fix/outputValChanges
Draft

[core] node: Prevent using default output value if it is already defined#3126
cbentejac wants to merge 2 commits into
developfrom
fix/outputValChanges

Conversation

@cbentejac

Copy link
Copy Markdown
Contributor

Description

This pull request enhances the handling and testing of output attribute value templates in node descriptions. The main focus is on ensuring that when a node's output value template changes, existing graphs preserve the saved values, while new nodes adopt the updated template. The changes include improvements to attribute value formatting logic and the addition of comprehensive tests to verify compatibility behavior.

Testing and compatibility improvements:

  • Added OutputTemplateNodeV1 and OutputTemplateNodeV2 test node classes in tests/test_compatibility.py to simulate a node whose output value template changes from file.abc to file.cba.
  • Introduced the test_outputValueAfterDefaultOutputChange test, which verifies that saved graphs retain old output values after a template change, while new nodes use the updated template. This ensures backward compatibility and correct application of new defaults.

Core logic enhancement:

  • Updated _buildAttributeExpVars in meshroom/core/node.py to improve output attribute value formatting. The logic now always recomputes the invalidation value from the template, ensuring that the attribute's state reflects the current UID and the latest template, regardless of whether the value was previously set.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates how node attribute expression variables are built in meshroom/core/node.py to handle default value changes, and adds compatibility tests in tests/test_compatibility.py. The review feedback identifies a critical bug where formatting attr.value directly makes it static, preventing subsequent updates when inputs change; a suggestion is provided to cache the initial template string in _templateValue. Additionally, the feedback suggests expanding the compatibility test to verify that modifying an input on a reloaded node correctly updates its output path while preserving the old template filename.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread meshroom/core/node.py
Comment on lines +1275 to 1283
if attr.value and (attr.value != defaultValue and
attr.value != defaultValue.format(**self._expVars)):
attr.value = attr.value.format(**self._expVars)
else:
attr.value = defaultValue.format(**self._expVars)

# Always recompute from the template so _invalidationValue
# reflects the current UID, regardless of attr.value state
attr._invalidationValue = defaultValue.format(**expVarsNoCache)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

There is a critical bug in this implementation. When a node's inputs are modified, its UID and nodeCacheFolder change. However, once attr.value is formatted on the first run, it becomes a static path (e.g., "/path/to/cache_A/file.abc") and no longer contains any placeholders (like {nodeCacheFolder}). On subsequent updates, calling .format() on this static path does nothing, so the output path remains pointing to the old cache folder instead of updating to the new one.

To fix this, we can cache the initial template string (either the loaded custom template or the default template) in a private attribute _templateValue on the attr object on the first run. On subsequent runs, we can always format from this cached template. This preserves the old template for loaded graphs while still allowing the output path to dynamically update when the cache folder changes.

Suggested change
if attr.value and (attr.value != defaultValue and
attr.value != defaultValue.format(**self._expVars)):
attr.value = attr.value.format(**self._expVars)
else:
attr.value = defaultValue.format(**self._expVars)
# Always recompute from the template so _invalidationValue
# reflects the current UID, regardless of attr.value state
attr._invalidationValue = defaultValue.format(**expVarsNoCache)
if not hasattr(attr, '_templateValue'):
if attr.value and '{' in str(attr.value):
attr._templateValue = attr.value
else:
attr._templateValue = defaultValue
attr.value = attr._templateValue.format(**self._expVars)
# Always recompute from the template so _invalidationValue
# reflects the current UID, regardless of attr.value state
attr._invalidationValue = attr._templateValue.format(**expVarsNoCache)

Comment on lines +493 to +498
# The internal folder (driven by inputs, not outputs) must be unchanged
assert reloadedNode.internalFolder == n.internalFolder

# A new instance of this node should reflect the new default value in the description
n = g.addNewNode(OutputTemplateNodeV1.__name__)
assert n.output.value.endswith("/file.cba")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Let's add assertions to verify that modifying an input on the reloaded node correctly updates its output path to the new internalFolder while still preserving the old template filename.

Suggested change
# The internal folder (driven by inputs, not outputs) must be unchanged
assert reloadedNode.internalFolder == n.internalFolder
# A new instance of this node should reflect the new default value in the description
n = g.addNewNode(OutputTemplateNodeV1.__name__)
assert n.output.value.endswith("/file.cba")
# The internal folder (driven by inputs, not outputs) must be unchanged
assert reloadedNode.internalFolder == n.internalFolder
# Modify an input to trigger a UID and internalFolder change
reloadedNode.input.value = "new_input_value"
# The output value should update to the new internalFolder but still preserve the old template filename
assert reloadedNode.output.value.startswith(reloadedNode.internalFolder)
assert reloadedNode.output.value.endswith("/file.abc")
# A new instance of this node should reflect the new default value in the description
n = g.addNewNode(OutputTemplateNodeV1.__name__)
assert n.output.value.endswith("/file.cba")

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.36%. Comparing base (37993f4) to head (b044140).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3126      +/-   ##
===========================================
+ Coverage    85.33%   85.36%   +0.03%     
===========================================
  Files           73       73              
  Lines        11414    11439      +25     
===========================================
+ Hits          9740     9765      +25     
  Misses        1674     1674              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cbentejac cbentejac force-pushed the fix/outputValChanges branch from 2e466fe to b044140 Compare June 8, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant