From 5df6b650c4929249ceffabff07c6b52789af0834 Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Tue, 18 Feb 2025 09:31:13 -0300 Subject: [PATCH 1/2] feat: add fine-grained policy merge Previously, if a parameter was added to the designer-provided policy, no automated policy determination would be performed. That meant that the init flag would not be computed in this case. To fix this, a merge_policy function was implemented to merge the explicit policy (the designer-provided one) and the computed policy (init, for instance) The fix follows these rules: - if a value is in policy, use it; - otherwise use the value from computed_policy Also, if the type policy is set to data, the size policy must be removed, as its only meaningful for the type buffer and it causes the connection to fail. The Ruby method `merge` merges two hashes. According to the documentation: "Returns a new hash containing the contents of other_hash and the contents of hsh. If no block is specified, the value for entries with duplicate keys will be that of other_hash." In this case, to follow the rules of prioritizing the value from explicit_policy in case of key duplication, 'other_hash' is explicit_policy and 'hsh' is computed_policy --- .../network_generation/dataflow_dynamics.rb | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/syskit/network_generation/dataflow_dynamics.rb b/lib/syskit/network_generation/dataflow_dynamics.rb index 6b69b8bc3..03fd89f91 100644 --- a/lib/syskit/network_generation/dataflow_dynamics.rb +++ b/lib/syskit/network_generation/dataflow_dynamics.rb @@ -553,6 +553,16 @@ def compute_connection_policies policy_graph end + def merge_policy(explicit_policy, computed_policy) + merged_policy = computed_policy.merge(explicit_policy) + + if merged_policy[:type] == :data + merged_policy.delete(:size) + end + + merged_policy + end + # @api private # # Compute the policies for all connections starting from a given task @@ -563,13 +573,10 @@ def compute_policies_from(connection_graph, source_task, policy_graph = {}) mappings.each_with_object({}) do |(port_pair, policy), h| policy = policy.dup fallback_policy = policy.delete(:fallback_policy) - if policy.empty? - h[port_pair] = - policy_for(source_task, *port_pair, sink_task, - fallback_policy) - else - h[port_pair] = policy - end + computed_policy = policy_for( + source_task, *port_pair, sink_task, fallback_policy + ) + h[port_pair] = merge_policy(policy, computed_policy) end policy_graph[[source_task, sink_task]] = computed_policies end From 7489a9042d22aeabca55aabcad7ac0353021f32e Mon Sep 17 00:00:00 2001 From: eduardacoppo Date: Tue, 18 Feb 2025 13:29:17 -0300 Subject: [PATCH 2/2] test: add tests and update existing --- .../test_dataflow_dynamics.rb | 67 ++++++++++++++++++- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/test/network_generation/test_dataflow_dynamics.rb b/test/network_generation/test_dataflow_dynamics.rb index b638b9856..db65349d6 100644 --- a/test/network_generation/test_dataflow_dynamics.rb +++ b/test/network_generation/test_dataflow_dynamics.rb @@ -298,7 +298,7 @@ module NetworkGeneration policy_graph[[cmp.c_child, task]][%w[out in]]) end - it "uses in-graph policies over the computed ones" do + it "merges in-graph policies with the computed ones" do plan.add(task0 = @task_m.new) plan.add(task1 = @task_m.new) @@ -307,10 +307,13 @@ module NetworkGeneration task0.out_port.connect_to(task1.in_port, type: :buffer, size: 42) - @dynamics.should_receive(:policy_for).never + @dynamics + .should_receive(:policy_for) + .with(task0, "out", "in", task1, nil) + .and_return(type: :buffer, size: 10, init: nil) policy_graph = @dynamics.compute_connection_policies - assert_equal({ type: :buffer, size: 42 }, + assert_equal({ type: :buffer, size: 42, init: nil }, policy_graph[[task0, task1]][%w[out in]]) end @@ -360,6 +363,64 @@ module NetworkGeneration add_agents(tasks[0, 2]) flexmock(@dynamics).should_receive(:propagate).with(tasks[0, 2]) end + + it "handles the case where the explicit policy sets the type to :data" do + plan.add(task0 = @task_m.new) + plan.add(task1 = @task_m.new) + + add_agents(tasks = [task0, task1]) + flexmock(@dynamics).should_receive(:propagate).with(tasks) + + task0.out_port.connect_to task1.in_port, type: :data + + flexmock(@dynamics) + .should_receive(:policy_for) + .with(task0, "out", "in", task1, nil) + .and_return(type: :buffer, size: 10, init: true) + + policy_graph = @dynamics.compute_connection_policies + expected_policy = { type: :data, init: true } + assert_equal(expected_policy, + policy_graph[[task0, task1]][%w[out in]]) + end + end + + describe "merge_policy" do + before do + @dynamics = NetworkGeneration::DataFlowDynamics.new(plan) + end + + it "merges policies by preferring explicit values over " \ + "computed values" do + explicit_policy = { type: :buffer, size: 20, init: true } + computed_policy = { type: :buffer, size: 10, init: true } + + merged_policy = + @dynamics.merge_policy(explicit_policy, computed_policy) + + assert_equal({ type: :buffer, size: 20, init: true }, merged_policy) + end + + it "removes the size value when the type is set to :data" do + explicit_policy = { type: :data, init: true } + computed_policy = { type: :buffer, size: 10, init: true } + + merged_policy = + @dynamics.merge_policy(explicit_policy, computed_policy) + + assert_equal({ type: :data, init: true }, merged_policy) + end + + it "falls back to computed values when explicit values " \ + "are not provided" do + explicit_policy = {} + computed_policy = { type: :buffer, size: 10, init: true } + + merged_policy = + @dynamics.merge_policy(explicit_policy, computed_policy) + + assert_equal({ type: :buffer, size: 10, init: true }, merged_policy) + end end describe "#policy_for" do