Add new YAML configuration format#8
Conversation
destogl
left a comment
There was a problem hiding this comment.
We have also here one conceptual issue regarding hardware. For controllers, it is sufficient to have active/inactive states. But for the hardware we also need unconfigured as we can physically unplug the hardware, but want to keep SW component loaded. To achieve this we have to terminate the communication which happen in on_clenup method.
| listed names are ACTIVE and every other name in the universe is INACTIVE. | ||
| """ | ||
| #TODO: revisit the name "universe" (e.g. profile_universe, all_components or every_named_component) to avoid ambiguity. | ||
| universe_controllers = set() |
There was a problem hiding this comment.
why not simply call all_controllers? As this wouldn't need any explanation what does prefix means.
There was a problem hiding this comment.
Yes, will rename it to all_controllers. Same for all_hardware and all_lifecycle_nodes
| def _collect_profiles(data): | ||
| """Find the top-level entries that are profiles (i.e. dicts with controllers/hardware/lifecycle_nodes keys whose value is a list).""" | ||
| profiles = {} | ||
| for name, body in data.items(): |
There was a problem hiding this comment.
Why "body", would be more logical "components" or similar? Body is very, very generic.
There was a problem hiding this comment.
Okay, agreed 👍
I will rename it to profile or may be profile_components, since the value is a single profile's definition (a dict of controller/hardware/lifecycle_node lists), not a single component.
| active_lifecycle = set(body.get('lifecycle_nodes', []) or []) | ||
|
|
||
| hw_goals = [] | ||
| for hw_name in sorted(universe_hardware): |
There was a problem hiding this comment.
why sorting? What does sorting means here? Is it alphabetical?
There was a problem hiding this comment.
Yes, it is alphabetical and only for deterministic output (stable order in the goals, tests, and logs).
It has no functional effect on the planner, so I willl drop it.
| return profiles | ||
|
|
||
|
|
||
| def _expand_profiles(profiles): |
There was a problem hiding this comment.
not good function name. As without comment there is no clue what is happening. So you basically here prepare all the calls you are doing later? Correct?
There was a problem hiding this comment.
You are right, i will rename it.
The function takes the profiles and produces one SystemGoal per profile: the same object the existing goal_states form produces; so the engine and planner stay unchanged.
It builds the target states, not the service calls; the planner turns those goals into transitions at runtime. I am thinking to rename it to _create_goals_from_profiles.
| lifecycle_node_goals=lc_goals | ||
| ) | ||
|
|
||
| # Expand profiles into goals, alongside the goal_states above. |
There was a problem hiding this comment.
I would rather use word "convert" or "create" than "expand".
There was a problem hiding this comment.
Okay, agreed 👍
| base: | ||
| controllers: [joint_state_broadcaster] | ||
| hardware: [RRBot] | ||
|
|
||
| running: | ||
| controllers: [joint_state_broadcaster, forward_position_controller] | ||
| hardware: [RRBot] |
There was a problem hiding this comment.
Shouldn't they be defined within a namespace? (same goes for all the following files)
@destogl what do you think?
There was a problem hiding this comment.
Yes, I can wrap the profiles under a top-level profiles namespace.
I originally kept them flat to emphasize that the YAML file contains only profile definitions, as I am considering removing configuration keys such as controller_manager and ransition_pause and moving them to the launch file instead.
This approach also simplifies the parser, since it can read data['profiles'] directly rather than detecting profile-shaped entries. I will update the format accordingly and modify all example files to follow this structure.
There was a problem hiding this comment.
How about this?
profiles:
profile1:
controllers: [] # active controllers for profile1 only
hardware: [] # active hardware for profile1 only
lifecycle_nodes: [] # active lifecycle nodes for profile1 only
profile2:
controllers: []
hardware: []
lifecycle_nodes: []| def _collect_profiles(data): | ||
| """Find the top-level entries that are profiles (i.e. dicts with controllers/hardware/lifecycle_nodes keys whose value is a list).""" | ||
| profiles = {} | ||
| for name, body in data.items(): |
| #TODO: revisit the name "universe" (e.g. profile_universe, all_components or every_named_component) to avoid ambiguity. | ||
| universe_controllers = set() | ||
| universe_hardware = set() | ||
| universe_lifecycle = set() |
There was a problem hiding this comment.
So universe is all controllers listed?
There was a problem hiding this comment.
universe is the complete set of every component any profile mentions, which is grouped into controllers, hardware, and lifecycle nodes.
For a given profile, the names a profile lists are ACTIVE and the rest of that set are INACTIVE, so each profile's goal is a complete target state, not a partial diff.
I will rename universe to all_components and add proper documentation.
also, here universe_controller is all controllers listed.
| for name, body in profiles.items(): | ||
| active_controllers = set(body.get('controllers', []) or []) | ||
| active_hardware = set(body.get('hardware', []) or []) | ||
| active_lifecycle = set(body.get('lifecycle_nodes', []) or []) | ||
|
|
||
| hw_goals = [] | ||
| for hw_name in sorted(universe_hardware): | ||
| hw_state = LifecycleState.ACTIVE if hw_name in active_hardware else LifecycleState.INACTIVE | ||
| hw_goals.append(Component( | ||
| name=hw_name, | ||
| component_type=ComponentType.HARDWARE, | ||
| lifecycle_state=hw_state | ||
| )) |
There was a problem hiding this comment.
What's happening here?. I'm lost
There was a problem hiding this comment.
This block builds the target state for one profile, and the reason it looks different from the old parser code is where the state comes from.
In the old goal_states form, the YAML already writes each component's state explicitly:
goal_states:
running:
hardware:
RRBot: active # state is given (old code)so the parser just reads "active" and builds Component(RRBot, HARDWARE, ACTIVE).
There was a problem hiding this comment.
In this profile form, the state is implicit, we only list what should be active:
running:
hardware: [RRBot] # only the active ones are listedso the parser has to derive each state: for every hardware in the full set, it's ACTIVE if this profile lists it, otherwise INACTIVE. That is exactly whatr this loop does:
for hw_name in sorted(all_hardware):
hw_state = LifecycleState.ACTIVE if hw_name in active_hardware else LifecycleState.INACTIVE
hw_goals.append(Component(name=hw_name, component_type=ComponentType.HARDWARE, lifecycle_state=hw_state))There was a problem hiding this comment.
This new parser, it only constructs the goal (the same Component objects the old form produces).
It does not activate anything or decide order: bringing hardware up before the controllers that depend on it is the "planner's" job at runtime, not this loop's.
I will add a short comment saying this and simplify the construction.
|
Yes, i think this PR implementation need more clarity, the objective was to build on the top of existing work. For context, my main intention was to reuse the existing functionality of parser.py to build goals (the target "profiles" here, such as base or running) from the new YAML format. Today, ''parser.py" converts the YAML into a ParsedScenario object (controller_manager, ransition_pause, hardware, lifecycle_nodes, dependency_rules, goals, and tracked_components). My change simply adds a second way to add goals. With the existing goal_states format, every component and its exact state must be restated for every mode, which becomes lengthy and error prone on systems with many controllers. What the parser already does is read the YAML and convert each goal_states entry into a SystemGoal (lists of hardware, controllers, and lifecycle nodes with their target lifecycle states). I reused exactly the same output representation. Each profile (for example, base or running) is converted into the same SystemGoal object, so neither the engine nor the planner requires any changes. By the time a profile reaches them, it is indistinguishable from a goal defined using the existing The conversion works as follows. First, I gather the complete set of component names mentioned across all profiles. I called this the "universe", although intitally I was not entirely sure about the naming; "universe" felt inclusive(and initialy I avoided using "all" because For each profile, the listed components become ACTIVE; while the remaining components from that set(universe) become INACTIVE. This ensures that every profile expands into a complete target state. The use of sorted() is only to provide stable, alphabetical output for reproducible goals, tests, and logs. It has no functional impact on the planner, and I can remove it if preferred. The two helper methods are:
There is also a small block in parse_yaml_file() that merges the generated goals and ensures that all referenced components are added to the set of tracked components monitored by Foreman. I agree that the naming and documentation need more work and clarification. Apologies for that. I will address these issues and reply to each review comment individually to make this implementation and intent moer clearer. Also, I opened this PR only to explore whether the existing YAML format could be simplified by listing only the ACTIVE components and to gather feedback on this approach. |
Got it; currently, by this implementation there is no way to request the One idea would be to keep We can discuss whether this is the right approach during the meeting 👍 |
I have updated the comment above with a proper explanation.
I modeled these tests on the existing |
Description
Add a new YAML "profiles" config format for goal states in Foreman, as an optional alternative to the verbose
goal_statesform.Note: This is a draft PR to showcase the proposed approach for the new YAML format.
A profile is a named mode that lists only the components which should be active; every other component any profile mentions is set inactive. Profiles are expanded at parse time into the existing
SystemGoal, so the engine and planner are unchanged and the verbosegoal_statesform keeps working.This is the new YAML fomat:
What this PR does:
SystemGoal(listed names ACTIVE, every other name any profile mentions INACTIVE).goal_statesform fully working.parser.py(_collect_profiles,_expand_profiles) and one block inparse_yaml_file; existing parser logic is untouched.Is this a user-facing behavior change?
Yes, but additive and opt-in. A new, optional YAML format is now accepted. Existing configs using
goal_statesparse and behave exactly as before; the new behavior applies only to files that use the profile format.Did you use Generative AI?
Yes. Claude and ChatGPT were used for drafting, and refining the write-up; the implementation and validation were done manually.
Additional Information
Environment: developed and tested in a ROS 2
lyricalDocker container.Setup (workspace + demos):
How to run (Example 1, RRBot):
Examples 1 (RRBot) and 2 (DiffBot) run end-to-end(screenshot attached).
How to test:
Expected: 14 from
test_parser.py(verbose form, unchanged) and 9 fromtest_parser_profiles.py, all passing. (screenshot attached)The new tests live in a separate file because
test_parser.pyis bound to the shippedscenario.yamland asserts facts specific to the old format; a new profile config has none of those, so it cannot reuse those fixtures.Notes on the demos: On Examples 3 and 13 the goal is accepted and the correct first transition is issued, but the
controller_managerrejects a specific deactivation.This is not a config-format issue; the rejection comes from the controller_manager service not from Foreman.
(screenshot attached)
Note
Pre-existing test note:
test_engine.pyhas 8 failures that pre-date this change and reproduce on cleanmaster(by stashing this change). The engine tests do not call the parser.Files added:
foreman/test/test_parser_profiles.pyandforeman/test/test_parser_profiles_config.yaml: new tests and their fixture config.foreman/config/demos/example_{1,2,3,13}.yaml: worked profile examples, one per ros2_control demo.Follow-up PRs (next steps, each scoped to one thing):
controller_managerandtransition_pauseto launch/CLI args.