rospy: added get_param_cached #1515
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mgrrx
force-pushed
the
rospy-get-param-cached
branch
from
95e9f34
to
f478a9b
Compare
|
Is there anything blocking merging on this? This brings the rospy client in-line with the behavior of roscpp client, and will solve a number of performance issues that I am seeing with rospy client. |
|
This PR was labeled as an enhancement and did not receive further feedback. It would be great if you could test this feature and report feedback. |
This was referenced Jan 31, 2019
Closed
|
We've been running this patch w/ locusrobotics#10 at Locus since February with success. Thanks @mgrrx! |
|
@mgrrx Hi, thank you very much for the patch! ( I tested the patch in #1855 ) IMO, It would be better to append |
|
@ros-pull-request-builder retest this please |
|
any chance to get this one in melodic? |
|
@ros-pull-request-builder retest this please |
|
Thanks for the improvement. |
howardcochran
pushed a commit
to BadgerTechnologies/ros_comm
that referenced
this pull request
Nov 22, 2021
This bug is severe because it can break ordinary rospy.get_param()
after a rospy.get_param_cached() has been perfomed. And, furthermore,
rospy.log*() does a rospy.get_param_cached()!
When a parameter is set in the cache, the cached value will always be
returned, even when the caller uses ordinary rospy.get_param(). This is
as designed, as subscribeParam() is done to make the cache
receive updates from the param server whenever the value changes.
However, existing code only uses the presence of a key in the cache to
indicate whether the cached param is valid. However, this is not enough.
Suppose the param server has these contents:
top1:
min: 1
max: 10
print(rospy.get_param('/top'))
# So far, so good. Output is:
# {'min': 1, 'max': 10}
print(rospy.get_param_cached('/top/min'))
# As expected, this outputs:
# 1
# However, ***'s cache now contains:
# {'top': {'min': 1}}
# This is correct, because only the value of 'min' is cached.
print(rospy.get_param('/top'))
# Incorrect output:
# {'top': {'min': 1}}
# Because ****'s cache makes it look like /top is in the cache, but
# it was never actually cached.
The fix is to track the actual keys that are cached in a set. Upon, only
return a cached value if either the key or a parent of it is in the set.
This was found by productioncode which attempts to get all params using
rospy.get_param('/'). This works OK as long as no calls to
rospy.get_param_cached() have been made. However, the rospy.log*()
functions all do
rospy.get_param_cached('rosout_disable_topics_generation'). So after the
first loginfo, subsequent calls to rospy.get_param('/') only the single
cached key rather than all params.
Fixes: d16296a ("rospy: added get_param_cached (ros#1515)")
howardcochran
pushed a commit
to BadgerTechnologies/ros_comm
that referenced
this pull request
Nov 23, 2021
This bug is severe because it can break ordinary rospy.get_param()
after a rospy.get_param_cached() has been perfomed. And, furthermore,
rospy.log*() does a rospy.get_param_cached()!
When a parameter is set in the cache, the cached value will always be
returned, even when the caller uses ordinary rospy.get_param(). This is
as designed, as subscribeParam() is done to make the cache
receive updates from the param server whenever the value changes.
However, existing code only uses the presence of a key in the cache to
indicate whether the cached param is valid. However, this is not enough.
Suppose the param server has these contents:
top:
min: 1
max: 10
print(rospy.get_param('/top'))
# So far, so good. Output is:
# {'min': 1, 'max': 10}
print(rospy.get_param_cached('/top/min'))
# As expected, this outputs:
# 1
# However, rospy.MastProxy's cache now contains:
# {'top': {'min': 1}}
# This is correct, because only the value of 'min' is cached.
print(rospy.get_param('/top'))
# Incorrect output:
# {'min': 1}
# Ugh! 'max': 10 is missing!
# Because rospy.MastProxy's cache makes it look like /top is in the
# cache, but it was never actually cached; only the individual item
# within it, 'min', was cached.
The fix is to track the actual keys that are cached in a Python set().
Only return a cached value if either the key or a parent of it is in
the set.
This was found by production code which attempts to get all params using
rospy.get_param('/'). This works OK as long as no calls to
rospy.get_param_cached() have been made. However, the rospy.log*()
functions all do
rospy.get_param_cached('rosout_disable_topics_generation'). So after the
first loginfo, subsequent calls to rospy.get_param('/') return only the
single cached key rather than all params.
Fixes: d16296a ("rospy: added get_param_cached (ros#1515)")
howardcochran
pushed a commit
to BadgerTechnologies/ros_comm
that referenced
this pull request
Nov 23, 2021
If rospy.get_param_cached() is called with a non-existent parameter,
existing code would attempt to encode that the parameter doesn't exist
by storing its value in the cache as an empty dictionary. And it would
correctly make a subscribeParam call to rosmaster to receive updates.
However, every subsequent .get_param_cached()
on this non-existent parameter would re-subscribe for updates from
rosmaster, nullifying any performance benefit from caching. Likewise, a
regular .get_param() would simply re-read the parameter from rosmaster
every time, still not benefitting from caching.
To make matters worse, the rospy.log* functions do a
rospy.get_param_cached('rosout_disable_topics_generation'), so we pay
this penalty on every log message whenever that paremeter is not set (it
is not set by default!).
The cause of this problem is that the translation of an empty dict in
the cache to a KeyError occurs within the cache itself, so the
caller can't know whether the parameter is simply unset vs. not
subscribed for updates.
The fix is simple: Move the check for empty dict from the cache itself
to the caller.
Fixes: d16296a ("rospy: added get_param_cached (ros#1515)")
howardcochran
pushed a commit
to BadgerTechnologies/ros_comm
that referenced
this pull request
Nov 23, 2021
This bug is severe because it can break ordinary rospy.get_param()
after a rospy.get_param_cached() has been perfomed. And, furthermore,
rospy.log*() does a rospy.get_param_cached()!
When a parameter is set in the cache, the cached value will always be
returned, even when the caller uses ordinary rospy.get_param(). This is
as designed, as subscribeParam() is done to make the cache
receive updates from the param server whenever the value changes.
However, existing code only uses the presence of a key in the cache to
indicate whether the cached param is valid. However, this is not enough.
Suppose the param server has these contents:
top:
min: 1
max: 10
print(rospy.get_param('/top'))
# So far, so good. Output is:
# {'min': 1, 'max': 10}
print(rospy.get_param_cached('/top/min'))
# As expected, this outputs:
# 1
# However, ParamServerCache now contains:
# {'top': {'min': 1}}
# This is correct, because only the value of 'min' is cached.
print(rospy.get_param('/top'))
# Incorrect output:
# {'min': 1}
# Ugh! 'max': 10 is missing!
# ...because it looks like /top is in the cache, but it was never
# actually cached; only the individual item within it, 'min', was
# cached.
The fix is to track the actual keys that are cached in a Python set().
Only return a cached value if either the key or a parent of it is in
the set.
This was found by production code which attempts to get all params using
rospy.get_param('/'). This works OK as long as no calls to
rospy.get_param_cached() have been made. However, the rospy.log*()
functions all do
rospy.get_param_cached('rosout_disable_topics_generation'). So after the
first loginfo, subsequent calls to rospy.get_param('/') return only the
single cached key rather than all params.
Fixes: d16296a ("rospy: added get_param_cached (ros#1515)")
howardcochran
pushed a commit
to BadgerTechnologies/ros_comm
that referenced
this pull request
Nov 23, 2021
If rospy.get_param_cached() is called with a non-existent parameter,
existing code would attempt to encode that the parameter doesn't exist
by storing its value in the cache as an empty dictionary. And it would
correctly make a subscribeParam call to rosmaster to receive updates.
However, every subsequent .get_param_cached()
on this non-existent parameter would re-subscribe for updates from
rosmaster, nullifying any performance benefit from caching. Likewise, a
regular .get_param() would simply re-read the parameter from rosmaster
every time, still not benefitting from caching.
To make matters worse, the rospy.log* functions do a
rospy.get_param_cached('rosout_disable_topics_generation'), so we pay
this penalty on every log message whenever that paremeter is not set (it
is not set by default!).
The cause of this problem is that the translation of an empty dict in
the cache to a KeyError occurs within the cache itself, so the
caller can't know whether the parameter is simply unset vs. not
subscribed for updates.
The fix is simple: Move the check for empty dict from the cache itself
to the caller.
Fixes: d16296a ("rospy: added get_param_cached (ros#1515)")
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rospy had an incomplete version of a local parameter cache in the code base. The code was never called due to https://github.com/ros/ros_comm/blob/melodic-devel/clients/rospy/src/rospy/msproxy.py#L119 .
I borrowed some of the code in rosmaster and added the rospy toplevel function
get_param_cached(in accordance toroscpp::getParamCached). It was surprisingly easy since the subscribeParam functionality was already there.