-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding ability to add parameters through yaml at runtime #2406
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a look at this in our issue triage meeting and the consensus was that "loading" parameters based on a yaml file is useful, however we don't think we should continue to expand the methods in the Node class or the NodeParametersInterface class and we'd prefer to see a free-function or similar that uses a Node and a yaml file to perform the operations.
To this point, your pull request doesn't add these methods to the LifecycleNode
class, but if it was a free-function that took a rclcpp::NodeInterface<...>
template argument (see
rclcpp/rclcpp/include/rclcpp/node_interfaces/node_interfaces.hpp
Lines 68 to 146 in a29f58e
/// Create a new NodeInterfaces object using the given node-like object's interfaces. | |
/** | |
* Specify which interfaces you need by passing them as template parameters. | |
* | |
* This allows you to aggregate interfaces from different sources together to pass as a single | |
* aggregate object to any functions that take node interfaces or node-likes, without needing to | |
* templatize that function. | |
* | |
* You may also use this constructor to create a NodeInterfaces that contains a subset of | |
* another NodeInterfaces' interfaces. | |
* | |
* Finally, this class supports implicit conversion from node-like objects, allowing you to | |
* directly pass a node-like to a function that takes a NodeInterfaces object. | |
* | |
* Usage examples: | |
* ```cpp | |
* // Suppose we have some function: | |
* void fn(NodeInterfaces<NodeBaseInterface, NodeClockInterface> interfaces); | |
* | |
* // Then we can, explicitly: | |
* rclcpp::Node node("some_node"); | |
* auto ni = NodeInterfaces<NodeBaseInterface, NodeClockInterface>(node); | |
* fn(ni); | |
* | |
* // But also: | |
* fn(node); | |
* | |
* // Subsetting a NodeInterfaces object also works! | |
* auto ni_base = NodeInterfaces<NodeBaseInterface>(ni); | |
* | |
* // Or aggregate them (you could aggregate interfaces from disparate node-likes) | |
* auto ni_aggregated = NodeInterfaces<NodeBaseInterface, NodeClockInterface>( | |
* node->get_node_base_interface(), | |
* node->get_node_clock_interface() | |
* ) | |
* | |
* // And then to access the interfaces: | |
* // Get with get<> | |
* auto base = ni.get<NodeBaseInterface>(); | |
* | |
* // Or the appropriate getter | |
* auto clock = ni.get_clock_interface(); | |
* ``` | |
* | |
* You may use any of the standard node interfaces that come with rclcpp: | |
* - rclcpp::node_interfaces::NodeBaseInterface | |
* - rclcpp::node_interfaces::NodeClockInterface | |
* - rclcpp::node_interfaces::NodeGraphInterface | |
* - rclcpp::node_interfaces::NodeLoggingInterface | |
* - rclcpp::node_interfaces::NodeParametersInterface | |
* - rclcpp::node_interfaces::NodeServicesInterface | |
* - rclcpp::node_interfaces::NodeTimeSourceInterface | |
* - rclcpp::node_interfaces::NodeTimersInterface | |
* - rclcpp::node_interfaces::NodeTopicsInterface | |
* - rclcpp::node_interfaces::NodeTypeDescriptionsInterface | |
* - rclcpp::node_interfaces::NodeWaitablesInterface | |
* | |
* Or you use custom interfaces as long as you make a template specialization | |
* of the rclcpp::node_interfaces::detail::NodeInterfacesSupport struct using | |
* the RCLCPP_NODE_INTERFACE_HELPERS_SUPPORT macro. | |
* | |
* Usage example: | |
* ```RCLCPP_NODE_INTERFACE_HELPERS_SUPPORT(rclcpp::node_interfaces::NodeBaseInterface, base)``` | |
* | |
* If you choose not to use the helper macro, then you can specialize the | |
* template yourself, but you must: | |
* | |
* - Provide a template specialization of the get_from_node_like method that gets the interface | |
* from any node-like that stores the interface, using the node-like's getter | |
* - Designate the is_supported type as std::true_type using a using directive | |
* - Provide any number of getter methods to be used to obtain the interface with the | |
* NodeInterface object, noting that the getters of the storage class will apply to all | |
* supported interfaces. | |
* - The getter method names should not clash in name with any other interface getter | |
* specializations if those other interfaces are meant to be aggregated in the same | |
* NodeInterfaces object. | |
* | |
* \param[in] node Node-like object from which to get the node interfaces | |
*/ |
Node
as well as LifecycleNode
or any other kinds of "node-like" objects in the future. This is not only more reusable, but it helps us keep the size of the Node class manageable and therefore helps with maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't think we should continue to expand the methods in the Node class or the NodeParametersInterface class
i understand the concern and benefits for having this as free functions.
on the other hand, i also feel like load_parameters
method can belong to NodeParameterInterface
.
// pseudo code
// list parameters belong to the node
auto parameter_list = node->list_parameters({}, 0);
// load parameters to the node
results = rclcpp::load_parameters(node, yaml_file);
i do not have strong opinion, but it would be nice if we can provide the consistent way to manage parameters?
it would be probably nice to refactor the existed methods to use free function like load_parameters
above? what do you think?
rclcpp/include/rclcpp/node.hpp
Outdated
*/ | ||
RCLCPP_PUBLIC | ||
std::vector<rcl_interfaces::msg::SetParametersResult> | ||
load_parameters(const std::string & yaml_filepath, const std::string & node_name_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside the on-going discussion on how to implement, do we need to have 2nd argument node_name_
? what about load_parameters
takes yaml file
and internally calls get_fully_qualified_name()
. if load_parameters
belong to the Node class, i think it should load the parameters from yaml file in the context of the Node
?
I would have to agree with this statement, I feel that too much separation will create inconsistency thus a worse DX. Furthermore, filing a free function doesn't seem clear right now, as right now in testing I just stuck it in
Though the benefits of a free function would be its variability to take various NodeInterfaces, if we have a limited amount of nodes that we care about; for example if we only care about |
Looking at this holistically, I think my other problem with this is that our developer experience around parameters is already poor. We have too many node APIs that deal with parameters. Right now we have (all from rclcpp/rclcpp/include/rclcpp/node.hpp Lines 374 to 1254 in c10764f
And this list ignores the ParametersClient, ParameterEvents, ParameterEventsFilter, etc, that also revolve around parameters. I think we need to think long and hard before we add to this mess, because I don't think it is sustainable to keep adding APIs here. Instead I'm leaning towards just documenting how to do this, since I think this is completely achievable with the current APIs. |
+1 on documenting and/or add utility functions (that take node interfaces as input) as opposed to add APIs to the node class itself |
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
*/ | ||
template<typename NodeT> | ||
std::vector<rcl_interfaces::msg::SetParametersResult> | ||
load_parameters( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file name should be changed into something else like parameter_interfaces.hpp
? copy_all_parameter_values.hpp
sounds dedicated to rclcpp::copy_all_parameter_values
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I agree, I just didn't want to change it too fast. parameter_interfaces.hpp
sounds good to me.
template<typename NodeT> | ||
std::vector<rcl_interfaces::msg::SetParametersResult> | ||
load_parameters( | ||
const std::string & yaml_filepath, NodeT node_interface) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we didn't use a template here, but rather the NodeInterfaces
class or directly passing the two required interface (node_base_interface
and node_parameters_interface
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be better for compile time.
const std::string & yaml_filepath, NodeT node_interface) | ||
{ | ||
rclcpp::ParameterMap parameter_map = | ||
rclcpp::parameter_map_from_yaml_file(yaml_filepath, node_interface->get_name()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be get_fully_qualified_name
?
TEST_F(TestNode, TestFileImport) | ||
{ | ||
const uint64_t expected_param_count = 4; | ||
auto load_node = std::make_shared<rclcpp::Node>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can add LifecycleNode
test case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that not make a circular testing structure, as LifecycleNode
in lifecycle_rclcpp
needs to include rclcpp
.
Signed-off-by: CursedRock17 <mtglucas1@gmail.com>
* \throws InvalidParametersException if no valid parameters found. | ||
*/ | ||
std::vector<rcl_interfaces::msg::SetParametersResult> | ||
load_parameters( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear; further to my comments in #2406 (comment) , this PR is currently a NAK from me.
Does this mean Negative Acknowledgement? I'm not familiar with the acronym. If it does, I can see that point and would assume that in the context of:
you're mainly on board with just documentation. Though, how detrimental would a utility function - which uses Then, as a sidenote, would it be worth creating an issue for the long list of parameter APIs, if
It sounds like, some of them may not be necessary, as suggested with this message. That issue would just highlight the process for removing certain methods. |
Hi, is there any update on where this should be put ? How about the documentation ? Should it be only added to ros2 documentation or to some rclcpp functions as well |
This is a pull request meant to address #1029 and may start as a draft depending on the issue actually needing to be addressed. It adds the
load_parameters
function toNodeParametersInterface
and all of its children; a way to add parameters during runtime through a yaml file.