feat: integrate with rai_state_logs
#314
Conversation
boczekbartek
force-pushed
the
bb/feat/integrate_state_logs_with_agent
branch
2 times, most recently
from
a761b2a to
5485560
Compare
boczekbartek
changed the base branch from
development
to
bb/refactor/xml_launch_files_to_python
boczekbartek
changed the base branch from
bb/refactor/xml_launch_files_to_python
to
development
|
@coderabbitai full review |
WalkthroughThe pull request introduces significant modifications across several files, primarily focusing on restructuring the logging mechanisms in the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@boczekbartek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (14)
💤 Files with no reviewable changes (2)
🧰 Additional context used📓 Learnings (1)src/rai/rai/node.py (1)🔇 Additional comments (27)examples/rosbot-xl_allowlist.txt (1)
The sensor topics seem incomplete. Common ROS sensor topics that might be needed:
✅ Verification successfulThe initial search showed many files that might use these topics, but we need to verify if these topics are specifically used with ROSBot XL and if they're essential for its operation. Let's search for explicit topic usage and ROSBot XL specific configuration files to make a more informed decision. Let's examine the ROSBot XL launch file to understand what topics are actually required for this specific robot configuration, and look for any additional ROSBot related files that might indicate which topics are necessary. The launch file shows this is part of a demo setup and references additional scripts. Let's examine these scripts to understand what topics are actually required for the ROSBot XL's operation in this specific demo context. Current sensor topics are sufficient for this demo Based on the demo script and system prompt analysis:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for common sensor topics in the codebase
echo "Searching for camera_info, point cloud, and IMU topics..."
rg -l "camera_info|points|imu/data" | grep -v "allowlist"
Length of output: 784 Script: #!/bin/bash
# Search for actual usage of these topics in the codebase
echo "Searching for specific sensor topic usage..."
rg -A 2 "/camera/camera/color/camera_info|/camera/camera/depth/camera_info|/camera/camera/depth/points|/imu/data"
# Check if there are any ROSBot XL specific configurations or launch files
echo -e "\nSearching for ROSBot XL specific files..."
fd -t f "rosbot.*\.yaml|rosbot.*\.launch|rosbot.*\.xml"
Length of output: 400 Script: #!/bin/bash
# Check the ROSBot XL launch file to understand what topics are actually used
echo "Checking ROSBot XL launch file content..."
cat examples/rosbot-xl.launch.py
# Search for any ROSBot related documentation or configuration
echo -e "\nSearching for ROSBot related files..."
rg -l "rosbot" --type-not allowlist
Length of output: 2084 Script: #!/bin/bash
# Check the demo script and navigation script referenced in the launch file
echo "Checking ROSBot XL demo script..."
cat examples/rosbot-xl-demo.py
echo -e "\nChecking navigation script..."
cat src/examples/rai-rosbot-xl-demo/run-nav.bash
# Look for ROSBot XL related files with correct grep
echo -e "\nSearching for ROSBot files..."
find . -type f -exec grep -l "rosbot" {} \; | grep -v "allowlist"
Length of output: 6421 src/rai_whoami/setup.py (2)
The added imports are necessary for the new launch files configuration and follow Python's import style guidelines.
The launch files configuration is well-structured and uses cross-platform compatible path handling. The glob pattern covers all standard ROS 2 launch file extensions. Let's verify that all necessary launch files are captured by the glob pattern: ✅ Verification successfulLaunch files configuration is correctly set up and functional The verification confirms that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify launch files configuration
# 1. Check if launch directory exists
# 2. List all files in launch directory
# 3. Verify the glob pattern matches all launch files
# Check if launch directory exists and list its contents
if [ -d "src/rai_whoami/launch" ]; then
echo "Launch directory contents:"
ls -la src/rai_whoami/launch
echo -e "\nFiles matched by glob pattern:"
cd src/rai_whoami && python3 -c "from glob import glob; print('\n'.join(glob('launch/*launch.[pxy][yma]*')))"
else
echo "Note: No launch directory found in src/rai_whoami/"
fi
Length of output: 608 Script: #!/bin/bash
# Let's verify the launch files using just shell commands without relying on Python
echo "Launch files in the directory:"
find src/rai_whoami/launch -type f -name "*launch.[pxy][yma]*" 2>/dev/null || echo "No matching launch files found"
echo -e "\nContent of launch file(s) to verify structure:"
for file in $(find src/rai_whoami/launch -type f -name "*launch.[pxy][yma]*" 2>/dev/null); do
echo -e "\n=== $file ==="
head -n 5 "$file" 2>/dev/null || echo "Unable to read file"
done
Length of output: 827 src/rai_whoami/launch/rai_whoami.launch.py (3)
The file includes a proper Apache 2.0 license header and all necessary ROS 2 launch-related imports.
The launch configuration is well-structured with:
Given that this PR aims to integrate with examples/rosbot-xl.launch.py (4)
The copyright header includes all necessary licensing information and is correctly formatted.
The launch configuration is properly organized with:
Consider using FindPackageShare for script location. The - ExecuteProcess(
- cmd=["bash", "run-nav.bash"],
- cwd="src/examples/rai-rosbot-xl-demo",
- output="screen",
- ),
+ ExecuteProcess(
+ cmd=[
+ "bash",
+ str(FindPackageShare("rai_examples").find("rai_examples") / "rai-rosbot-xl-demo/run-nav.bash")
+ ],
+ output="screen",
+ ),
The allowlist configuration is being passed correctly, but let's verify its integration with the new logging system. ✅ Verification successfulIntegration with rai_state_logs is properly configured The verification confirms:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the allowlist file exists and check its format compatibility with rai_state_logs
# Check if the allowlist file exists
fd -t f "rosbot-xl_allowlist.txt" examples/
# Verify the content format of the allowlist file
if [ -f "examples/rosbot-xl_allowlist.txt" ]; then
echo "Allowlist content:"
cat "examples/rosbot-xl_allowlist.txt"
fi
# Check if sim_whoami_demo.launch.py properly handles the allowlist parameter
rg -A 5 "allowlist.*=.*LaunchConfiguration" "$(fd -t f "sim_whoami_demo.launch.py")"
Length of output: 859 src/examples/turtlebot4/turtlebot.launch.py (3)
The copyright header and imports are well-structured and include all necessary components for the launch file functionality.
The PR objectives mention integration with ✅ Verification successfulrai_state_logs integration is properly configured The IncludeLaunchDescription(
PythonLaunchDescriptionSource([
FindPackageShare("rai_state_logs"),
"/launch/rai_state_logs.launch.py",
])
),This ensures that the 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for rai_state_logs configuration in the included launch file
echo "Checking for rai_state_logs configuration..."
rg -A 5 "rai_state_logs" $(fd -t f "sim_whoami_demo.launch.py" -p "rai_bringup")
Length of output: 11045
Consider using FindPackageShare for file paths The launch arguments use relative paths which might be problematic in different installation scenarios. Consider using Example refactor: launch_arguments={
- "allowlist": "src/examples/turtlebot4/allowlist.txt",
- "demo_script": "src/examples/turtlebot4/turtlebot_demo.py",
+ "allowlist": [FindPackageShare("your_package"), "/examples/turtlebot4/allowlist.txt"],
+ "demo_script": [FindPackageShare("your_package"), "/examples/turtlebot4/turtlebot_demo.py"],
"robot_description_package": "turtlebot4_whoami",
"game_launcher": game_launcher,
}.items(),Let's verify the existence of required files: src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py (1)
The change from examples/rosbot-xl-demo.py (2)
The implementation of command-line argument parsing is clean and follows Python best practices with proper type hints. Also applies to: 39-40
The node is initialized with src/rai/rai/node.py (4)
The imports reflect the transition from RosoutBuffer to the new logging system using NodeDiscovery and create_logs_parser.
The examples using RaiStateBasedLlmNode need to be updated to explicitly set the logs_parser_type parameter for better clarity and maintainability.
Enhance the summarize_logs method. The method needs documentation, error handling, and logging for better maintainability and debugging. def summarize_logs(self) -> str:
- return self.logs_parser.summarize()
+ """Summarize the logs using the configured parser.
+
+ Returns:
+ str: A summary of the logs. Returns empty string if summarization fails.
+ """
+ try:
+ summary = self.logs_parser.summarize()
+ self.get_logger().debug(f"Log summary: {summary}")
+ return summary
+ except Exception as e:
+ self.get_logger().error(f"Failed to summarize logs: {e}")
+ return ""
Add error handling for logs parser creation. The logs parser initialization could fail, but there's no error handling. Consider wrapping it in a try-except block to handle potential initialization failures gracefully. # We have to use a separate node that we can manually spin for ros-service based
# parser and this node ros-subscriber based parser
logs_parser_node = self if logs_parser_type == "llm" else self._async_tool_node
- self.logs_parser = create_logs_parser(
- logs_parser_type, logs_parser_node, callback_group=self.callback_group
- )
+ try:
+ self.logs_parser = create_logs_parser(
+ logs_parser_type, logs_parser_node, callback_group=self.callback_group
+ )
+ except Exception as e:
+ self.get_logger().error(f"Failed to create logs parser: {e}")
+ raise RuntimeError(f"Failed to initialize {logs_parser_type} parser") from e
src/rai_bringup/launch/sim_whoami_demo.launch.py (3)
The Apply this diff to add the following declaration: + DeclareLaunchArgument(
+ "robot_description_package",
+ description="Package containing robot description files",
+ ),
The default Apply this diff to update the default value: DeclareLaunchArgument(
"allowlist",
- default_value="src/examples/turtlebot4/allowlist.txt",
+ default_value=[
+ FindPackageShare("rai_examples"),
+ "/config/default_allowlist.txt",
+ ],
description="A list of ros interfaces that are exposed to rai agent.",
),
The launch file correctly integrates the required components, including the src/rai/rai/utils/ros_logs.py (4)
The error message in the
When calling Apply this diff to include exception handling: future = self.rai_state_logs_client.call_async(request)
try:
rclpy.spin_until_future_complete(self.node, future)
response: Optional[rai_interfaces.srv.StringList.Response] = future.result()
except Exception as e:
self.node.get_logger().error(f"Service call failed with exception: {e}")
return ""
if response is None or not response.success:
self.node.get_logger().error(f"'{self.SERVICE_NAME}' service call failed")
return ""
Currently, the code assumes that Apply this diff to include error handling: def summarize(self):
if len(self._buffer) == 0:
return "No logs"
buffer = self.get_raw_logs()
self.clear()
+ try:
response = self.llm.invoke({"rosout": buffer})
+ except Exception as e:
+ self.node.get_logger().error(f"LLM invocation failed: {e}")
+ return "Error during log summarization"
return str(response.content)
The Consider using a threading lock to synchronize access to +import threading
class LlmRosoutParser(BaseLogsParser):
"""Bufferize `/rosout` and summarize it with LLM"""
def __init__(
self,
llm: BaseChatModel,
node: rclpy.node.Node,
callback_group: rclpy.callback_groups.CallbackGroup,
bufsize: int = 100,
):
self.bufsize = bufsize
self._buffer: Deque[str] = deque()
+ self._buffer_lock = threading.Lock()
self.template = ChatPromptTemplate.from_messages(
[
(
"system",
"Shorten the following log keeping its format - for example merge similar or repeating lines",
),
("human", "{rosout}"),
]
)
self.llm = self.template | llm
self.node = node
rosout_qos = rclpy.qos.QoSProfile(
reliability=rclpy.qos.ReliabilityPolicy.BEST_EFFORT,
durability=rclpy.qos.DurabilityPolicy.VOLATILE,
depth=10,
)
self.rosout_subscription = self.init_rosout_subscription(
self.node, callback_group, rosout_qos
)
def rosout_callback(self, msg: rcl_interfaces.msg.Log):
self.node.get_logger().debug(f"Received rosout: {msg}")
if "rai_node" in msg.name:
return
- self.append(f"[{msg.stamp.sec}][{msg.name}]:{msg.msg}")
+ with self._buffer_lock:
+ self.append(f"[{msg.stamp.sec}][{msg.name}]:{msg.msg}")
def clear(self):
+ with self._buffer_lock:
self._buffer.clear()
def append(self, line: str):
+ with self._buffer_lock:
self._buffer.append(line)
if len(self._buffer) > self.bufsize:
self._buffer.popleft()
def get_raw_logs(self, last_n: int = 30) -> str:
+ with self._buffer_lock:
return "\n".join(list(self._buffer)[-last_n:])
def summarize(self):
if len(self._buffer) == 0:
return "No logs"
+ with self._buffer_lock:
buffer = self.get_raw_logs()
self.clear()
try:
response = self.llm.invoke({"rosout": buffer})
except Exception as e:
self.node.get_logger().error(f"LLM invocation failed: {e}")
return "Error during log summarization"
return str(response.content)Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py (1)
44-44: Enhance the comment to explain the ROS 2 workaround.The current comment "this means that no filters will be applied" could be more informative about why this specific string format is used.
- default_value='[""]', # this means that no filters will be applied + default_value='[""]', # Empty list in string format to work around ROS 2 parameter handling limitationsrc/rai_bringup/launch/sim_whoami_demo.launch.py (1)
49-56: Consider adding configuration flexibility for rai_state_logs.The integration of
rai_state_logslooks good, but consider exposing key configuration parameters through launch arguments to make the setup more flexible and maintainable.Example enhancement:
IncludeLaunchDescription( PythonLaunchDescriptionSource( [ FindPackageShare("rai_state_logs"), "/launch/rai_state_logs.launch.py", ] ), + launch_arguments={ + "filters": LaunchConfiguration("log_filters", default="[]"), + }.items(), ),And add the corresponding launch argument:
DeclareLaunchArgument( "log_filters", default_value="[]", description="Filters for rai_state_logs configuration", ),src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp (2)
45-50: Improve documentation of the ROS 2 workaround.The workaround for the ROS 2 issue is implemented correctly. However, consider rephrasing the comment to be more professional and include when this workaround can be removed.
- // Hack to overcome https://github.com/ros2/rclcpp/issues/1955 + // Workaround for empty filters handling (see https://github.com/ros2/rclcpp/issues/1955) + // TODO: Remove this workaround once the issue is fixed in ROS 2
Thread safety concerns confirmed in concurrent access patterns
The code analysis reveals multiple instances of unprotected concurrent access to shared data structures:
log_callbackmodifies bothlogs_andtimestamps_through:
- Insert/erase operations on
logs_- Multiple modifications to
timestamps_including clear operations
digest_request_callbackconcurrently accesses the same data:
- Reads from both maps
- Performs clear operations when
clear_on_retrieval_is trueThese operations are not protected by any synchronization mechanism, while other nodes in the codebase (e.g.,
rai_whatisee_node.cpp) properly use mutex protection.Example locations requiring synchronization:
- Line 67:
logs_.find()in digest callback- Line 134: Multiple map operations in log callback
- Line 146: Size check and erasure operations
🔗 Analysis chain
Line range hint
1-186: Consider essential improvements for robustness and performance.While the overall implementation is solid, consider the following improvements:
Thread Safety: The
logs_andtimestamps_maps are accessed from both the subscription callback and service callback. Consider adding mutex protection to prevent potential race conditions.Memory Management: The rolling window implementation could be more efficient by using a circular buffer or similar data structure instead of map erasure operations.
String Operations: Consider reducing string allocations in the hot path:
- Pre-allocate strings in
formatMessageandformatSource- Use string_view for filter operations
- Consider using a string pool for repeated messages
Here's an example of adding thread safety:
#include <mutex> class LogDigestNode : public rclcpp::Node { private: std::mutex logs_mutex_; // Add mutex for thread safety void log_callback(const rcl_interfaces::msg::Log & msg) { // ... std::lock_guard<std::mutex> lock(logs_mutex_); // ... existing log processing code ... } void digest_request_callback( [[maybe_unused]] const std::shared_ptr<rai_interfaces::srv::StringList::Request> request, const std::shared_ptr<rai_interfaces::srv::StringList::Response> response) { std::lock_guard<std::mutex> lock(logs_mutex_); // ... existing callback code ... } };Let's verify the thread safety concerns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential thread safety issues by looking for concurrent access patterns # Test: Search for subscription callbacks and service handlers accessing shared resources # Look for shared resource access patterns rg -A 5 "logs_\." rg -A 5 "timestamps_\." # Look for existing mutex usage rg "mutex|lock_guard|unique_lock"Length of output: 6024
src/rai/rai/utils/ros.py (5)
Line range hint
26-33: Rename thesetmethod to avoid shadowing built-in namesIt's best practice to avoid naming methods the same as built-in Python functions to prevent confusion. Consider renaming the
setmethod to something more descriptive likeupdateorconfigure.
Line range hint
35-43: Use single underscore for private methods instead of double underscoresDouble underscores in method names trigger name mangling in Python, which is typically used for class-private variables. Since
__filteris intended as a private method within the class, consider renaming it to_filterto follow standard naming conventions.
Line range hint
24-24: Review the default value andNonecheck forallowlistThe
allowlistattribute is initialized as an empty list (default_factory=list), so it will never beNone. The conditionif self.allowlist is not None:at line 32 will always beTrue. If you intend forallowlistto be optional and allowNone, consider setting its default value toNone.Also applies to: 32-33
Line range hint
27-28: Handle potential empty lists into_dictto preventIndexErrorIn the
to_dictfunction, accessingv[0]assumes that eachvininfois a non-empty list. Ifvis an empty list, this will raise anIndexError. Consider adding a check to ensurevis not empty before accessingv[0].
Line range hint
45-50: Rename thedictmethod to avoid shadowing built-in typesNaming the method
dictcan overshadow Python's built-indicttype, potentially causing confusion. Consider renaming the method toas_dictorto_dictfor improved clarity.src/rai/rai/utils/ros_logs.py (3)
65-69: Consider adding a timeout or handling for the service wait loopThe while loop waiting for the service
/get_log_digestcan potentially become infinite if the service is unavailable. Consider adding a timeout mechanism or maximum retry attempts to prevent indefinite blocking.For example, you can implement a retry counter:
max_retries = 10 retry_count = 0 while not self.rai_state_logs_client.wait_for_service(timeout_sec=1.0): self.node.get_logger().info( f"'{self.SERVICE_NAME}' service is not available, waiting again..." ) retry_count += 1 if retry_count >= max_retries: self.node.get_logger().error(f"Service '{self.SERVICE_NAME}' unavailable after {max_retries} attempts.") break
135-136: Use exact match for node name filtering inrosout_callbackUsing
into check if"rai_node"is inmsg.namemight unintentionally exclude messages from nodes with similar names. It is safer to use an exact match to ensure only the intended node is filtered out.Apply this diff to improve the filtering:
if "rai_node" in msg.name: + # Change to exact match - if "rai_node" in msg.name: + if msg.name == "rai_node": return
133-134: Adjust logging level for received rosout messagesThe debug log in
rosout_callbackmay produce excessive output in large systems. Consider adjusting the logging level or adding a condition to log only when necessary.If detailed logs are not required, you might disable this log or set it to a higher level:
- self.node.get_logger().debug(f"Received rosout: {msg}") + # Consider commenting out or adjusting the log level + # self.node.get_logger().debug(f"Received rosout: {msg}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/rai/rai/node.py(4 hunks)src/rai/rai/utils/ros.py(1 hunks)src/rai/rai/utils/ros_logs.py(1 hunks)src/rai_bringup/launch/sim_whoami_demo.launch.py(1 hunks)src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py(1 hunks)src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp(1 hunks)
🔇 Additional comments (6)
src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py (1)
44-44: Verify string parsing of empty filters.
The change from None to '[""]' looks good as it addresses the ROS 2 library issue. However, we should verify that the node correctly parses this string representation.
✅ Verification successful
String parsing of empty filters is properly handled
The node correctly handles the '[""]' filter format:
- Uses
as_string_array()to parse the parameter value - Explicitly handles the empty filter case by checking for a single empty string (
filters_.size() == 1 && filters_[0].empty()) - Clears the filters vector in this case, effectively applying no filtering
- The implementation aligns with the comment in the launch file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the node handles the '[""]' filter format correctly
# Look for filter parsing logic in the node implementation
# Search for filter parsing in the node
ast-grep --pattern 'def $_($$$) -> $_:
$$$
filters = $_
$$$'
# Backup: Search for any filter-related code
rg -A 5 'filters.*=|parse.*filter'
Length of output: 1506
src/rai_bringup/launch/sim_whoami_demo.launch.py (1)
49-56: Verify launch sequence dependencies.
The launch sequence appears correct, but since this PR depends on PR 322, we should verify that the launch order satisfies any initialization requirements.
✅ Verification successful
Launch sequence is correctly ordered
After examining both launch files and their dependencies, I can confirm that the launch sequence is properly ordered. The rai_state_logs node is a standalone logging component that doesn't have any initialization dependencies on other components. It's launched first in sim_whoami_demo.launch.py, which is appropriate as it allows capturing logs from the startup of subsequent components. The node only requires basic ROS 2 infrastructure to be running and doesn't have any special ordering requirements with respect to other nodes in the launch sequence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any explicit ordering requirements in the dependent PR
# and related launch files
# Check for any launch-related changes in PR 322
gh pr view 322 --json files -q '.files[].path' | grep -E 'launch|\.xml$|\.yaml$'
# Check for any ordering requirements in rai_state_logs launch file
rg -A 5 "launch_arguments|condition|PushRosNamespace|GroupAction" $(fd -t f rai_state_logs.launch.py)
Length of output: 355
Script:
#!/bin/bash
# Let's examine the content of both launch files to understand their dependencies
# First, let's look at the content of sim_whoami_demo.launch.py
cat src/rai_bringup/launch/sim_whoami_demo.launch.py
# Then, let's look at the content of rai_state_logs.launch.py
cat src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py
# Also check for any dependencies declared in package.xml
cat src/state_tools/rai_state_logs/package.xml
Length of output: 6369
src/rai/rai/utils/ros_logs.py (1)
151-157: Ensure consistent return type in summarize method
The method summarize returns a string, but in the case of no logs, it returns "No logs". Confirm that this is the desired behavior and that downstream code can handle this message appropriately.
Ensure that the return value "No logs" is handled correctly wherever summarize is called.
src/rai/rai/node.py (3)
54-55: Imports updated to include NodeDiscovery and create_logs_parser
The new imports of NodeDiscovery and create_logs_parser are correctly added to support the updated logging mechanism.
348-348: Added logs_parser_type parameter to RaiStateBasedLlmNode
The addition of the logs_parser_type parameter allows for flexibility in choosing the logs parser. The default value "rai_state_logs" is appropriately set.
405-406: New summarize_logs method correctly delegates to logs_parser
The summarize_logs method is properly added and delegates the call to self.logs_parser.summarize() as expected.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
src/rai_whoami/setup.py (1)
31-34: Consider tightening the launch file pattern and adding directory verificationWhile the implementation works, there are two suggestions for improvement:
- The current pattern
*launch.[pxy][yma]*could match unintended files (e.g.,.pyml). Consider using a more specific pattern.- Add verification for the launch directory's existence during setup.
Here's a suggested improvement:
- ( - os.path.join("share", package_name, "launch"), - glob(os.path.join("launch", "*launch.[pxy][yma]*")), - ), + ( + os.path.join("share", package_name, "launch"), + glob(os.path.join("launch", "*launch.py")) + + glob(os.path.join("launch", "*launch.yaml")) + + glob(os.path.join("launch", "*launch.yml")) + if os.path.exists("launch") else [], + ),src/rai_whoami/launch/rai_whoami.launch.py (1)
30-42: Consider adding namespace and logging configurationA few suggestions to enhance the node configuration:
- Add a namespace parameter to support multi-robot scenarios
- Add configuration for the
rai_state_logsintegration mentioned in the PR objectivesExample configuration:
Node( package="rai_whoami", executable="rai_whoami_node", name="rai_whoami_node", + namespace=LaunchConfiguration('robot_namespace', default=''), parameters=[ { "robot_description_package": LaunchConfiguration( "robot_description_package" ) + "use_rai_state_logs": True, + "log_parser_type": "rai_state_logs" } ], output="screen", ),src/rai_bringup/launch/sim_whoami_demo.launch.py (1)
69-73: Consider making the Streamlit HMI path configurable.The path to the Streamlit HMI script is hardcoded. This could break if the package structure changes.
Consider using
FindPackageShare:ExecuteProcess( - cmd=shlex.split("streamlit run src/rai_hmi/rai_hmi/text_hmi.py") + cmd=shlex.split("streamlit run " + + str(FindPackageShare("rai_hmi").perform(context)) + + "/rai_hmi/text_hmi.py") + [robot_description_package, allowlist], output="screen", ),examples/rosbot-xl-demo.py (2)
43-44: Enhance error handling and documentationWhile the implementation is functional, consider these improvements:
- Be more specific about the exceptions you're catching
- Provide more context in the warning message about empty allowlist implications
- Document the transition from hardcoded topics to dynamic allowlist
Consider this enhanced implementation:
def main(allowlist: Optional[Path] = None): rclpy.init() ros2_allowlist = [] if allowlist is not None: try: content = allowlist.read_text().strip() if content: ros2_allowlist = content.splitlines() else: rclpy.logging.get_logger("rosbot_xl_demo").warning( - "Allowlist file is empty" + "Allowlist file is empty. No topics will be filtered." ) - except Exception as e: + except (IOError, OSError) as e: rclpy.logging.get_logger("rosbot_xl_demo").error( f"Failed to read allowlist: {e}" )Also applies to: 50-63
Line range hint
44-116: Consider externalizing environment configurationThe system prompt contains hardcoded coordinates and environment information. Consider moving this configuration to a separate YAML file for better maintainability.
src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp (2)
45-50: LGTM! Consider enhancing error handling.The workaround for ROS 2 issue #1955 is well-implemented and properly documented. The code correctly handles the edge case where the filters parameter contains a single empty string.
Consider enhancing the error handling by:
- Adding debug logging to track when this workaround is applied
- Making the workaround more maintainable by extracting it into a helper method
if (filters_.size() == 1 && filters_[0].empty()) { + RCLCPP_DEBUG( + get_logger(), + "Applying workaround for ROS 2 issue #1955: clearing single empty filter"); filters_.clear(); }
45-50: Consider architectural improvements for better performance and maintainability.While the current implementation works correctly, here are some suggestions for future improvements:
- The rolling window implementation using
std::mapandstd::unordered_mapcould be optimized using a circular buffer or a more efficient data structure.- Consider extracting the log filtering logic into a separate class to improve separation of concerns and testability.
- The log message formatting could be moved to a separate utility class.
Would you like me to provide more detailed suggestions for any of these improvements?
src/rai/rai/node.py (1)
397-402: Document the node separation logicThe comment about using separate nodes for different parser types could be more descriptive. Consider adding a detailed docstring explaining why this separation is necessary and the implications of each parser type.
- # We have to use a separate node that we can manually spin for ros-service based - # parser and this node ros-subscriber based parser + # Parser node selection: + # - "llm" parser: Uses the current node as it relies on ROS subscribers + # - "rai_state_logs" parser: Uses a separate node that can be manually spun + # for ROS service-based operations to prevent blockingsrc/rai/rai/utils/ros.py (4)
Line range hint
21-22: Clarify 'allowlist' default value and usageThe
allowlistfield is declared withOptional[List[str]]and defaults to an empty list. Sincedefault_factory=listinitializes it as an empty list,self.allowlistwill never beNone. However, in thesetmethod, you checkif self.allowlist is not None, which suggests thatself.allowlistcould beNone. To avoid confusion and ensure consistent behavior, consider setting the default value ofallowlisttoNoneand updating the check, or remove theifcondition if an empty list is acceptable.Apply this diff to set
allowlistdefault toNone:- allowlist: Optional[List[str]] = field(default_factory=list) + allowlist: Optional[List[str]] = NoneAnd update the check in the
setmethod:def set(self, topics, services, actions): ... - if self.allowlist is not None: + if self.allowlist: self.__filter(self.allowlist)Also applies to: 30-31
Line range hint
23-25: Add type annotations to 'set' method parameters for clarityThe
setmethod parameterstopics,services, andactionslack type annotations, which can reduce code readability and type safety. Since theto_dictfunction expectsinfo: List[Tuple[str, List[str]]], adding type hints to the method parameters will enhance clarity and help with static type checking.Apply this diff to add type annotations:
def set(self, topics, services, actions): + def set( + self, + topics: List[Tuple[str, List[str]]], + services: List[Tuple[str, List[str]]], + actions: List[Tuple[str, List[str]]], + ):
Line range hint
26-27: Handle potential empty lists in 'to_dict' function to avoid IndexErrorIn the
to_dictfunction, accessingv[0]assumes thatvis a non-empty list. Ifvis empty, this will raise anIndexError. To prevent this, consider adding a check to ensure thatvis not empty before accessing the first element.Apply this diff to handle empty
ventries:def to_dict(info: List[Tuple[str, List[str]]]) -> Dict[str, str]: - return {k: v[0] for k, v in info} + return {k: v[0] for k, v in info if v}This modification skips entries where
vis an empty list. Alternatively, you could decide on appropriate handling, such as assigning a default value or raising a specific exception ifvis empty.
Line range hint
35-42: Consider renaming '__filter' to '_filter' to follow Python conventionsThe method
__filteruses double underscores, which triggers name mangling in Python and is typically reserved for special methods. If name mangling is not necessary, renaming it to_filteradheres to Python's convention for indicating a method is intended for internal use.Apply this diff to rename the method:
- def __filter(self, allowlist: List[str]): + def _filter(self, allowlist: List[str]): for d in [ self.topics_and_types, self.services_and_types, self.actions_and_types, ]: to_remove = [k for k in d if k not in allowlist] for k in to_remove: d.pop(k) def set(self, topics, services, actions): ... - if self.allowlist is not None: - self.__filter(self.allowlist) + if self.allowlist: + self._filter(self.allowlist)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
examples/rosbot-xl-demo.py(4 hunks)examples/rosbot-xl.launch.py(1 hunks)examples/rosbot-xl_allowlist.txt(1 hunks)examples/rosbotxl.launch.xml(0 hunks)src/examples/turtlebot4/turtlebot.launch.py(1 hunks)src/examples/turtlebot4/turtlebot.launch.xml(0 hunks)src/rai/rai/node.py(4 hunks)src/rai/rai/utils/ros.py(1 hunks)src/rai/rai/utils/ros_logs.py(1 hunks)src/rai_bringup/launch/sim_whoami_demo.launch.py(1 hunks)src/rai_whoami/launch/rai_whoami.launch.py(1 hunks)src/rai_whoami/setup.py(2 hunks)src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py(1 hunks)src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- examples/rosbotxl.launch.xml
- src/examples/turtlebot4/turtlebot.launch.xml
🧰 Additional context used
📓 Learnings (2)
examples/rosbot-xl.launch.py (1)
Learnt from: boczekbartek
PR: RobotecAI/rai#324
File: examples/rosbot-xl.launch.py:40-44
Timestamp: 2024-11-28T14:37:22.619Z
Learning: In the `rosbot-xl.launch.py` and `turtlebot.launch.py` launch files, the `run-nav.bash` scripts are different for each demo and should remain as separate calls to maintain clarity in the demo launches.
examples/rosbot-xl_allowlist.txt (1)
Learnt from: boczekbartek
PR: RobotecAI/rai#324
File: examples/rosbot-xl_allowlist.txt:1-8
Timestamp: 2024-11-28T14:34:27.968Z
Learning: In the context of ROS2 applications within the `rai` system, the topics `/from_human`, `/to_human`, `/asr_status`, `/tts_status`, and `/hmi_status` are internal and should not be included in the allowlist files.
🔇 Additional comments (24)
examples/rosbot-xl_allowlist.txt (2)
1-8: LGTM! Essential system and sensor topics are properly configured.
The allowlist correctly includes fundamental ROS topics for:
- System logging via
/rosout - Camera feeds (color and depth images)
- Core robot functionality (mapping, scanning, movement, diagnostics)
9-22: LGTM! Navigation and control topics are comprehensive.
The allowlist includes a complete set of navigation and control topics:
- Path planning and computation
- Waypoint following
- Docking/undocking operations
- Basic movement commands
Let's verify that these navigation topics match the actual services provided by the navigation stack:
✅ Verification successful
Navigation topics in allowlist match the actual Nav2 implementation
The verification confirms that the allowlist entries correspond to actual navigation capabilities as evidenced by:
- Navigation parameters in
navigation_params_humble.yamlandnavigation_params_jazzy.yamlshowing matching actions:navigate_to_pose,navigate_through_posescompute_path_to_pose,compute_path_through_posesspin,wait,backup,drive_on_heading- Waypoint following functionality
- Docking operations
The allowlist accurately reflects the standard Nav2 action and service interfaces used in ROS 2 navigation stack.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the navigation topics in the allowlist match the actual navigation stack services
# Expected: Find corresponding service/action definitions in the nav2 packages
# Search for navigation-related service and action definitions
rg -t idl -A 5 "service|action" | grep -E "compute_path|follow|navigate|dock|undock|spin|wait"
Length of output: 124
Script:
#!/bin/bash
# Let's search for navigation-related files and definitions using different approaches
# Search for navigation-related files
echo "=== Navigation-related files ==="
fd -t f -e action -e srv
# Search for navigation-related content in all files
echo -e "\n=== Navigation-related content ==="
rg -l "action|service" | xargs rg -l "compute_path|follow|navigate|dock|undock|spin|wait"
# Search specifically in launch files and configs that might reference these actions
echo -e "\n=== Launch and config files referencing navigation actions ==="
fd -e launch -e yaml -e xml | xargs rg "compute_path|follow|navigate|dock|undock|spin|wait"
Length of output: 6236
src/rai_whoami/setup.py (1)
16-17: LGTM! Clean import additions
The added imports for os and glob are appropriate for handling file paths and pattern matching in the setup configuration.
src/rai_whoami/launch/rai_whoami.launch.py (4)
1-15: LGTM: License header is properly formatted
The Apache 2.0 license header is correctly formatted with appropriate copyright information.
16-20: LGTM: Imports are complete and well-organized
All necessary launch and ROS 2 components are imported and logically organized.
25-29: Consider making the default package name more generic
The default value "rosbot_xl_whoami" suggests this is specific to RosBot XL, but based on the description and PR objectives, this should be a more generic implementation.
Consider using a more generic default value or documenting why RosBot XL is the default choice.
22-44: 🛠️ Refactor suggestion
Add error handling for invalid robot_description_package
Consider adding validation for the robot_description_package parameter to ensure it exists and is properly formatted.
You could add a condition group to validate the package:
from launch.conditions import IfCondition
from launch.substitutions import PythonExpression
from ament_index_python.packages import get_package_share_directory
# Add before the Node action
ValidatePackage(
condition=IfCondition(
PythonExpression([
"not '",
LaunchConfiguration('robot_description_package'),
"' in '",
get_package_share_directory('rai_whoami'),
"'"
])
),
message="Invalid robot_description_package specified"
)examples/rosbot-xl.launch.py (3)
1-14: LGTM! License header is properly formatted.
15-20: LGTM! Imports are appropriate and well-organized.
33-38: Verify the existence of referenced files and packages.
The launch arguments reference several files and packages that need to be verified:
examples/rosbot-xl_allowlist.txtexamples/rosbot-xl-demo.pyrosbot_xl_whoamipackage
src/examples/turtlebot4/turtlebot.launch.py (3)
1-21: LGTM: Copyright and imports are properly structured
The copyright header is complete, and imports are organized logically with appropriate grouping.
23-54: Document and verify the game_launcher parameter
The game_launcher configuration is used but not documented. Consider:
- Adding a docstring to explain the purpose and expected values of this parameter
- Verifying that this parameter is properly handled when not provided
Let's check how this parameter is used in the referenced launch file:
35-38: 🛠️ Refactor suggestion
Improve path handling robustness
The current implementation uses hardcoded relative paths which could break if:
- The launch file is called from a different working directory
- The repository structure changes
- The package is installed system-wide
Consider using FindPackageShare for package-relative paths. Example refactor:
launch_arguments={
- "allowlist": "src/examples/turtlebot4/allowlist.txt",
- "demo_script": "src/examples/turtlebot4/turtlebot_demo.py",
+ "allowlist": [FindPackageShare("rai_examples"), "/turtlebot4/allowlist.txt"],
+ "demo_script": [FindPackageShare("rai_examples"), "/turtlebot4/turtlebot_demo.py"],
"robot_description_package": "turtlebot4_whoami",
"game_launcher": game_launcher,
}.items(),
ExecuteProcess(
cmd=["bash", "run-nav.bash"],
- cwd="src/examples/turtlebot4",
+ cwd=[FindPackageShare("rai_examples"), "/turtlebot4"],
output="screen",
),
Node(
package="rviz2",
executable="rviz2",
name="rviz2",
output="screen",
- arguments=["-d", "src/examples/turtlebot4/config.rviz"],
+ arguments=["-d", [FindPackageShare("rai_examples"), "/turtlebot4/config.rviz"]],
),Let's verify if these files are properly packaged:
Also applies to: 42-43, 51-51
src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py (1)
44-44: LGTM! Good fix for the ROS 2 empty filters issue.
The change from None to '[""]' is a good solution that:
- Maintains valid JSON format
- Effectively represents "no filters" as intended
- Avoids the known ROS 2 library issue with empty filters
Let's verify that the JSON string format is valid and properly handled:
✅ Verification successful
Default filter value '[""]' is properly handled by the node implementation
The C++ implementation shows that:
- The node correctly handles the
'[""]'default value through a specific check:
if (filters_.size() == 1 && filters_[0].empty()) {
filters_.clear();
}- This check explicitly addresses the ROS 2 issue (referenced in code:
https://github.com/ros2/rclcpp/issues/1955) - The parameter is properly declared as a string vector and parsed using
as_string_array() - Empty filters result in no filtering, as intended
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the JSON string format and its handling
# Check if the node properly handles the default filter value
rg -A 5 "filters.*=.*\[\"\"\]" src/state_tools/rai_state_logs/
# Look for any JSON parsing of filters to ensure compatibility
ast-grep --pattern 'json.$_(filters)'
Length of output: 102
Script:
#!/bin/bash
# Let's try different approaches to verify the JSON handling:
# 1. Search for filter-related code in Python files
rg -t python "filters.*=.*\[" src/state_tools/rai_state_logs/
# 2. Look for JSON imports and usage
rg -t python "import.*json" src/state_tools/rai_state_logs/
# 3. Search for filter parameter handling
ast-grep --pattern 'self.declare_parameter("filters")'
# 4. Look for string list parameter declarations
rg -t python "StringArray|Array.*String" src/state_tools/rai_state_logs/
# 5. Check for filter processing logic
rg -t python "def.*filter" src/state_tools/rai_state_logs/
Length of output: 442
Script:
#!/bin/bash
# Let's try without file type filtering since Python type isn't recognized:
# 1. Search for filter-related code
rg "filters.*=.*\[" src/state_tools/rai_state_logs/
# 2. Look for JSON imports and usage
rg "import.*json" src/state_tools/rai_state_logs/
# 3. Look for filter parameter declarations and usage
rg "filters.*parameter" src/state_tools/rai_state_logs/
# 4. Look for string array type hints or declarations
rg "List\[str\]|StringArray" src/state_tools/rai_state_logs/
# 5. Check the node implementation file directly
fd "rai_state_logs_node.py" -x cat {}
Length of output: 631
Script:
#!/bin/bash
# Let's examine the C++ implementation since we found it:
# 1. Get the full context of filter handling in the node
rg -A 5 -B 5 "filters_" src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp
# 2. Look for parameter declarations in the header file
fd "rai_state_logs_node.hpp" -x cat {}
# 3. Check for parameter declarations in CMake
rg "declare_parameter.*filter" src/state_tools/rai_state_logs/
Length of output: 1902
src/rai_bringup/launch/sim_whoami_demo.launch.py (3)
1-26: LGTM! Well-structured imports and proper license header.
The file starts with a proper Apache 2.0 license header and includes all necessary imports for ROS 2 launch configuration.
49-56: LGTM! Proper integration of rai_state_logs.
The inclusion of rai_state_logs launch file before rai_whoami aligns with the PR objectives for integrating state-based logging.
34-79: Verify integration with other example launch files.
The PR objectives mention adding rai_state_logs to Rai examples. Let's verify this integration is consistent across all example launch files.
✅ Verification successful
Integration with examples is properly structured
The example launch file src/examples/turtlebot4/turtlebot.launch.py includes the sim_whoami_demo.launch.py from rai_bringup package, which we can see from the original review contains the rai_state_logs integration. This is the correct approach as it maintains a clean hierarchy where examples reuse the core launch files rather than duplicating the rai_state_logs integration in each example.
- The example launch file properly includes
sim_whoami_demo.launch.pywhich contains therai_state_logsintegration - The launch arguments are correctly passed through to configure the example-specific paths and parameters
- This structure ensures consistent integration of
rai_state_logsacross examples through the shared launch file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if rai_state_logs is properly integrated in other example launch files
# Find all launch files in examples directory
echo "Checking launch files in examples directory:"
fd -e launch.py . src/examples -x grep -l "rai_state_logs"
# Check if they properly include the state logs launch file
echo "\nChecking for proper integration of rai_state_logs.launch.py:"
fd -e launch.py . src/examples -x grep -A 5 "rai_state_logs"
Length of output: 353
Script:
#!/bin/bash
# Let's first check what example launch files exist
echo "Finding all launch files in examples directory:"
fd -e launch.py . src/examples
# Let's also check the content of these launch files to see their structure
echo -e "\nChecking content of launch files:"
fd -e launch.py . src/examples -x cat
Length of output: 2387
examples/rosbot-xl-demo.py (3)
14-16: LGTM: Clean implementation of command-line argument handling
The argument parser implementation and imports are well-structured and follow Python best practices.
Also applies to: 39-40
141-142: LGTM: Clean script entry point implementation
The argument parsing and main function invocation are well-implemented.
117-117: Verify the impact of removing observe_topics
The removal of observe_topics and observe_postprocessors (set to None) needs verification to ensure it doesn't affect the node's functionality.
✅ Verification successful
Setting observe_topics to None is safe and intentional
The removal of hardcoded observe_topics and observe_postprocessors is safe because:
- The RaiStateBasedLlmNode class properly handles None values by defaulting to empty list/dict
- Other examples in the codebase (turtlebot_demo.py) also use None for these parameters
- The functionality is being replaced by the allowlist parameter, which is a more controlled approach to topic access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of observe_topics and observe_postprocessors
rg -A 5 "observe_topics|observe_postprocessors" --type python
Length of output: 94
Script:
#!/bin/bash
# Search for usages of observe_topics and observe_postprocessors without file type restriction
rg -A 5 "observe_topics|observe_postprocessors"
Length of output: 3597
src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp (1)
45-50: Verify service interface compatibility.
Since this node is being integrated with RaiStateBasedLlmNode, let's verify the service interface compatibility.
✅ Verification successful
Service interface is compatible and properly implemented
The service interface is correctly implemented and integrated:
- Service definition (
StringList.srv) uses standard empty request and string array response - C++ node properly implements the service callback with correct response population
- Python client correctly creates and uses the service with matching interface
- Both components use the same service name
/get_log_digest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify service interface compatibility between this node and RaiStateBasedLlmNode
# Search for service usage in the codebase
rg -A 5 "get_log_digest"
# Search for StringList service message usage
rg -A 5 "rai_interfaces/srv/StringList"
Length of output: 937
Script:
#!/bin/bash
# Let's verify the service interface by checking:
# 1. The service definition
# 2. The callback implementation
# 3. The client usage
# Find the service definition
fd -e srv StringList
# Check the callback implementation
ast-grep --pattern 'digest_request_callback($$$) {
$$$
}'
# Check the client usage in Python
rg -A 10 "rai_state_logs_client = node.create_client"
Length of output: 1005
Script:
#!/bin/bash
# Let's check the service definition and the C++ implementation
# Check the service definition content
cat src/rai_interfaces/srv/StringList.srv
# Check the complete implementation of the service callback
rg -B 5 -A 15 "digest_request_callback" src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp
Length of output: 1810
src/rai/rai/node.py (1)
580-580: Handle exceptions in summarize_logs to prevent disruptions
The call to summarize_logs() could raise an exception and interrupt the state_update_callback execution.
src/rai/rai/utils/ros_logs.py (2)
46-48: Correct the error message to reflect actual parameters
The error message mentions qos_profile, which is not a parameter of the function. It should be updated to reflect the actual parameters to avoid confusion.
Apply this diff to fix the error message:
if any([v is None for v in [llm, callback_group, bufsize]]):
raise ValueError(
- "Must provide llm, callback_group, qos_profile, and bufsize"
+ "Must provide llm, callback_group, and bufsize"
)156-157: Add error handling for LLM invocation in summarize method
Currently, the code assumes that self.llm.invoke will always succeed. It's advisable to add error handling to manage exceptions and cases where the LLM invocation fails, ensuring the node remains robust.
Apply this diff to include error handling:
self.clear()
- response = self.llm.invoke({"rosout": buffer})
+ try:
+ response = self.llm.invoke({"rosout": buffer})
+ except Exception as e:
+ self.node.get_logger().error(f"LLM invocation failed: {e}")
+ return "Error during log summarization"
return str(response.content)| rclpy.spin_until_future_complete(self.node, future) | ||
| response: Optional[rai_interfaces.srv.StringList.Response] = future.result() | ||
| if response is None or not response.success: | ||
| self.node.get_logger().error(f"'{self.SERVICE_NAME}' service call failed") | ||
| return "" |
There was a problem hiding this comment.
Add exception handling for potential service call failures
When calling future.result(), exceptions can occur if the service call fails or times out. Adding error handling ensures that the node remains robust and can handle such scenarios gracefully.
Apply this diff to include exception handling:
future = self.rai_state_logs_client.call_async(request)
- rclpy.spin_until_future_complete(self.node, future)
- response: Optional[rai_interfaces.srv.StringList.Response] = future.result()
+ try:
+ rclpy.spin_until_future_complete(self.node, future)
+ response: Optional[rai_interfaces.srv.StringList.Response] = future.result()
+ except Exception as e:
+ self.node.get_logger().error(f"Service call failed with exception: {e}")
+ return ""
if response is None or not response.success:
self.node.get_logger().error(f"'{self.SERVICE_NAME}' service call failed")
return ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rclpy.spin_until_future_complete(self.node, future) | |
| response: Optional[rai_interfaces.srv.StringList.Response] = future.result() | |
| if response is None or not response.success: | |
| self.node.get_logger().error(f"'{self.SERVICE_NAME}' service call failed") | |
| return "" | |
| try: | |
| rclpy.spin_until_future_complete(self.node, future) | |
| response: Optional[rai_interfaces.srv.StringList.Response] = future.result() | |
| except Exception as e: | |
| self.node.get_logger().error(f"Service call failed with exception: {e}") | |
| return "" | |
| if response is None or not response.success: | |
| self.node.get_logger().error(f"'{self.SERVICE_NAME}' service call failed") | |
| return "" |
boczekbartek
changed the base branch from
development
to
bb/refactor/xml_launch_files_to_python
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (15)
src/state_tools/rai_state_logs/launch/rai_state_logs.launch.py (1)
44-46: Consider enhancing the filter documentation.While the comment explains that no filters will be applied, it would be helpful to add more context about:
- The expected filter format for when users want to add actual filters
- Why
'[""]'is used instead of'[]'for representing no filtersConsider updating the comment like this:
- default_value='[""]', # this means that no filters will be applied + default_value='[""]', # Empty string in list means no filters. For filtering, use ["filter1", "filter2"]examples/rosbot-xl-demo.py (3)
43-43: Enhance error handling and logging messagesWhile the error handling is present, it could be improved for better debugging and user experience.
Consider these improvements:
ros2_allowlist = [] if allowlist is not None: try: content = allowlist.read_text().strip() if content: ros2_allowlist = content.splitlines() else: rclpy.logging.get_logger("rosbot_xl_demo").warning( - "Allowlist file is empty" + f"Allowlist file '{allowlist}' is empty. No filters will be applied." ) - except Exception as e: + except FileNotFoundError as e: rclpy.logging.get_logger("rosbot_xl_demo").error( - f"Failed to read allowlist: {e}" + f"Allowlist file '{allowlist}' not found: {e}" + ) + except PermissionError as e: + rclpy.logging.get_logger("rosbot_xl_demo").error( + f"Permission denied reading allowlist file '{allowlist}': {e}" + ) + except Exception as e: + rclpy.logging.get_logger("rosbot_xl_demo").error( + f"Unexpected error reading allowlist file '{allowlist}': {e}" )Also applies to: 50-63
141-142: Consider adding runtime type validationWhile the implementation is correct, consider adding runtime type validation for the parsed arguments to ensure robustness.
if __name__ == "__main__": args = p.parse_args() + if args.allowlist is not None and not isinstance(args.allowlist, Path): + raise TypeError(f"Expected Path for allowlist, got {type(args.allowlist)}") main(**vars(args))
Line range hint
65-115: Consider externalizing configuration dataThe SYSTEM_PROMPT contains hardcoded coordinates and rules. Consider moving this configuration to a separate YAML or JSON file for better maintainability and reusability.
This would:
- Make it easier to update coordinates without changing code
- Allow different configurations for different environments
- Improve separation of concerns
src/state_tools/rai_state_logs/src/rai_state_logs_node.cpp (2)
45-50: LGTM! Consider adding defensive programming and tracking the hack.The hack appropriately addresses the ROS 2 issue #1955 with empty parameter arrays. However, to improve maintainability:
- Consider adding a TODO comment to track when this can be removed (after the ROS 2 issue is fixed).
- Add defensive programming to handle potential future parameter changes.
- // Hack to overcome https://github.com/ros2/rclcpp/issues/1955 + // TODO(cleanup): Remove this hack once https://github.com/ros2/rclcpp/issues/1955 is fixed + // Hack: Handle empty parameter arrays in ROS 2 if (filters_.size() == 1 && filters_[0].empty()) { filters_.clear(); } + + // Defensive check for invalid filter configurations + for (const auto& filter : filters_) { + if (filter.empty()) { + RCLCPP_WARN(get_logger(), "Empty filter detected in configuration"); + } + }
Line range hint
1-190: Consider architectural improvements for robustness and performance.While the overall implementation is solid, here are some suggestions to enhance the code:
Thread Safety:
- Add mutex protection for shared data structures (
logs_andtimestamps_) accessed from both the callback and service.- Consider using lock-free data structures or a producer-consumer pattern.
Performance Optimizations:
- Use string_view for filter operations to avoid copies
- Pre-reserve space in vectors and strings
- Consider a circular buffer for the rolling window
Memory Management:
- Add memory limits for log messages
- Consider using a more efficient data structure for the rolling window
Here's an example of adding thread safety:
class LogDigestNode : public rclcpp::Node { private: // Add mutex for thread safety std::mutex mutex_; void digest_request_callback(...) { std::lock_guard<std::mutex> lock(mutex_); // ... existing code ... } void log_callback(const rcl_interfaces::msg::Log & msg) { std::lock_guard<std::mutex> lock(mutex_); // ... existing code ... } };And an example of performance optimization:
bool passesFilters(const std::string_view text) const { return std::any_of( filters_.cbegin(), filters_.cend(), [&text](const std::string& f) { return text.find(f) != std::string_view::npos; }); }src/rai/rai/node.py (2)
348-348: Add parameter documentation.The new
logs_parser_typeparameter is well-defined with appropriate typing. Consider adding docstring documentation to explain the available options and their implications.def __init__( self, system_prompt: str, observe_topics: Optional[List[str]] = None, observe_postprocessors: Optional[Dict[str, Callable[[Any], Any]]] = None, allowlist: Optional[List[str]] = None, tools: Optional[List[Type[BaseTool]]] = None, logs_parser_type: Literal["llm", "rai_state_logs"] = "rai_state_logs", + """ + Args: + logs_parser_type: Type of logs parser to use. + - "llm": Uses LLM-based parsing of ROS logs + - "rai_state_logs": Uses the rai_state_logs service (default) + """
580-584: Remove duplicated error handling.The error handling here duplicates what should be in the
summarize_logsmethod. Consider removing this try-except block and lettingsummarize_logshandle the errors.ts = time.perf_counter() - try: - state_dict["logs_summary"] = self.summarize_logs() - except Exception as e: - self.get_logger().error(f"Error summarizing logs: {e}") - state_dict["logs_summary"] = "" + state_dict["logs_summary"] = self.summarize_logs() te = time.perf_counter() - tssrc/rai/rai/utils/ros.py (4)
Line range hint
25-25: Correct the default value and handle 'allowlist' consistencyCurrently,
allowlistis typed asOptional[List[str]]but is defaulted to an empty list usingfield(default_factory=list). This meansallowlistwill never beNoneunless explicitly set, and the conditionif self.allowlist is not Nonein thesetmethod will always beTrue. This could lead to unintended behavior whenallowlistis empty.Apply this diff to fix the inconsistency:
- allowlist: Optional[List[str]] = field(default_factory=list) + allowlist: Optional[List[str]] = NoneAlso, update the condition in the
setmethod to properly check ifallowlistis provided:self.actions_and_types = to_dict(actions) - if self.allowlist is not None: + if self.allowlist: self.__filter(self.allowlist)
Line range hint
28-29: Handle potential empty lists in theto_dictfunctionThe
to_dictfunction assumes thatv[0]exists, but ifvis an empty list, this will raise anIndexError. To ensure robustness, consider adding a check or providing a default value whenvis empty.Apply this diff to handle potential empty lists safely:
def to_dict(info: List[Tuple[str, List[str]]]) -> Dict[str, str]: - return {k: v[0] for k, v in info} + return {k: v[0] if v else None for k, v in info}Alternatively, you might want to filter out entries where
vis empty:def to_dict(info: List[Tuple[str, List[str]]]) -> Dict[str, str]: - return {k: v[0] for k, v in info} + return {k: v[0] for k, v in info if v}
Line range hint
47-47: Avoid using built-in names for thedictmethodThe method name
dictshadows the built-indicttype, which can lead to confusion or unexpected behavior. It's a best practice to avoid using names that conflict with built-in types.Apply this diff to rename the method for clarity:
- def dict(self): + def to_dict(self):Ensure that any calls to this method are also updated accordingly.
Line range hint
21-52: Add docstrings to the class and methods for better maintainabilityProviding docstrings enhances code readability and helps others understand the purpose and usage of the class and its methods. This is important for future maintenance and collaboration.
Apply this diff to add docstrings:
@dataclass class NodeDiscovery: + """Class for discovering and managing ROS nodes' topics, services, and actions.""" topics_and_types: Dict[str, str] = field(default_factory=dict) services_and_types: Dict[str, str] = field(default_factory=dict) actions_and_types: Dict[str, str] = field(default_factory=dict) allowlist: Optional[List[str]] = None def set(self, topics, services, actions): + """ + Set the topics, services, and actions from the provided lists. + + Args: + topics (List[Tuple[str, List[str]]]): List of topic names and types. + services (List[Tuple[str, List[str]]]): List of service names and types. + actions (List[Tuple[str, List[str]]]): List of action names and types. + """ def to_dict(info: List[Tuple[str, List[str]]]) -> Dict[str, str]: return {k: v[0] for k, v in info} self.topics_and_types = to_dict(topics) self.services_and_types = to_dict(services) self.actions_and_types = to_dict(actions) if self.allowlist: self.__filter(self.allowlist) def __filter(self, allowlist: List[str]): + """ + Filter the topics, services, and actions based on the allowlist. + + Args: + allowlist (List[str]): List of allowed names. + """ for d in [ self.topics_and_types, self.services_and_types, self.actions_and_types, ]: to_remove = [k for k in d if k not in allowlist] for k in to_remove: d.pop(k) def to_dict(self): + """ + Return a dictionary representation of the NodeDiscovery instance. + + Returns: + Dict[str, Dict[str, str]]: A dictionary containing topics, services, and actions. + """ return { "topics_and_types": self.topics_and_types, "services_and_types": self.services_and_types, "actions_and_types": self.actions_and_types, }src/rai/rai/utils/ros_logs.py (3)
83-84: Fix typo in class docstringThere's a typo in the docstring of the
LlmRosoutParserclass. It should read "summarize it with LLM" instead of "summarize is with LLM".Apply this diff to correct the typo:
class LlmRosoutParser(BaseLogsParser): - """Bufferize `/rosout` and summarize is with LLM""" + """Bufferize `/rosout` and summarize it with LLM"""
133-134: Consider configuring the node name filterThe current implementation filters out log messages from nodes named
"rai_node". If there are multiple nodes or if the node name changes, this hardcoded filter may not be sufficient.Consider making the node name to filter configurable or more flexible. For example:
def rosout_callback(self, msg: rcl_interfaces.msg.Log): self.node.get_logger().debug(f"Received rosout: {msg}") - if "rai_node" in msg.name: + if msg.name == self.node.get_name(): return
136-136: Include nanoseconds in timestamp for higher precisionCurrently, the timestamp only includes seconds via
msg.stamp.sec. Including nanoseconds will provide higher precision, which might be important for log analysis.Apply this diff to include nanoseconds:
- self.append(f"[{msg.stamp.sec}][{msg.name}]:{msg.msg}") + timestamp = msg.stamp.sec + msg.stamp.nanosec * 1e-9 + self.append(f"[{timestamp:.6f}][{msg.name}]:{msg.msg}")
🛑 Comments failed to post (3)
examples/rosbot-xl_allowlist.txt (2)
1-22: 🛠️ Refactor suggestion
Consider adding essential ROS 2 topics
The allowlist is missing some essential ROS 2 topics that are typically required for proper system operation:
/parameter_events: Required for dynamic parameter updates/tfand/tf_static: Required for coordinate transformations/robot_description: Required for robot model information
1-22: 💡 Codebase verification
Based on the gathered information, I can now provide a final response:
Security measures need to be implemented for exposed topics
The codebase shows:
- QoS profiles are configured but primarily for reliability/durability, not security
- Velocity limits and collision detection are implemented but focused on safety, not security
- No authentication/authorization mechanisms found for topic access control
Required actions:
- Implement ROS2 Security Enclaves (DDS Security) to control topic access
- Add rate limiting for camera topics (currently missing in QoS configurations)
- Configure authentication for critical control topics (
/cmd_vel,/dock_robot)- Document security configurations in deployment guide
🔗 Analysis chain
Review security implications of exposed topics
The allowlist exposes critical control topics (
/cmd_vel,/dock_robot, etc.) and sensor data. Please ensure:
- Access control mechanisms are in place
- Input validation is implemented for control commands
- Rate limiting is configured for high-bandwidth topics (camera feeds)
Let me gather more specific information about security mechanisms in the ROS2 configuration and launch files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for security-related code echo "Checking for access control mechanisms..." rg -l "security|authentication|authorization" echo "Checking for input validation..." ast-grep --pattern 'if ($_ < $_) { $$$ }' # Basic range checks ast-grep --pattern 'try { $$$ } catch ($$$) { $$$ }' # Error handling echo "Checking for rate limiting..." rg -l "rate|throttle|limit"Length of output: 7489
Script:
#!/bin/bash # Check for ROS2 security configurations in launch files echo "Checking launch files for security configurations..." rg -l "security|qos|reliability|durability" --type xml --type python # Check for QoS settings that might include rate limiting echo "Checking for QoS profile configurations..." rg "QoSProfile|ReliabilityPolicy|DurabilityPolicy" -A 3 # Check for any validation in the nodes that subscribe to critical topics echo "Checking nodes handling critical topics..." rg -l "cmd_vel|dock_robot" --type cpp --type python # Look for ROS2 node configurations echo "Checking for node configurations..." fd -e yaml -x cat {} \;Length of output: 45317
src/examples/turtlebot4/turtlebot.launch.py (1)
41-52: 💡 Codebase verification
Security concerns partially validated, path handling needs improvement
The
run-nav.bashscript content has been verified and appears to be safe:
- It only launches ROS2 navigation stack with configuration parameters
- Uses environment variables appropriately
- No suspicious or dangerous operations detected
However, the path handling concerns remain valid:
- Both the script and RViz config are referenced using relative paths
- No package.xml was found in the turtlebot4 directory or parent examples directory
- This indicates these files are not properly integrated into a ROS2 package structure
The original recommendation to use
FindPackageShareremains valid, but needs to be adjusted since these files are not part of a proper ROS2 package. You should either:
- Move these files into a proper ROS2 package structure
- Use absolute paths resolved from a well-defined environment variable (e.g.,
TURTLEBOT_CONFIG_DIR)🔗 Analysis chain
Security and robustness improvements needed
- The bash script execution could pose security risks and the relative path might be problematic:
- Consider validating the script exists before execution
- Use FindPackageShare for the script path
- RViz configuration also uses a relative path that should be made more robust
Consider these improvements:
ExecuteProcess( - cmd=["bash", "run-nav.bash"], - cwd="src/examples/turtlebot4", + cmd=["bash", [FindPackageShare("your_package"), "/examples/turtlebot4/run-nav.bash"]], output="screen", ), Node( package="rviz2", executable="rviz2", name="rviz2", output="screen", - arguments=["-d", "src/examples/turtlebot4/config.rviz"], + arguments=["-d", [FindPackageShare("your_package"), "/examples/turtlebot4/config.rviz"]], ),Let's verify the bash script content for security:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check bash script content for potential security issues echo "Checking run-nav.bash content..." fd -t f "run-nav.bash" -p "examples/turtlebot4" -x cat {}Length of output: 239
Script:
#!/bin/bash # Let's first find the correct path by searching for the file echo "Finding the run-nav.bash file..." fd -t f "run-nav.bash" # Let's also check for the rviz config file echo -e "\nFinding the rviz config file..." fd -t f "config.rviz" # Let's check if we're in a ROS2 package by looking for package.xml echo -e "\nChecking for package.xml..." fd -t f "package.xml" -x cat {}Length of output: 8521
Script:
#!/bin/bash # Let's check the content of run-nav.bash echo "Checking run-nav.bash content..." cat src/examples/turtlebot4/run-nav.bash # Let's also check if this is part of a ROS package by looking for package.xml in the turtlebot4 directory echo -e "\nChecking for package.xml in turtlebot4 directory..." fd -t f "package.xml" -p "src/examples/turtlebot4" # Let's check the parent directory for package information echo -e "\nChecking for package.xml in parent directory..." fd -t f "package.xml" -p "src/examples"Length of output: 887
- the reason is that it can't be empty in launch files that inherit from this launchfile which is not conventient
- because when current rai_state_logs launch file is included in other launches they say that `filters` param is required, even though it is not required by the base launch. Linked issue: ros2/rclcpp#1955
boczekbartek
force-pushed
the
bb/feat/integrate_state_logs_with_agent
branch
from
9dddf36 to
9e42307
Compare
|
I've just resolved conflicts and rebased on |
There was a problem hiding this comment.
Works as intended. Please add
"Do not use cmd_vel topic to navigate.- use appropriate ros2 actions instead." to the demo's system prompt.
I just can't look at the poor LLM trying to navigate using cmd vel...
Edit: This might not be the most suitable PR for that. Let's do it in the next one (update system prompts to enhance multiple demos at once)
For diff base of this PR is #324 -> merge after it.
Purpose
rai_state_logshas been a separate module not used by RaiStateBasedLlmNode andraiexamplesProposed Changes
rai_state_logsorllm) can be set asRaiStateBasedLlmNodeparameter.rai_state_logsas a default logs parserrai_state_logsto rai examplesfilterslist was added due to: Make it possible to use empty lists/arrays as parameter values everywhere ros2/rclcpp#1955Related PRs and issues that should be resolved before merging this PR:
rai_state_logs#322rai_state_logswill be on by default after thie PR is merged, if this issue is not resoled, examples will not work: change XML launch files to python examples #323Issues
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor