Add a standalone configuration validator (#312) - #388
facontidavide wants to merge 3 commits into
Conversation
Add generate_parameter_library_validate, a CLI that checks a ROS 2 parameter configuration file against one or more parameter definition files. It needs no ROS 2 installation, no colcon workspace and no running node, so it can run as a CI step or on the laptop of somebody configuring a robot. The tool reuses the existing parser and the validators in python_validators.py, so the rules it applies are the ones the generated code applies at runtime. It reports a value whose type does not match the declared type, a value rejected by a built-in validator, and a parameter declared without a default_value that the configuration does not set. --strict also reports parameters that no definition declares, with a suggestion when the name is close to a declared one, and parameters absent from the configuration that will take their default. Custom validator functions are C++ and cannot run outside a build, so they are reported as skipped. read_only describes runtime behaviour and is out of scope. CodeGenVariableBase now keeps the size of a fixed size type on the instance. It was computed and discarded, and the validator needs it to check array lengths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- run_validations returns a severity with each message instead of the caller deciding it by looking for a phrase in the text. A validator that rejects a value is an error; a validator that cannot run here, because it is C++ or because it raised, is a warning, since the value may be correct and only the check is missing. The exception branch was previously reported as an error. - Parse only the definitions whose namespace a configuration refers to, and report the ones left out so that skipping stays visible. With 50 definitions and a configuration matching one of them this goes from 538 ms to 122 ms. - Bound the cost of the "did you mean" suggestions, which compare a name against every declared parameter. A configuration that has drifted wholesale is not helped by a list of guesses. - CodeGenFixedVariable sets fixed_size itself rather than the base class decoding the subclass's tuple layout. - One helper for the dotted name join that was repeated at five call sites, Diagnostic as a dataclass, and drop two attributes nothing read. - Tests pin the severity rather than the wording, cover a validator that raises, and cover a definition that no section refers to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each of these was a silent wrong answer on a configuration shape the project's own fixtures and readme use, verified against ROS 2 Jazzy where the question was about runtime behaviour. - A file whose only section is `/**` was validated against nothing and exited 0. It is now checked like any other section, which is the usual shape when the node name is not pinned. - Parameters of type `none` were reported as missing or wrongly typed. The generator declares nothing for them, so they are skipped, and the subtree below one is not reported as unknown. - A mapped parameter whose key list is not set in the configuration was skipped, and under --strict its keys were reported as unknown. The node expands the map over the effective value, so the declared default is used when the configuration sets nothing. - A section named with a leading slash or under a namespace matched no definition. Section names are reduced to the node name before matching. - --strict reported `use_sim_time` and the other parameters a node declares for itself as unknown. - Scalars are now read the way rcl_yaml_param_parser reads them: `1e5` is a double, which PyYAML calls a string, and the value is converted before the validators run so they are not handed text. The quoting style is not preserved by the loader, so a word ROS 2 reads as a bool is a warning. - An empty sequence is reported, because ROS 2 gives the node PARAMETER_NOT_SET rather than an empty array. - Two definitions with the same root element silently discarded the first. - A single definition was applied to sections that clearly belong to other nodes; it is now only used when no section matches any definition. - A missing or malformed file printed a traceback. All diagnostics go to one stream so that their order is kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@facontidavide I don't mind at all. I'll have my bob review this and will try it out myself. I don't have permissions to merge things here anymore but I'm happy to review stuff. |
tylerjw
left a comment
tylerjw
left a comment
There was a problem hiding this comment.
Hi @facontidavide — I'm Bob, Tyler's AI assistant, posting this review with Tyler's explicit permission.
I reproduced three cases where the validator exits successfully despite a configuration that fails at runtime; the inline comments include details. The validator and mapped-parameter tests pass (38 tests). I couldn't collect the full Python suite because ament_index_python is unavailable in this environment.
| if reads_as_number(value, expected): | ||
| return None |
There was a problem hiding this comment.
[P1] Preserve quoting when resolving numeric types
A configuration containing rate: "2.0" passes this check for a parameter declared as double: reads_as_number() accepts the string and coerce_value() converts it to a float. I reproduced the CLI returning 0. ROS 2 preserves quoted scalars as strings, so the node rejects that override instead. This also affects quoted integers and array elements. Please retain scalar style/tag information while loading YAML and resolve the ROS type independently of the expected declaration, so unquoted scientific notation remains supported without accepting quoted numbers.
Reference: https://github.com/ros2/rcl/blob/rolling/rcl_yaml_param_parser/src/parse.c#L227-L248
| f'{parameter.default_value}', | ||
| ) | ||
| ) | ||
| continue |
There was a problem hiding this comment.
[P2] Validate the effective default when a parameter is omitted
This continue skips all validation for omitted parameters that have a default. For example, a string_array declared with default_value: [] and validation: {not_empty<>: null} passes the CLI with my_node: {ros__parameters: {}} (exit 0), although the generated runtime initialization validates the effective value and rejects the empty array. A numeric default outside its declared bounds has the same problem. Please run the applicable validators on the effective default before continuing; the strict-mode warning alone does not detect the startup failure.
| for nested_key, nested_value in value.items(): | ||
| if isinstance(nested_value, dict) and ROS_PARAMETERS_KEY in nested_value: | ||
| sections[f'{key}/{nested_key}'] = nested_value[ROS_PARAMETERS_KEY] or {} |
There was a problem hiding this comment.
[P2] Discover node sections recursively through namespaces
Only the first two mapping levels are searched for ros__parameters. A valid ROS parameter file nested as robot -> arm -> my_node -> ros__parameters is therefore treated as an unnamed plain parameter tree. I reproduced rate: -5.0 under that section returning exit 0 with a definition that has default_value: 1.0 and gt<>: [0.0]; the actual override is never checked. In strict mode it instead produces unknown-parameter errors for the flattened namespace. Please recurse through namespace mappings until reaching each ros__parameters section.
ROS's parser accumulates namespace components until it reaches that key: https://github.com/ros2/rcl/blob/rolling/rcl_yaml_param_parser/src/parse.c#L902-L938
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
Thanks for taking this one!
Should we directly use this in this repo for integration testing the existing configuration files?
@Nitschi has originally proposed this, could you have a look please and leave a review here?
3 participants
@tylerjw hello !
I hope you don't mind too much the AI generated PR.
I found myself needing this and I would like to contribute upstream, so that more people may benefit from it.
let me know what you think
Description
Add generate_parameter_library_validate, a CLI that checks a ROS 2 parameter configuration file against one or more parameter definition files. It needs no ROS 2 installation, no colcon workspace and no running node, so it can run as a CI step or on the laptop of somebody configuring a robot.
The tool reuses the existing parser and the validators in python_validators.py, so the rules it applies are the ones the generated code applies at runtime. It reports a value whose type does not match the declared type, a value rejected by a built-in validator, and a parameter declared without a default_value that the configuration does not set. --strict also reports parameters that no definition declares, with a suggestion when the name is close to a declared one, and parameters absent from the configuration that will take their default.
Custom validator functions are C++ and cannot run outside a build, so they are reported as skipped. read_only describes runtime behaviour and is out of scope.
CodeGenVariableBase now keeps the size of a fixed size type on the instance. It was computed and discarded, and the validator needs it to check array lengths.