Skip to content

Allow declaring uninitialized parameters #798

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 19, 2021
Merged

Allow declaring uninitialized parameters #798

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6226539
Allow declaring uninitialized parameters
jacobperron May 18, 2021
c589221
Fix flake8
jacobperron May 18, 2021
5aeceba
Minor refactor
jacobperron May 18, 2021
5e8266f
Don't allow static parameters to be undeclared
jacobperron May 18, 2021
38f85c3
Return alternative for uninitialized parameters in get_parameter_or
jacobperron May 19, 2021
763ef63
Ditto for dynamic parameters
jacobperron May 19, 2021
1dda921
Fix lint
jacobperron May 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions rclpy/rclpy/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ def __init__(self, parameter, *args):
Exception.__init__(self, 'Attempted to modify read-only parameter', parameter, *args)


class NoParameterOverrideProvidedException(ParameterException):
"""Raised when no override is provided for a statically typed parameter with no default."""
class ParameterUninitializedException(ParameterException):
"""Raised when an uninitialized parameter is accessed."""

def __init__(self, parameter_name, *args):
Exception.__init__(
self,
f"No parameter override provided for '{parameter_name}' when one was expected",
f"The parameter '{parameter_name}' is not initialized",
*args)


Expand Down
28 changes: 20 additions & 8 deletions rclpy/rclpy/node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@
from rclpy.exceptions import InvalidParameterTypeException
from rclpy.exceptions import InvalidParameterValueException
from rclpy.exceptions import InvalidTopicNameException
from rclpy.exceptions import NoParameterOverrideProvidedException
from rclpy.exceptions import NotInitializedException
from rclpy.exceptions import ParameterAlreadyDeclaredException
from rclpy.exceptions import ParameterImmutableException
from rclpy.exceptions import ParameterNotDeclaredException
from rclpy.exceptions import ParameterUninitializedException
from rclpy.executors import Executor
from rclpy.expand_topic_name import expand_topic_name
from rclpy.guard_condition import GuardCondition
Expand Down Expand Up @@ -364,7 +364,6 @@ def declare_parameters(
parameters: List[Union[
Tuple[str],
Tuple[str, Parameter.Type],
Tuple[str, Any],
Copy link
Member

@ivanpauno ivanpauno May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't this for passing a value, e.g.: declare_parameters('', [('my_param', 5)])?

Copy link
Member Author

@jacobperron jacobperron May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the annotation was incorrect before. IIUC, we only expect a tuple up to length 3. The last value of the tuple can have type ParameterDescriptor or Any (for the case of passing a value).

Copy link
Member

@ivanpauno ivanpauno May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rclpy/rclpy/test/test_node.py

Line 630 in 1b335d3

('baz', 2.41),

We actually can be more specific than using Any, I think Union[int, float, str, bytes, None] would be fine

Tuple[str, Any, ParameterDescriptor],
]],
ignore_override: bool = False
Expand Down Expand Up @@ -467,9 +466,6 @@ def declare_parameters(
if not ignore_override and name in self._parameter_overrides:
value = self._parameter_overrides[name].value

if value is None and not descriptor.dynamic_typing:
raise NoParameterOverrideProvidedException(name)

if namespace:
name = f'{namespace}.{name}'

Expand All @@ -493,7 +489,9 @@ def declare_parameters(
raise_on_failure=True,
allow_undeclared_parameters=True
)
return self.get_parameters([parameter.name for parameter in parameter_list])
# Don't call get_parameters() to bypass check for NOT_SET parameters
parameter_names = [parameter.name for parameter in parameter_list]
return [self._parameters[name] for name in parameter_names]

def undeclare_parameter(self, name: str):
"""
Expand Down Expand Up @@ -562,6 +560,8 @@ def get_parameters(self, names: List[str]) -> List[Parameter]:
undeclared parameters are allowed.
:raises: ParameterNotDeclaredException if undeclared parameters are not allowed,
and at least one parameter hadn't been declared beforehand.
:raises: ParameterUninitializedException if at least one parameter is statically typed and
uninitialized.
"""
if not all(isinstance(name, str) for name in names):
raise TypeError('All names must be instances of type str')
Expand All @@ -577,9 +577,18 @@ def get_parameter(self, name: str) -> Parameter:
undeclared parameters are allowed.
:raises: ParameterNotDeclaredException if undeclared parameters are not allowed,
and the parameter hadn't been declared beforehand.
:raises: ParameterUninitializedException if the parameter is statically typed and
uninitialized.
"""
if self.has_parameter(name):
return self._parameters[name]
parameter = self._parameters[name]
if (
parameter.type_ != Parameter.Type.NOT_SET or
self._descriptors[name].dynamic_typing
):
return self._parameters[name]
# Statically typed, uninitialized parameter
raise ParameterUninitializedException(name)
elif self._allow_undeclared_parameters:
return Parameter(name, Parameter.Type.NOT_SET, None)
else:
Expand Down Expand Up @@ -941,7 +950,10 @@ def _apply_descriptor(

if descriptor.dynamic_typing:
descriptor.type = parameter.type_.value
elif descriptor.type != parameter.type_.value:
elif (
parameter.type_ != Parameter.Type.NOT_SET and
parameter.type_.value != descriptor.type
):
return SetParametersResult(
successful=False,
reason=(
Expand Down
24 changes: 21 additions & 3 deletions rclpy/test/test_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@
from rclpy.exceptions import InvalidParameterValueException
from rclpy.exceptions import InvalidServiceNameException
from rclpy.exceptions import InvalidTopicNameException
from rclpy.exceptions import NoParameterOverrideProvidedException
from rclpy.exceptions import ParameterAlreadyDeclaredException
from rclpy.exceptions import ParameterImmutableException
from rclpy.exceptions import ParameterNotDeclaredException
from rclpy.exceptions import ParameterUninitializedException
from rclpy.executors import SingleThreadedExecutor
from rclpy.parameter import Parameter
from rclpy.qos import qos_profile_sensor_data
Expand Down Expand Up @@ -631,8 +631,9 @@ def test_declare_parameters(self):
('value_not_set',)
]

with pytest.raises(NoParameterOverrideProvidedException):
self.node.declare_parameter('no_override', Parameter.Type.INTEGER)
# Declare uninitialized parameter
parameter_type = self.node.declare_parameter('no_override', Parameter.Type.INTEGER).type_
assert parameter_type == Parameter.Type.NOT_SET

with pytest.raises(InvalidParameterTypeException):
self.node.declare_parameter('initial_decl_wrong_type', Parameter.Type.INTEGER)
Expand Down Expand Up @@ -1833,16 +1834,33 @@ def test_integer_range_descriptor(self):
def test_static_dynamic_typing(self):
parameters = [
('int_param', 0),
('int_param_no_default', Parameter.Type.INTEGER),
('dynamic_param', None, ParameterDescriptor(dynamic_typing=True)),
]
result = self.node.declare_parameters('', parameters)

# Try getting parameters before setting values
int_param = self.node.get_parameter('int_param')
self.assertEqual(int_param.type_, Parameter.Type.INTEGER)
self.assertEqual(int_param.value, 0)
with pytest.raises(ParameterUninitializedException):
self.node.get_parameter('int_param_no_default')
self.assertEqual(self.node.get_parameter('dynamic_param').type_, Parameter.Type.NOT_SET)

result = self.node.set_parameters([Parameter('int_param', value='asd')])[0]
self.assertFalse(result.successful)
self.assertTrue(result.reason.startswith('Wrong parameter type'))

self.assertTrue(self.node.set_parameters([Parameter('int_param', value=3)])[0].successful)

result = self.node.set_parameters([Parameter('int_param_no_default', value='asd')])[0]
self.assertFalse(result.successful)
self.assertTrue(result.reason.startswith('Wrong parameter type'))

self.assertTrue(
self.node.set_parameters([Parameter('int_param_no_default', value=3)])[0].successful)
self.assertEqual(self.node.get_parameter('int_param_no_default').value, 3)

self.assertTrue(
self.node.set_parameters([Parameter('dynamic_param', value='asd')])[0].successful)
self.assertTrue(
Expand Down