feat: add fine-grained policy merge #466
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
jhonasiv
approved these changes
Feb 26, 2025
doudou
reviewed
Feb 27, 2025
Member
doudou
left a comment
doudou
left a comment
There was a problem hiding this comment.
This is a good first step, but there's one issue.
When using needs_reliable_connection, we have to add period information to the syskit models. Until now, setting the policy explicitly would be enough to avoid having to do this (because of time, or because Syskit limitations would make it hard to do). This PR will break these cases.
My suggestion is to split policy_for into:
- policy_compute_data_element(policy, fallback_policy)
- policy_default_init_flag
Always call the latter, but call the former only if the explicit policy does not have :type set already. Create corresponding unit tests.
eduardacoppo
force-pushed
the
policy_merge_in_dataflow_dynamics
branch
2 times, most recently
from
fb15b6f to
fdce77a
Compare
eduardacoppo
force-pushed
the
policy_merge_in_dataflow_dynamics
branch
from
fdce77a to
6e4d11e
Compare
doudou
reviewed
Mar 13, 2025
| source_task, source_port_name, sink_port_name, sink_task, fallback_policy | ||
| ) | ||
| policy = {} | ||
| unless policy[:type] |
Member
There was a problem hiding this comment.
policy is the empty hash here. This will always be nil.
The if should obviously be checking the policy that is coming from the policy graph.
I would actually pass policy from the loop in compute_policies_from to the method, and add the merge_policy call at the end of this method instead of doing it in said loop.
eduardacoppo
commented
Mar 14, 2025
jhonasiv
reviewed
Mar 17, 2025
doudou
force-pushed
the
policy_merge_in_dataflow_dynamics
branch
from
df2805a to
b1b6741
Compare
Member
|
@jhonasiv updated, please have a look |
jhonasiv
approved these changes
Mar 20, 2025
doudou
force-pushed
the
policy_merge_in_dataflow_dynamics
branch
from
b1b6741 to
c4c5a4b
Compare
doudou
force-pushed
the
transition-to-runkit
branch
from
75f8fa1 to
42d7f4c
Compare
doudou
force-pushed
the
policy_merge_in_dataflow_dynamics
branch
from
888c078 to
2ee8480
Compare
doudou
reviewed
Apr 4, 2025
Member
doudou
left a comment
doudou
left a comment
There was a problem hiding this comment.
Adding empty review to get it out of my queue (since I'm the one working on it now)
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
renamed variables and simplified code
- Updated test expectations - Updated mock expectations - Added test to make sure the size is not overridden when an explicit policy specifies `type: :buffer` - Fixed the merging logic in the reliable connection policy test, properly verifying that `merge_policy` is called instead of directly modifying the expected policy About this last one. I'm not sure if that's the best way to do it. Tried to think of a way of simplifying it, but the test expects this chain of mocks.
Removed the one-line, one-use policy_default_init_flag method, and changed the argument order of merge_policy to match the hash merge (i.e. computed, explicit instead of the opposite)
doudou
force-pushed
the
policy_merge_in_dataflow_dynamics
branch
from
2ee8480 to
cbd87b7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously here: tidewise#3
This was on top of another
tools/syskitPR