Add runtime dependency inference from controller manager#16
Add runtime dependency inference from controller manager#16lakhmanisahil wants to merge 8 commits into
Conversation
Signed-off-by: root <[email protected]>
| f'/{controller_manager_name}/list_hardware_components', | ||
| callback_group=self._node.callback_group_services | ||
| ) | ||
| # The two responses arrive separately, here None means "not received yet". |
There was a problem hiding this comment.
| # The two responses arrive separately, here None means "not received yet". |
| interface_owner = {} | ||
| for hardware in hardware_components: | ||
| for interface in list(hardware.command_interfaces) + list(hardware.state_interfaces): | ||
| interface_owner[interface.name] = hardware.name |
There was a problem hiding this comment.
What's interface.name here? and also hardware.name
There was a problem hiding this comment.
interface.name is the full interface name exported by a hardware component, it is derived from HardwareComponentState.command_interfaces/state_interfaces, e.g. rrbot_joint1/position.
hardware.name is the hardware component that exports it, e.g. RRBotSystemPositionOnly.
And here owning hardware means the hardware component whose exported-interface list contains that interface name. I build a map from each exported interface name to its owning hardware component, then for each controller I resolve its required_command_interfaces/required_state_interfaces to the owning hardware via that map.
| dependency_rules.append(ControllerDependencyRule( | ||
| controller_name=controller.name, | ||
| required_hardware=[ | ||
| HardwareRequirement(name=hardware_name, state=LifecycleState.ACTIVE) |
There was a problem hiding this comment.
How can you assume that the hardware component is active?
There was a problem hiding this comment.
Initially my intent was if a controller needs a hardware interface, the safe assumption is the hardware should be fully ACTIVE before the controller runs, so I recorded active for every required hardware.
But you are correct, I now see that this may be a strict assumption, since a controller can usually be configured while the hardware is still INACTIVE, and only needs ACTIVE when it is activated.
There was a problem hiding this comment.
I also found that dependency inference is only reliable when controller_manager actually reports the controller’s required interfaces.
In my testing for example 1, required_command_interfaces stayed populated, but required_state_interfaces became empty when the hardware was UNCONFIGURED, even though the hardware still reported those interfaces, so may we assume that hardware compoenent should be etiher active or inactive?
There was a problem hiding this comment.
I don't think we should make an assumption, it should reflect the real state of the hardware.
| if not (self._client_list_controllers.service_is_ready() | ||
| and self._client_list_hardware_components.service_is_ready()): | ||
| return | ||
| self._dependencies_inferred = True |
There was a problem hiding this comment.
Isn't it too early to assume that dependencies are already inferred?
There was a problem hiding this comment.
My intent was to avoid re-querying on every activity message: once the controller_manager is visible and we have its controller and hardware data, I wanted to treat the dependencies as known.
But I missed an important case here: the first time the CM becomes visible, the controllers may still not be loaded yet, so if I set the guard before the responses arrive, we would never retry and would miss late-loaded controllers.
I will change that so inference is marked complete only after a successful, non-empty result. That way, if the first query returns no controllers yet, Foreman can try again later when they are available.
Does that match what is intended?
There was a problem hiding this comment.
Shouldn't it query on every activity message?, that's what we wanted right?
As in if a new controller is loaded, you need to redo the dependency tree
| # Cache the hardware; rules are built once the controllers response also arrives. | ||
| try: | ||
| self._hardware_components = future.result().component | ||
| self._build_and_push_dependency_rules() |
There was a problem hiding this comment.
I don't like we call this method assuming we have the controllers list too. Think of a better definitive approach
There was a problem hiding this comment.
Okay, understood. I will rework on this so there is a single flow for inference.
One option is to request list_controllers first, then request list_hardware_components, and only build the rules once both results have been collected in a single path.
That gives us one clear point where inference happens, rather than having each callback assume the other data is already available 👍
There was a problem hiding this comment.
You can do them sequentially and wait for future, not sure how it works in python, but it should be doable
| def update_dependency_rules(self, dependency_rules: List[ControllerDependencyRule]): | ||
| """Hand newly inferred dependency rules to the planner.""" | ||
| with self._state_lock: | ||
| self._planner.replace_dependency_rules(dependency_rules) | ||
|
|
||
| def get_next_transition(self) -> Optional[SystemTransitionCommand]: |
There was a problem hiding this comment.
Not a fan of this approach. Try to come up with a different way, so we don't have to update the dependency rules
There was a problem hiding this comment.
My intent was that since the dependencies are inferred from the live CM, and that information is not available when Foreman starts, I would infer them and then update the planner once the information became available.
I understand your concern about updating the planner's dependency rules at runtime. Since the inference is only done once, I think we could instead infer the dependencies at startup and set the rules once before any planning happens by the planner.
This would be similar to how the parsed scenario is created once at startup and then treated as fixed for the lifetime of the node, which is also what we discussed and concluded during the meeting.
There was a problem hiding this comment.
Another option could be for the planner to query the dependency information when needed instead of storing a mutable set of rules.
What approach do we prefer?
There was a problem hiding this comment.
Yes, it needs to query, because this information keeps changing with respect to activity topic
| def replace_dependency_rules(self, dependency_rules: List[ControllerDependencyRule]): | ||
| """Swap the whole rule set at runtime with freshly inferred rules.""" | ||
| self.rules = {rule.controller_name: rule for rule in dependency_rules} | ||
|
|
Signed-off-by: root <[email protected]>
|
Hello, I have updated the PR as per the review comments. I also changed the flow so the controller and hardware data are read in one chained path, removed the run-once behavior, and updated the tests. PTAL |
| for interface in controller.required_command_interfaces: | ||
| hardware_name = owner_of_interface.get(interface) | ||
| if hardware_name: | ||
| required_hardware_state[hardware_name] = LifecycleState.ACTIVE |
There was a problem hiding this comment.
You cannot assume this, you need to query the hardware lifecycle state either from activity topic or simply query it with the service.
There was a problem hiding this comment.
Okay, I understand now.
You want the hardware state to come from the real controller_manager data instead of me deciding ACTIVE/INACTIVE from the interface type. Initially i understood that differently, I will update this 👍
There was a problem hiding this comment.
I have a doubt about how to represent the hardware state in the dependency rule. Suppose we have a controller X depends on the hardware Y in its lifecycle state Z.
If Z is just the hardware’s current observed state, then the rule becomes circular.
For example, here if the hardware is currently UNCONFIGURED, the rule would say that the controller requires UNCONFIGURED, which does not describe a real dependency.
I think you mean that Z should be the lifecycle state at which the controller’s required interfaces actually become available, based on the data reported by the controller_manager, rather than a state inferred from the interface type or the hardware’s current state.
Could you confirm what Z should represent?
Thid will determine how I map the hardware state into the dependency rule.
There was a problem hiding this comment.
If it really defines a valid state or not, should be handled separately in a different state. At the staruo, when you load the profiles then you validate the profiles, so that way you can be sure about the possibility. But IMO here it should be different
| for interface in controller.required_state_interfaces: | ||
| hardware_name = owner_of_interface.get(interface) | ||
| if hardware_name and hardware_name not in required_hardware_state: | ||
| required_hardware_state[hardware_name] = LifecycleState.INACTIVE |
| return | ||
| def _refresh_dependency_rules(self): | ||
| """Refresh dependency rules based on latest controller and hardware data. Runs on every activity update.""" | ||
| if not (self._client_list_controllers.service_is_ready() |
There was a problem hiding this comment.
You are checking both but only calling the list_controllers. why?. Isn't it possible to wait for the response in the same callback instead of adding new callbacks?
There was a problem hiding this comment.
I check both services because both controller and hardware information are required before building the dependency rules.
The intent was to first get the controller list and then request the hardware list once that response arrives. I used add_done_callback to keep the flow non-blocking, since waiting synchronously for a service response inside a callback can block the ROS 2 executor.
I agree the current callback chain can be made clearer, and I can restructure it if you prefer a different approach.
There was a problem hiding this comment.
I don't think it's a problem to block the executor. Our application is a single threaded and we don't need to run things in parallel right?
Do you see a usecase where the parallelism is needed?
There was a problem hiding this comment.
I don't think it's a problem to block the executor. Our application is a single threaded and we don't need to run things in parallel right?
Okay, got it 👍
Do you see a usecase where the parallelism is needed?
No, I don't see any use case for it as of now.
There was a problem hiding this comment.
No we should not block, as we ant to have reactive application, so I would not wait indefenitelly. Also first getting hw info makes more sense as first the hw is available and then controllers.
There was a problem hiding this comment.
Thanks @destogl for the review, I'll change the order to query the hardware first 👍
There was a problem hiding this comment.
Regarding the blocking call, would you prefer adding a timeout so Foreman remains responsive if the controller_manager doesn't reply, or should I switch to a fully non-blocking call_async approach and update the dependency rules asynchronously?
Since we're using a MultiThreadedExecutor, either approach should fit the current design?
What will you prefer?
There was a problem hiding this comment.
Hello @destogl, do let me know what will be preferred here? i will try to update this acordingly?
There was a problem hiding this comment.
I think that the feedback (callbacks) has to be fully independed from the main loop. But they have to be synchronized. So make sure they are.
| rules = self.infer_dependency_rules(self._controller_states, self._hardware_components) | ||
| self._engine.update_dependency_rules(rules) | ||
| rules = self.infer_dependency_rules(controllers, hardware_components) | ||
| self._engine.set_dependency_rules(rules) |
There was a problem hiding this comment.
We said the engine is gonna query the rule instead of us setting it right?
There was a problem hiding this comment.
I think I misunderstood this initially. When you said the information changes with activity, I interpreted it as the monitor re-querying the controller_manager, which is what I implemented.
I now understand that you mean the engine/planner should query the rules when needed instead of the monitor pushing them via this set_dependency_rules.
There was a problem hiding this comment.
Should the planner hold a reference to a dependency source and read the current rules during planning?
Please let me know.
There was a problem hiding this comment.
I think I misunderstood this initially. When you said the information changes with activity, I interpreted it as the monitor re-querying the controller_manager, which is what I implemented.
I now understand that you mean the engine/planner should query the rules when needed instead of the monitor pushing them via this set_dependency_rules.
We need both of them. But the query needs to happen instead of setting the information and duplicating it.
There was a problem hiding this comment.
Okay, so from now on, the planner will pull the current rules from the Component State Monitor adapter rather than storing its own copy 👍
|
Hello, I have updated the PR as per the review comments. The planner now queries the current dependency rules at runtime instead of storing its own copy. Please have a look. Regarding the hardware dependency state, my understanding is that the dependency rule should represent the required hardware state (e.g. ACTIVE for command interfaces and INACTIVE for state interfaces), rather than the hardware's current state, since the rule describes what state the hardware is required to be in. Please let me know if any further changes are required 👍 |
| """Update planner with new dependency rules.""" | ||
| with self._state_lock: | ||
| self._planner.set_dependency_rules(dependency_rules) | ||
| def set_dependency_provider(self, dependency_provider): |
There was a problem hiding this comment.
Ok, aded the missing type hint using a DependencyProvider protocol. This keeps the engine independent of the concrete adapter and only requires the get_dependency_rules() interface.
| current_dependency_rules = self._planner.get_current_rules() | ||
| for ctrl_goal in goal.controller_goals: | ||
| rule = self._planner.rules.get(ctrl_goal.name) | ||
| rule = current_dependency_rules.get(ctrl_goal.name) |
There was a problem hiding this comment.
What would happen if the key element doesn't exist here?. Add a test for that case and fix it
There was a problem hiding this comment.
Hi,the engine already handles a missing rule with if not rule: continue, so a controller with no dependency rule is treated as having no requirements.
I added a test to cover this case 👍
| lifecycle_nodes=self.foreman_config.lifecycle_nodes | ||
| ) | ||
| # Planner queries dependency rules from the monitor when planning. | ||
| self.foreman_engine.set_dependency_provider(self.component_state_monitor) |
There was a problem hiding this comment.
Nice, this is kinda the approach I'm talking about 👌🏾
| """Update the controller dependency rules.""" | ||
| self.rules = {rule.controller_name: rule for rule in dependency_rules} | ||
| def __init__(self, dependency_rules: List[ControllerDependencyRule] = None, | ||
| dependency_provider=None): |
Got it. I'm fine with it |
A goal can name a controller that has no inferred rule. The check already handles this with `if not rule: continue`; this test pins it so it can't regress into a None crash. Signed-off-by: root <[email protected]>
Add a DependencyProvider Protocol in types.py and annotate the engine and planner provider parameters with it, instead of leaving them untyped. Signed-off-by: root <[email protected]>
|
Hello, I have updated the PR as per the review comments. Please have a look 👍 |
Hardware comes up before controllers bind to it, so query hardware first. Refs: ros-controls#16 Signed-off-by: root <[email protected]>
get_dependency_rules() will infer from the latest stored controller and hardware observations instead of blocking on the controller_manager. Refs: ros-controls#16 Signed-off-by: root <[email protected]>
Replace blocking service calls in the planning path with asynchronous controller_manager queries. On each activity update, query hardware first, then controllers, and store the latest observations. get_dependency_rules() now infers dependency rules from those synchronized observations, keeping the planning loop responsive. Refs: ros-controls#16 Signed-off-by: root <[email protected]>
|
Hi, I have updated the PR according to the feedbavk. PTAL. The dependency refresh is now asynchronous, with hardware is queried before controllers. |


Infer controller-to-hardware dependencies from controller_manager.
This PR removes manually declared controller dependency rules from the scenario YAML and infers them at runtime using the
controller_manager.Instead of writing:
the dependency is inferred automatically from the information reported by
list_controllersandlist_hardware_components.The scenario YAML now only describes the desired goal states.
Design
The
ComponentStateMonitoracts as the dependency provider.Whenever the planner needs dependency rules, it queries the
ComponentStateMonitor, which reads the current controller and hardware information from thecontroller_managerservices and builds the dependency rules on demand.Each controller's required interfaces are matched to the hardware exporting those interfaces using exact interface names.
Currently, dependency inference follows these rules:
The planner always uses the latest dependency rules instead of maintaining its own copy.
YAML Changes
Before:
After:
Current Changes
controller_manager.ComponentStateMonitornow provides dependency rules to the planner.Running the Example
Terminal 1
Terminal 2
Terminal 3
Terminal 4
Use the following YAML for testing:
Current Limitations