Fix: Variable input shapes caused by missions.#1039
Conversation
Now the task receiving conflicting inputs will always declare the *union* of all the possible inputs.
fe350ad to
303d5c6
Compare
|
@elpiel can you give it a shot? Thanks |
There was a problem hiding this comment.
Pull request overview
This PR addresses mission-scoped graph differences that previously caused generated task input shapes (and/or ordering) to vary between missions. It introduces a stable ordering mechanism and adds proc-macro support to generate a consistent input binding shape by filling missing mission-specific inputs with empty CuMsgs.
Changes:
- Add a stable
connection_orderto runtime-plan input metadata and sort task inputs by it. - Extend
#[copper_runtime]codegen to compute a canonical per-task input layout across missions and generate placeholder inputs when a mission omits some slots. - Add runtime and trybuild coverage for mission-scoped sink inputs and stable input positioning.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| core/cu29_runtime/src/curuntime.rs | Adds connection_order to CuInputMsg and sorts inputs by serialized connection order. |
| core/cu29_runtime/tests/missions.rs | Runtime test verifying mission-scoped inputs affect sink arity and ordering is stable. |
| core/cu29_derive/src/lib.rs | Computes canonical task input layouts across missions and generates consistent input bindings with placeholders. |
| core/cu29_derive/tests/config/mission_sink_input_union_valid.ron | Test config for a mission adding an extra sink input. |
| core/cu29_derive/tests/config/mission_sink_reused_id_stable_positions_valid.ron | Test config ensuring stable slot positions across missions with different upstream tasks. |
| core/cu29_derive/tests/compile_pass/copper_runtime/mission_sink_input_union.rs | Trybuild pass test validating union-style input shape generation. |
| core/cu29_derive/tests/compile_pass/copper_runtime/mission_sink_reused_id_stable_positions.rs | Trybuild pass test validating stable input positions across missions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) -> GeneratedTaskInput { | ||
| let task_id = step.node.get_id().to_string(); | ||
| let layout = task_input_layouts | ||
| .get(&task_id) | ||
| .unwrap_or_else(|| panic!("Missing canonical input layout for task '{task_id}'")); | ||
|
|
||
| let mut setup = Vec::new(); | ||
| let mut refs = Vec::new(); | ||
|
|
||
| for (slot_index, slot) in layout.iter().enumerate() { | ||
| if let Some(input) = step.input_msg_indices_types.get(slot.position) { |
There was a problem hiding this comment.
generate_task_input_binding indexes step.input_msg_indices_types by slot.position. With the current canonical layout strategy this can only be correct if every mission’s input vector is positionally aligned; when missions insert/remove connections before other inputs, positional indexing won’t find the intended input and will synthesize an empty CuMsg for the wrong slot. If you switch to an alignment-based canonical layout, consider carrying an explicit per-mission mapping (canonical slot -> optional input index) and use that mapping here rather than direct indexing.
| ) -> GeneratedTaskInput { | |
| let task_id = step.node.get_id().to_string(); | |
| let layout = task_input_layouts | |
| .get(&task_id) | |
| .unwrap_or_else(|| panic!("Missing canonical input layout for task '{task_id}'")); | |
| let mut setup = Vec::new(); | |
| let mut refs = Vec::new(); | |
| for (slot_index, slot) in layout.iter().enumerate() { | |
| if let Some(input) = step.input_msg_indices_types.get(slot.position) { | |
| task_input_index_mappings: &HashMap<String, Vec<Option<usize>>>, | |
| ) -> GeneratedTaskInput { | |
| let task_id = step.node.get_id().to_string(); | |
| let layout = task_input_layouts | |
| .get(&task_id) | |
| .unwrap_or_else(|| panic!("Missing canonical input layout for task '{task_id}'")); | |
| let input_index_mapping = task_input_index_mappings | |
| .get(&task_id) | |
| .unwrap_or_else(|| panic!("Missing canonical input index mapping for task '{task_id}'")); | |
| if input_index_mapping.len() != layout.len() { | |
| panic!( | |
| "Canonical input layout/index mapping length mismatch for task '{}': layout has {} slot(s), mapping has {} slot(s)", | |
| task_id, | |
| layout.len(), | |
| input_index_mapping.len() | |
| ); | |
| } | |
| let mut setup = Vec::new(); | |
| let mut refs = Vec::new(); | |
| for (slot_index, slot) in layout.iter().enumerate() { | |
| if let Some(input_index) = input_index_mapping[slot_index] { | |
| let input = step.input_msg_indices_types.get(input_index).unwrap_or_else(|| { | |
| panic!( | |
| "Canonical input index mapping for task '{}' points to missing mission input index {} for canonical slot {}", | |
| task_id, input_index, slot_index | |
| ) | |
| }); |
| pub struct CuInputMsg { | ||
| pub culist_index: u32, | ||
| pub msg_type: String, | ||
| pub src_port: usize, | ||
| pub edge_id: usize, | ||
| pub connection_order: usize, | ||
| } |
There was a problem hiding this comment.
Adding connection_order to the public CuInputMsg struct is a SemVer-breaking change for any downstream crate constructing CuInputMsg via struct literals. If this type is intended to be part of the public API surface, consider making it #[non_exhaustive] and/or providing a constructor/accessors so future layout changes don’t require breaking updates.
| let task_layout = layouts.entry(node.get_id().to_string()).or_default(); | ||
| let mut edge_ids = graph.get_dst_edges(node_id)?; | ||
| edge_ids.sort_by_key(|edge_id| { | ||
| graph | ||
| .edge(*edge_id) | ||
| .map(|edge| edge.order) | ||
| .unwrap_or(usize::MAX) | ||
| }); | ||
| for (position, edge_id) in edge_ids.into_iter().enumerate() { | ||
| let edge = graph.edge(edge_id).ok_or_else(|| { | ||
| CuError::from(format!( | ||
| "Missing edge {edge_id} while collecting inputs for task '{}'", | ||
| node.get_id() | ||
| )) | ||
| })?; | ||
| if let Some(existing_msg_type) = task_layout.get(position) { | ||
| if existing_msg_type != &edge.msg { | ||
| return Err(CuError::from(format!( | ||
| "Task '{}' input position {} has incompatible message types '{}' and '{}' across missions", | ||
| node.get_id(), | ||
| position, | ||
| existing_msg_type, | ||
| edge.msg | ||
| ))); | ||
| } | ||
| } else { | ||
| task_layout.push(edge.msg.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
collect_task_input_layouts builds the canonical layout by sorting edges by edge.order and then using enumerate() as the slot index. This effectively assumes mission differences only add/remove inputs at the end; if a mission-only connection appears earlier in the serialized cnx list than a shared connection, subsequent inputs shift positions and the macro will either error with “incompatible message types” or bind inputs to the wrong slot instead of producing a union with a placeholder. Consider deriving the canonical layout via a stable alignment/supersequence of per-mission input type sequences (or otherwise tracking gaps explicitly) so inserted mission-only inputs don’t shift existing slots across missions.
Now the task receiving conflicting inputs will always declare the union of all the possible inputs.
Summary
Related issues
Changes
Testing
just fmtjust lintjust testjust std-ci(if std/runtime paths are impacted)just nostd-ci(if embedded/no_std paths are impacted)pro-tip:
justwith no parameters in the root defaults tojust fmt,just lint, andjust test.Checklist
Breaking changes (if any)
Additional context