Spawner: Load controller with remote-only .yaml #2853
Conversation
github-actions
bot
requested review from
Maverobot,
bmagyar,
christophfroehlich,
mhubii and
moriarty
saikishor
left a comment
saikishor
left a comment
There was a problem hiding this comment.
If i understood correctly it expects that the file exist on the same PC as the controller manager. I thought this PR is for cases when you want to load files of PC launching the command, but not on the PC the controller_manager is running
No its inverted its for cases where you don't have the I wanted to add the inverse as well if we want this. |
mamueluth
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2853 +/- ##
==========================================
- Coverage 89.57% 89.47% -0.10%
==========================================
Files 152 152
Lines 18087 17732 -355
Branches 1470 1442 -28
==========================================
- Hits 16201 15866 -335
+ Misses 1297 1287 -10
+ Partials 589 579 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
christophfroehlich
changed the title
| parser.add_argument( | ||
| "-n", | ||
| "--namespace", | ||
| help="DEPRECATED Namespace for the controller_manager and the controller(s)", | ||
| default=None, | ||
| required=False, | ||
| ) |
There was a problem hiding this comment.
| parser.add_argument( | |
| "-n", | |
| "--namespace", | |
| help="DEPRECATED Namespace for the controller_manager and the controller(s)", | |
| default=None, | |
| required=False, | |
| ) |
This is no more needed from rolling on
| def parse_type_from_controllers(controller_names: list[str]) -> dict[str, str]: | ||
| controller_to_type : dict[str, str] = dict() | ||
| for name in controller_names: | ||
| # We expect controller:some/type | ||
| # -> split[0]=controller AND split[1]=some/type | ||
| split = name.split(":") | ||
| if len(split) != 2 or not split[0] or not split[1]: | ||
| raise ValueError( | ||
| f"Invalid format '{name}'. Expected format is 'controller_name:some/controller_type' if '--param-file-remote-only' flag is used." | ||
| ) | ||
| controller = split[0] | ||
| controller_type = split[1] | ||
|
|
||
| if controller in controller_to_type: | ||
| raise ValueError( | ||
| f"Controller names must be unique. Got multiple occurrences of {controller}" | ||
| ) | ||
| else: | ||
| controller_to_type[controller] = controller_type | ||
| return controller_to_type | ||
|
|
||
|
|
There was a problem hiding this comment.
For the type, maybe it makes sense to have the --type arg ? Instead of doing it this way?
| parser.add_argument( | ||
| "--param-file-remote-only", | ||
| help="Set this to load the param file only remotely. Param file is not needed to be present locally only remotely.", | ||
| default=False, | ||
| action="store_true", | ||
| required=False, | ||
| ) |
There was a problem hiding this comment.
I've been thinking about the arg, should we use something like --skip--param-file-parsing ? Because what you are doing within the spawner's changes is simply avoiding parsing the files and setting them as the parameters
| if node.get_namespace() != "/" and args.namespace: | ||
| raise RuntimeError( | ||
| f"Setting namespace through both '--namespace {args.namespace}' arg and the ROS 2 standard way " | ||
| f"'--ros-args -r __ns:={node.get_namespace()}' is not allowed!" | ||
| ) | ||
|
|
||
| if args.namespace: | ||
| warnings.filterwarnings("always") | ||
| warnings.warn( | ||
| "The '--namespace' argument is deprecated and will be removed in future releases." | ||
| " Use the ROS 2 standard way of setting the node namespacing using --ros-args -r __ns:=<namespace>", | ||
| DeprecationWarning, | ||
| ) | ||
|
|
||
| spawner_namespace = args.namespace if args.namespace else node.get_namespace() | ||
|
|
||
| if not spawner_namespace.startswith("/"): | ||
| spawner_namespace = f"/{spawner_namespace}" | ||
|
|
||
| if not controller_manager_name.startswith("/"): | ||
| if spawner_namespace and spawner_namespace != "/": | ||
| controller_manager_name = f"{spawner_namespace}/{controller_manager_name}" | ||
| else: | ||
| controller_manager_name = f"/{controller_manager_name}" | ||
|
|
There was a problem hiding this comment.
| if node.get_namespace() != "/" and args.namespace: | |
| raise RuntimeError( | |
| f"Setting namespace through both '--namespace {args.namespace}' arg and the ROS 2 standard way " | |
| f"'--ros-args -r __ns:={node.get_namespace()}' is not allowed!" | |
| ) | |
| if args.namespace: | |
| warnings.filterwarnings("always") | |
| warnings.warn( | |
| "The '--namespace' argument is deprecated and will be removed in future releases." | |
| " Use the ROS 2 standard way of setting the node namespacing using --ros-args -r __ns:=<namespace>", | |
| DeprecationWarning, | |
| ) | |
| spawner_namespace = args.namespace if args.namespace else node.get_namespace() | |
| if not spawner_namespace.startswith("/"): | |
| spawner_namespace = f"/{spawner_namespace}" | |
| if not controller_manager_name.startswith("/"): | |
| if spawner_namespace and spawner_namespace != "/": | |
| controller_manager_name = f"{spawner_namespace}/{controller_manager_name}" | |
| else: | |
| controller_manager_name = f"/{controller_manager_name}" |
3 participants
Replaces: #2349
There are cases in which you want to spawn controllers running on a remote machine (robot) without the
controllers.yamlbeeing available on the local machine. This PR extends the spawner with the option to activate a controller on the remote machine without the need of havingcontrollers.yamlon your local machine.Example:
Open questions
How to pass controller type? In the spawner we need to know at least the
controller nameand thecontroller type. Usually we would parse the type from thecontroller.yamlbut since the file is not present on the local machine we need to pass the type. I choose to do:controller:typeas can be seen in the example. Is this good?How should we go about testing? In tests the file will always be present. I would just define the
.yamlin code, don't have the file present and test like this or are there any other suggestions?