fix: preserve control ports for node group proxies - #5488
mikemikimike wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
[fix] Manual test
- Create 3x TextInput, chained together through Flow In->Flow Out
- Create SubflowNodeGroup
- Drag SubflowNodeGroup over the middle TextInput
Extra Flow In and Flow Out parameters are created.
There was a problem hiding this comment.
Similar with ForEachGroup, though given there are multiple control parameters in this case, and I don't know much about how ForEachGroup is meant to work, I'm not actually sure what the correct control parameter is to pass through 😕 . I guess safest just to wire up Flow In/Out and users can re-target if they want.
There was a problem hiding this comment.
Similarly with RetryGroup.
There was a problem hiding this comment.
[consider] It might be good to have an e2e test for this. I note that test_isolated_subflow_runs_data_only_graph is tangentially related, so could be augmented if suitable. Would be good to test after serialise/deserialise too.
| # replayed (for example, while rebuilding a node-group boundary). A plain Parameter has | ||
| # the right type information but loses the control-port shape the editor and executor use. | ||
| if has_control_type: | ||
| input_shaped = (request.input_types is not None and request.output_type is None) or ( |
There was a problem hiding this comment.
[consider] This works in-memory, but not for deserialisation.
That is SubflowNodeGroup._create_proxy_parameter_for_connection emits a AddParameterToNodeRequest with either input_types or output_type being None. So that's good.
But Parameter.to_dict() serialises input_types and output_type as non-None in all cases. So when loading a saved workflow we go down the else branch below to make a bare ControlParameter.
This doesn't actually seem to cause a problem (though it's hard to tell due to other bug 🙄 #5563).
I suspect with #5563 fixed, we'd see control flow parameters rendering incorrectly, though.
| # ControlParameter's convenience subclasses intentionally expose a small, purpose- | ||
| # built constructor. Apply the request-only lifecycle fields after construction so | ||
| # dynamic control parameters retain the same persistence semantics as data parameters. | ||
| new_param.allowed_modes = allowed_modes |
There was a problem hiding this comment.
[consider][minor] allowed_modes is always {INPUT, OUTPUT}, even though technically the parameters should only be one or the other. But this doesn't cause a problem, and the UI prevents users wiring things incorrectly.
So I'd just suggest a little comment here explaining as much.
There was a problem hiding this comment.
Though also relates to #5488 (comment) - worth figuring out that first.
| if input_shaped: | ||
| new_param = ControlParameterInput( | ||
| name=final_param_name, | ||
| tooltip=request.tooltip, |
There was a problem hiding this comment.
[consider][minor] If request.tooltip=None then this splats the default tooltip (same for ControlParameterOutput)
|
|
||
| if input_shaped: | ||
| new_param = ControlParameterInput( | ||
| name=final_param_name, |
There was a problem hiding this comment.
[consider] Although the display name is fine, i.e. "Flow Out"/"Flow In" as expected, the actual parameter name is always exec_out_# (where # is an incrementing number), for both "Flow Out" and "Flow In" parameters.
You can see it e.g. in the sidebar Properties panel
| ) | ||
|
|
||
| if input_shaped: | ||
| new_param = ControlParameterInput( |
There was a problem hiding this comment.
[fix] The inner node is also not connected to the walls of the group, whereas pre-PR it was.
I confirmed with sidebar agent (i.e. via MCP) that there are indeed internal connections to the Flow In/Out parameters, so they exist, but are just not rendered in the editor.
I'm actually not sure if we should have internal connections to the Flow In/Out. The subflow is like an independent mini-workflow, so the start and end of the flow are determined in the same way as a workflow (e.g. Start/End Flow nodes)... Something that needs a more knowledgeable person or some deep dive research to figure out.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Summary
Preserve control-flow port semantics when a node-group boundary proxy is rebuilt through an add-parameter request.
Changes
ControlParameterInputorControlParameterOutputinstead of a plainParameter.Tests
uv run --group test python -m pytest tests/unit -q(492 passed, 1 xfailed)uv run --group dev ruff checkon changed filesuv run --group dev ruff format --checkon changed filesuv run --group dev pyrighton changed filesuv run --group dev typoson changed filesgit diff --checkIssue
Closes #5487