Skip to content

[FIX] Assign recalculated node state to backing field in _create_graph (#845) - #868

Open
Leonard013 wants to merge 1 commit into
nipype:mainfrom
Leonard013:fix/845-nodeexecution-state-setter
Open

[FIX] Assign recalculated node state to backing field in _create_graph (#845)#868
Leonard013 wants to merge 1 commit into
nipype:mainfrom
Leonard013:fix/845-nodeexecution-state-setter

Conversation

@Leonard013

@Leonard013 Leonard013 commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Fixes #845

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Why

Running a workflow in which a node without its own splitter consumes the
combined outputs of two separate upstream .split(...).combine(...) nodes
crashes while the execution graph is being built:

File ".../pydra/engine/workflow.py", line 391, in _create_graph
    node.state = state.State(
AttributeError: property 'state' of 'NodeExecution' object has no setter

This is exactly the shape of the nested-workflow example in #845 (the inner
workflow's step3 consumes step1.out and step2.out, both combined).

Root cause / which object owns the state

Workflow._create_graph(nodes) is polymorphic — it is called with definition
Nodes via Workflow.graph() (visualisation) and with NodeExecution wrappers
via Workflow.execution_graph() (to run). Both classes expose state as a
read-only property:

  • Node.state returns its self._state backing field (the true owner of the state);
  • NodeExecution.state merely delegates to self.node.state.

Neither has a setter, so the connection-state recalculation at the bottom of
_create_graph

if node.state:
    node.state.update_connections(...)
else:
    node.state = state.State(...)   # <-- crashes

fails as soon as the else branch is reached with a NodeExecution — i.e. a
stateless node fed by (multiple) combined upstreams.

What this changes

One line: assign to the _state backing field instead of the read-only state
property.

-                    node.state = state.State(
+                    node._state = state.State(

Why this is the correct target and not a new setter:

  • The state is owned by Node._state. During execution the underlying
    Node's state has already been finalised in Workflow.construct() (via
    Node._set_state()); the execution-graph pass must not overwrite it.
  • NodeExecution is a transient execution wrapper with no _state field and
    a read-only-by-design .state (it mirrors its node). Writing node._state on
    it is therefore inert — exactly what we want during execution — while for a real
    definition Node (the graph() path) it sets the state as the original code
    intended. (Note: Node.state is also read-only, so the old line would have
    crashed the visualisation path too; this fixes that latent crash as well.)

I explicitly rejected the "obvious" alternative — adding a @state.setter to
NodeExecution delegating to self.node.state. It removes the crash but
silently produces wrong results: it forces the freshly re-derived State onto
the underlying node, which then mis-splits over its already-combined upstreams (the
#845 example returns an empty list instead of [2, 4, 6]). The _state fix keeps
the node's finalised state intact and yields correct output.

Note on the call site: reaching into node._state here is consistent with the
method's existing idiom (_create_graph already accesses node._task), but it is
admittedly a minimal fix. A cleaner long-term direction would let NodeExecution
own its recalculated state (or guard the assignment to definition Nodes) so the
call site no longer touches a private field — happy to take that route if preferred.

Tests

Added test_wf_ndst_combine_two_upstreams in
pydra/compose/tests/test_workflow_run.py, reproducing the reported nested
split/combine scenario (a stateless node consuming two combined upstreams).

  • Before: FAILED — AttributeError: property 'state' of 'NodeExecution' object has no setter at workflow.py:391.
  • After: PASSED on both debug and cf workers; outputs.out == [2, 4, 6].

The original #845 script (nested Outer/Inner) now runs to completion and
returns OuterOutputs(out=[2, 4, 6]).

Regression sweep (no new failures)

  • test_state.py + test_task_state.py + test_graph.py: 303 passed, 1 xfailed (pre-existing)
  • test_workflow_run.py + test_workflow_fields.py + test_lazy.py: 287 passed, 2 skipped, 4 xfailed (pre-existing)
  • ndst split/combine slice + new test: 84 passed
  • black --check clean on both changed files

Checklist

  • I have added tests to cover my changes
  • I have updated documentation (not necessary)

@Leonard013 @claude
[FIX] Assign recalculated node state to backing field in _create_graph (
04673bb 
nipype#845)

`Workflow._create_graph` runs both over definition `Node`s (via `graph()`)
and over `NodeExecution` wrappers (via `execution_graph()`). Both expose
`state` as a read-only property, so recalculating a node's connection state
with `node.state = state.State(...)` raised

    AttributeError: property 'state' of 'NodeExecution' object has no setter

whenever a stateless node consumed the combined outputs of two upstream
split/combine nodes -- e.g. nested workflows using `.split(...).combine(...)`.

Assign to the `_state` backing field instead. For definition `Node`s this
sets the state exactly as before. For the transient `NodeExecution` wrapper
the write is inert, which is the correct behaviour: the underlying node's
state is already finalised during construction, and overwriting it in the
execution-graph pass mis-splits the node and produces empty results.

Adds a regression test that reproduces the reported nested split/combine
scenario (fails with AttributeError before, passes after).

Fixes nipype#845

Co-authored-by: Claude <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.13%. Comparing base (02f9cba) to head (04673bb).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #868      +/-   ##
==========================================
- Coverage   88.37%   87.13%   -1.24%     
==========================================
  Files          88       88              
  Lines       18221    18240      +19     
  Branches     3572     3576       +4     
==========================================
- Hits        16102    15893     -209     
- Misses       1736     1959     +223     
- Partials      383      388       +5

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

effigies
effigies approved these changes Jul 13, 2026
View reviewed changes

@effigies effigies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks right to me. Thanks!

@effigies

effigies commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@tclose I'm guessing these are related to an upstream change in fileformats?

  FAILED pydra/compose/shell/tests/test_shell_fields.py::test_interface_template_more_complex - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for FileMock types
  FAILED pydra/compose/shell/tests/test_shell_fields.py::test_interface_template_with_defaults - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for FileMock types
  FAILED pydra/engine/tests/test_result.py::test_copyfile_workflow_conflicting_filenames - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for TextFileMock types
  FAILED pydra/utils/tests/test_hash.py::test_hash_file - AssertionError: assert 'a372c0b478b3f750b57a4e6a1c4042ce' == 'f32ab20c4a86616e32bf2504e1ac5a22'
    
    - f32ab20c4a86616e32bf2504e1ac5a22
    + a372c0b478b3f750b57a4e6a1c4042ce
  FAILED pydra/utils/tests/test_typing.py::test_typing_implicit_cast_from_super[func-func] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types
  FAILED pydra/utils/tests/test_typing.py::test_typing_implicit_cast_from_super[func-shell] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types
  FAILED pydra/utils/tests/test_typing.py::test_typing_implicit_cast_from_super[shell-func] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types
  FAILED pydra/utils/tests/test_typing.py::test_typing_implicit_cast_from_super[shell-shell] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types
  FAILED pydra/utils/tests/test_typing.py::test_typing_cast[func-func] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types
  FAILED pydra/utils/tests/test_typing.py::test_typing_cast[func-shell] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types
  FAILED pydra/utils/tests/test_typing.py::test_typing_cast[shell-func] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types
  FAILED pydra/utils/tests/test_typing.py::test_typing_cast[shell-shell] - fileformats.core.exceptions.FileFormatsExtrasNotImplementedError: No implementation for 'generate_sample_data' extra for MyFormatXMock types

@tclose

tclose commented Jul 14, 2026 via email

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@effigies effigies effigies approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

property 'state' of 'NodeExecution' object has no setter

3 participants

@Leonard013 @effigies @tclose