398 featureexperiment proper vscode debugging #478
Conversation
Zelberor
added
enhancement
WalkthroughThis pull request introduces several enhancements aimed at improving debugging capabilities for ROS nodes using the Changes
Assessment against linked issues
Possibly related PRs
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (52)
.vscode/launch.json (1)
1-23: Well-structured debugging configuration.The launch configuration effectively supports the PR's goal of enabling VS Code debugging for ROS nodes. A few architectural notes:
- The port range 53000-53100 is reserved for debugging, allowing multiple debug sessions
- The setup enables remote debugging through debugpy
- The path mapping structure supports proper source mapping between local and containerized environments
Consider documenting these debugging ports in the project's documentation to prevent port conflicts with other services.
🧰 Tools
🪛 Biome
[error] 22-23: Expected a property but instead found '}'.
Expected a property here.
(parse)
build/docker-compose.leaderboard.yaml (1)
14-18: Enhance command robustness and readability.While the command structure works, it could benefit from several improvements:
Consider applying these enhancements:
command: |- - bash -c "sleep 10 && sudo chown -R ${USER_UID}:${USER_GID} ../ && \ - sudo chmod -R a+w ../ && sudo mkdir -p $${XDG_RUNTIME_DIR} && sudo chmod 0700 $${XDG_RUNTIME_DIR} && sudo chown -R ${USER_UID}:${USER_GID} $${XDG_RUNTIME_DIR} && \ - (rqt_console &) && disown -a && \ - python3 /opt/leaderboard/leaderboard/leaderboard_evaluator.py --debug=0 --routes=$${ROUTE} --agent=/workspace/code/agent/src/agent/agent.py --host=$${CARLA_SIM_HOST} --track=MAP" + bash -c ' + # Ensure required environment variables are set + : "${XDG_RUNTIME_DIR:?Required}" "${USER_UID:?Required}" "${USER_GID:?Required}" + + # Wait for dependent services + sleep 10 + + # Setup permissions + sudo chown -R ${USER_UID}:${USER_GID} ../ || exit 1 + sudo chmod -R a+w ../ || exit 1 + + # Setup runtime directory + sudo mkdir -p ${XDG_RUNTIME_DIR} || exit 1 + sudo chmod 0700 ${XDG_RUNTIME_DIR} || exit 1 + sudo chown -R ${USER_UID}:${USER_GID} ${XDG_RUNTIME_DIR} || exit 1 + + # Launch rqt_console in background + nohup rqt_console > /dev/null 2>&1 & + + # Launch main application + exec python3 /opt/leaderboard/leaderboard/leaderboard_evaluator.py \ + --debug=0 \ + --routes=${ROUTE} \ + --agent=/workspace/code/agent/src/agent/agent.py \ + --host=${CARLA_SIM_HOST} \ + --track=MAP + 'Improvements:
- Added environment variable validation
- Improved error handling with
|| exit 1- Used
nohupfor better background process management- Used
execto replace shell with the main process- Improved readability with comments and formatting
code/agent/launch/agent.launch (2)
24-27: Consider adding configuration parameters for debugging.The debugging integration looks good, but consider making it more configurable by:
- Adding an argument to enable/disable debugging
- Allowing port configuration to be passed through
Consider this enhancement:
+ <arg name="enable_debugging" default="false" /> + <arg name="debug_port" default="53000" /> + <!-- debugging --> - <include file="$(find debugging)/launch/debugging.launch"> + <group if="$(arg enable_debugging)"> + <include file="$(find debugging)/launch/debugging.launch"> + <arg name="port" value="$(arg debug_port)" /> + </include> + </group>
24-24: Add documentation for the debugging section.Consider adding a brief comment explaining the purpose of the debugging section and how to use it.
- <!-- debugging --> + <!-- debugging: Enables VS Code debugging support for ROS nodes + Use with .vscode/launch.json configuration "53000 leaderboard attach" + -->build/agent_service.yaml (1)
33-33: Document debug configuration usage and security implications.Consider adding documentation that:
- Explains the purpose of
DEBUG_WRAPPER_DEFAULT_HOST- Provides guidelines for different environments (development vs. production)
- Lists security considerations when exposing debug interfaces
Would you like me to help create this documentation or open a GitHub issue to track this task?
code/perception/launch/perception.launch (1)
Line range hint
78-82: Consider aligning node configuration with existing patternsThe new lidar_distance node configuration could be improved to maintain consistency with other nodes in this launch file:
- Add role_name parameter to support multiple vehicle scenarios
- Consider adding control_loop_rate if the node processes data periodically
- Parameterize topic names using $(arg role_name)
Here's the suggested implementation:
<node pkg="perception" type="lidar_distance.py" name="lidar_distance" output="screen"> + <param name="role_name" value="$(arg role_name)" /> + <param name="control_loop_rate" value="$(arg control_loop_rate)" /> - <param name="point_cloud_topic" value="/carla/hero/LIDAR_filtered"/> - <param name="range_topic" value="/carla/hero/LIDAR_range"/> + <param name="point_cloud_topic" value="/carla/$(arg role_name)/LIDAR_filtered"/> + <param name="range_topic" value="/carla/$(arg role_name)/LIDAR_range"/> </node>code/acting/src/debug_wrapper.py (4)
Line range hint
52-71: Optimize logger connection handling.The current implementation creates and closes a connection for each log message, which could be inefficient under high logging load. Consider these improvements:
- Add a small delay between connection attempts
- Consider maintaining a persistent connection
- Make the port configurable through environment variables
def log(msg: str, level: str): error = None success = False start_time = time.monotonic() + logger_port = int(os.getenv('DEBUG_LOGGER_PORT', '52999')) while not success and start_time + 5.0 > time.monotonic(): try: - address = ("localhost", 52999) + address = ("localhost", logger_port) conn = Client(address, authkey=b"debug_logger") conn.send({"name": NODE_NAME, "msg": msg, "level": level}) conn.close() success = True + time.sleep(0.1) # Add delay between retries except BaseException as e: error = e
Line range hint
89-98: Improve module path handling and validation.The current implementation modifies sys.path without cleanup and lacks path validation. This could lead to:
- sys.path pollution
- Unclear errors if the module doesn't exist
def run_module_at(path: str): + if not os.path.exists(path): + raise FileNotFoundError(f"Module not found at path: {path}") + basename = os.path.basename(path) module_dir = os.path.dirname(path) module_name = os.path.splitext(basename)[0] + original_path = sys.path[:] sys.path.append(module_dir) - runpy.run_module(module_name, run_name="__main__") + try: + runpy.run_module(module_name, run_name="__main__") + finally: + sys.path[:] = original_path
Line range hint
101-127: Complete docstring and improve error handling.The function has an incomplete docstring and could benefit from more specific error handling.
def start_debugger( node_module_name: str, host: str, port: int, wait_for_client: bool = False ): - """_summary_ + """Start a debugpy debugging session for the node. + + Attempts to start a debugpy debugging session on the specified host and port. + If debugpy is not available, logs an error but allows the node to continue. Args: node_module_name (str): Name of the underlying node. Only used for logging host (str): host the debugger binds to port (int): debugger port wait_for_client (bool, optional): If the debugger should wait for a client to attach. Defaults to False. + + Raises: + debugpy.server.api.ServerError: If the debugger fails to start + (e.g., port already in use) """ debugger_spec = importlib.util.find_spec("debugpy") if debugger_spec is not None: try: import debugpy debugpy.listen((host, port)) logwarn(f"Started debugger on {host}:{port} for {node_module_name}") if wait_for_client: debugpy.wait_for_client() - except BaseException as error: + except Exception as error: # Yes, all exceptions should be catched and sent into rosconsole logerr(f"Failed to start debugger: {error}") else: logerr("debugpy module not found. Unable to start debugger")
Global NODE_NAME usage needs refactoring, but base directory resolution is correct
The base directory resolution using
os.path.abspath(os.path.dirname(__file__))is a standard and correct approach. However, the globalNODE_NAMEusage is problematic as it:
- Is initialized with a default "NAMEERR" value
- Gets modified in the main execution
- Is used for logging messages through socket connections
Suggested improvements:
- Replace global
NODE_NAMEwith a class-based logger or context manager- Pass node name as parameter to logging functions instead of relying on global state
- Keep the current base directory resolution as it correctly uses
__file__Example refactor:
class DebugLogger: def __init__(self, node_name: str): self.node_name = node_name def send_log(self, conn, msg: str, level: str): conn.send({"name": self.node_name, "msg": msg, "level": level})🔗 Analysis chain
Line range hint
159-186: Verify base directory resolution and improve global state handling.The current implementation uses a global variable and might have path resolution issues.
Consider these improvements:
- Pass NODE_NAME as a parameter instead of using a global variable
- Verify base_dir resolution works with different launch configurations
- Add logging for the actual resolved path
- global NODE_NAME - NODE_NAME = debug_node + def get_logger_name(debug_node: str) -> str: + return debug_node + + node_name = get_logger_name(debug_node) base_dir = os.path.abspath(os.path.dirname(__file__)) + loginfo(f"Resolved base directory: {base_dir}") if args.debug_port is not None: start_debugger(🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the base directory resolution works across different launch configurations fd --type f --extension launch . | xargs grep -l "debug_wrapper.py"Length of output: 67
Script:
#!/bin/bash # Let's verify how the debug_wrapper.py is being used and imported rg -l "debug_wrapper" # Check how NODE_NAME is used across the codebase rg "NODE_NAME" -A 2 # Check for launch files with different extensions fd -e xml -e launch -e yaml . | xargs grep -l "debug_wrapper" # Look for the actual debug_wrapper.py implementation to understand the NODE_NAME usage ast-grep --pattern 'NODE_NAME = $_'Length of output: 807
doc/development/debugging.md (4)
23-28: Add information about configuring log levels.The documentation should mention how to configure the logging level (e.g., through
rosparamor environment variables) as this is crucial for controlling which messages are actually published.Consider adding:
The logging level can be configured through: - The `ROSPY_LOG_LEVEL` environment variable - The `rospy.set_param('/rosout_level', 'DEBUG')` call - The `log_level` parameter in the launch file🧰 Tools
🪛 LanguageTool
[formatting] ~26-~26: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ic.rospy.logdebugis not recommended, because these messages are not published to /ro...(COMMA_BEFORE_BECAUSE)
21-29: Add code examples for logging functions.To make the documentation more practical, consider adding simple code examples demonstrating each logging function.
Consider adding:
Example usage: ```python import rospy # Initialize the node first rospy.init_node('my_node') # Then use logging functions rospy.loginfo("Node initialized successfully") rospy.logwarn("Warning: approaching resource limit") rospy.logerror("Error: failed to process data")🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: This expression is usually spelled with a hyphen.
Context: ...s: Message based and VS Code based ### Message based debugging Messages can be logged into ...(BASED_HYPHEN)
[formatting] ~26-~26: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ic.rospy.logdebugis not recommended, because these messages are not published to /ro...(COMMA_BEFORE_BECAUSE)
126-132: Add validation steps for Docker rebuild.Consider adding steps to verify successful container rebuild and configuration.
Add:
### Verifying the Setup After rebuilding, verify the setup with: ```bash # Verify containers are running with new images docker compose -f ./docker-compose.leaderboard.yaml ps docker compose -f ./docker-compose.dev.yaml ps # Verify debug port exposure docker compose -f ./docker-compose.leaderboard.yaml port build-agent 53000
134-136: Expand documentation sources.Consider adding more relevant documentation sources for a comprehensive reference.
Add:
## Sources - [ROS Logging](https://wiki.ros.org/rospy/Overview/Logging) - [VS Code Python Debugging](https://code.visualstudio.com/docs/python/debugging) - [debugpy Documentation](https://github.com/microsoft/debugpy) - [ROS Launch File XML Format](http://wiki.ros.org/roslaunch/XML)code/debugging/CMakeLists.txt (1)
103-108: Consider declaring explicit catkin dependencies.While the current configuration works, it's recommended to explicitly declare your catkin dependencies for better clarity:
catkin_package( -# INCLUDE_DIRS include -# LIBRARIES debugging -# CATKIN_DEPENDS rospy -# DEPENDS system_lib + CATKIN_DEPENDS rospy )code/debugging/src/debug_logger.py (5)
83-83: Useeprintinstead ofTo ensure that log messages are consistently captured in
agent.log, useeprintinstead ofApply this diff to replace
eprint:- print(f"[debug_logger]: connection accepted from {listener.last_accepted}") + eprint(f"connection accepted from {listener.last_accepted}")
87-88: Simplify nestedifstatements for readabilityCombine the nested
ifstatements into a single condition usingandto improve readability.Apply this diff to simplify the condition:
- if isinstance(msg, str): - if msg.lower() == CLOSE_MSG: + if isinstance(msg, str) and msg.lower() == CLOSE_MSG:🧰 Tools
🪛 Ruff
87-88: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
124-124: Userospy.signal_shutdowninstead ofexitfor graceful terminationIn a ROS node, it's better to use
rospy.signal_shutdownto gracefully shut down the node rather than callingexit(0).Apply this diff to use
rospy.signal_shutdown:- exit(0) + rospy.signal_shutdown("Failed to run listener")
131-133: Avoid using fixed sleep duration for node initializationUsing
time.sleep(5.0)to wait for node initialization can be unreliable and may introduce unnecessary delays. Consider using ROS methods to ensure the node is fully initialized or remove the sleep if it's not essential.For example, you might replace it with
rospy.sleep(0.1)in a loop until certain conditions are met.
69-70: Handle unrecognized log levels appropriately to prevent unintended fatal logsDefaulting to
rospy.logfatalfor unrecognized log levels might lead to unintended fatal error logs. Consider defaulting to a safer log level, such as warning, and include a message indicating the unrecognized level.Apply this diff to handle unrecognized log levels:
- else: - rospy.logfatal(msg) + else: + rospy.logwarn(f"Unrecognized log level '{level}', logging as warning: {msg}")code/mock/src/debug_wrapper.py (8)
Line range hint
120-128: Incomplete docstring forstart_debuggerfunctionThe docstring for the
start_debuggerfunction is incomplete and currently contains"""_summary_""". Please provide a meaningful description of the function's purpose and behavior.Apply this diff to improve the docstring:
def start_debugger( node_module_name: str, host: str, port: int, wait_for_client: bool = False ): - """_summary_ + """Starts the debugpy debugger for remote debugging. Args: node_module_name (str): Name of the underlying node, used for logging. host (str): Host the debugger binds to. port (int): Port the debugger listens on. wait_for_client (bool, optional): Whether to wait for a client to attach before proceeding. Defaults to False. """
Line range hint
74-81: Avoid catchingBaseException; catchExceptioninsteadIn the
logfunction, catchingBaseExceptionis too broad and may intercept system-exiting exceptions likeKeyboardInterruptandSystemExit. It's better to catchExceptionto handle standard errors.Apply this diff to fix the issue:
while not success and start_time + 5.0 > time.monotonic(): try: address = ("localhost", 52999) conn = Client(address, authkey=b"debug_logger") conn.send({"name": NODE_NAME, "msg": msg, "level": level}) conn.close() success = True - except BaseException as e: + except Exception as e: error = e
Line range hint
138-140: Avoid catchingBaseException; catchExceptioninsteadIn the
start_debuggerfunction, catchingBaseExceptionis not recommended as it includes non-standard exceptions. CatchingExceptionsuffices for most error handling.Apply this diff to fix the issue:
try: import debugpy debugpy.listen((host, port)) logwarn(f"Started debugger on {host}:{port} for {node_module_name}") if wait_for_client: debugpy.wait_for_client() - except BaseException as error: + except Exception as error: # Yes, all exceptions should be caught and sent into rosconsole logerr(f"Failed to start debugger: {error}")
Line range hint
192-194: Avoid catchingBaseException; catchExceptioninsteadIn the
mainfunction, catchingBaseExceptionwhen running the node can suppress important exceptions. UseExceptionto catch standard errors without affecting system-exiting exceptions.Apply this diff to fix the issue:
try: run_module_at(target_type_path) - except BaseException as error: + except Exception as error: logfatal(f"Failed to run node {debug_node}: {error}") raise error
Line range hint
167-167: Incorrect use oftype=boolin argument parsingUsing
type=boolwithargparsemay not work as intended because any non-empty string is consideredTrue. Useaction='store_true'for boolean flags.Apply this diff to fix the issue:
-parser.add_argument("--debug_wait", default=False, type=bool) +parser.add_argument("--debug_wait", action='store_true', help="Wait for debugger client before starting the node")
Line range hint
113-113: Modifysys.pathusinginsert(0, ...)instead ofappend()Appending to
sys.pathmay not prioritize your module directory when importing. Useinsert(0, ...)to ensure your directory is searched first.Apply this diff to improve module loading:
- sys.path.append(module_dir) + sys.path.insert(0, module_dir)
Line range hint
51-60: Consider using theloggingmodule instead of customeprintUsing the built-in
loggingmodule provides greater flexibility and control over logging output, including log levels and handlers.
Line range hint
149-152: Override botherrorandexitmethods inThrowingArgumentParserCurrently, only the
errormethod is overridden. To fully prevent the parser from exiting the program on errors, also override theexitmethod.Apply this diff to enhance error handling:
class ThrowingArgumentParser(argparse.ArgumentParser): def error(self, message): logfatal(f"Wrong node arguments. Check launch config. : {message}") raise ArgumentParserError(message) + def exit(self, status=0, message=None): + if message: + logfatal(message) + raise ArgumentParserError(message)code/perception/src/debug_wrapper.py (8)
1-1: Add a shebang line for script executionConsider adding a shebang line at the top to allow the script to be executed directly.
Apply this diff:
+#!/usr/bin/env python3
Line range hint
26-30: CatchExceptioninstead ofBaseExceptioninlogfunctionCatching
BaseExceptioncan inadvertently catch system-exiting exceptions likeKeyboardInterruptandSystemExit. It's recommended to catchExceptioninstead.Apply this diff:
except BaseException as e: error = e + except Exception as e: + error = e
Line range hint
54-70: Add a delay between retries in the logging function to prevent high CPU usageThe retry loop in the
logfunction doesn't include a delay, which could lead to high CPU usage during the retry period. Consider adding a short sleep between retries.Apply this diff:
while not success and start_time + 5.0 > time.monotonic(): try: address = ("localhost", 52999) conn = Client(address, authkey=b"debug_logger") conn.send({"name": NODE_NAME, "msg": msg, "level": level}) conn.close() success = True except Exception as e: error = e + time.sleep(0.1)
Line range hint
90-100: Provide a meaningful docstring forstart_debuggerfunctionThe docstring currently contains
"""_summary_""", which is incomplete. Please provide a detailed description of the function.Apply this diff:
def start_debugger( node_module_name: str, host: str, port: int, wait_for_client: bool = False ): - """_summary_ + """Start the debugpy debugger. Initializes the debugpy debugger to listen on the specified host and port. Optionally waits for a client to attach before proceeding. Args: node_module_name (str): Name of the underlying node, used for logging. host (str): Host the debugger binds to. port (int): Debugger port. wait_for_client (bool, optional): If True, waits for a client to attach. Defaults to False. """
Line range hint
96-100: CatchExceptioninstead ofBaseExceptioninstart_debuggerCatching
BaseExceptioncan inadvertently catch system-exiting exceptions likeKeyboardInterruptandSystemExit. It's better to catchExceptionto handle errors appropriately.Apply this diff:
except BaseException as error: # Yes, all exceptions should be catched and sent into rosconsole logerr(f"Failed to start debugger: {error}") + except Exception as error: + # Handle general exceptions + logerr(f"Failed to start debugger: {error}")
Line range hint
81-85: Avoid modifyingsys.path; useimportlibto run modules from a file pathModifying
sys.pathcan have unintended side effects. Consider usingimportlib.utilto load and run modules from a specific file path.Apply this diff:
def run_module_at(path: str): """Runs a python module based on its file path Args: path (str): python file path to run """ - basename = os.path.basename(path) - module_dir = os.path.dirname(path) - module_name = os.path.splitext(basename)[0] - sys.path.append(module_dir) - runpy.run_module(module_name, run_name="__main__") + spec = importlib.util.spec_from_file_location("module.name", path) + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module)
Line range hint
114-116: Avoid using global variables; passNODE_NAMEas a parameterUsing global variables like
NODE_NAMEcan make the code harder to maintain and test. Consider passing it as an argument to the functions that require it.
Line range hint
145-147: CatchExceptioninstead ofBaseExceptioninmainfunctionCatching
BaseExceptionmay unintentionally catch exceptions likeKeyboardInterrupt. It's safer to catchExceptionto handle runtime errors properly.Apply this diff:
try: run_module_at(target_type_path) - except BaseException as error: + except Exception as error: logfatal(f"Failed to run node {debug_node}: {error}") raise errorcode/planning/src/debug_wrapper.py (7)
Line range hint
59-69: Add a sleep interval in the retry loop to prevent busy-waitingIn the
logfunction, the retry loop attempts to send the log message continuously without any delay if it fails to connect to the logging service. This can lead to high CPU usage. Consider adding a short sleep interval within the loop to prevent busy-waiting.Apply this change:
def log(msg: str, level: str): error = None success = False start_time = time.monotonic() while not success and start_time + 5.0 > time.monotonic(): try: address = ("localhost", 52999) conn = Client(address, authkey=b"debug_logger") conn.send({"name": NODE_NAME, "msg": msg, "level": level}) conn.close() success = True except Exception as e: error = e + time.sleep(0.1) # Add a short sleep to prevent busy-waiting if not success: eprint(msg) if error is not None: eprint(f"Failed to send to logger: {error}")
Line range hint
63-63: ReplaceBaseExceptionwithExceptionwhen catching exceptionsCatching
BaseExceptioncan inadvertently catch exceptions likeKeyboardInterruptandSystemExit, which may not be intended. It's advisable to catchExceptioninstead to handle typical errors.Apply this change to the exception handlers:
# In the log function - except BaseException as e: + except Exception as e: # In the start_debugger function - except BaseException as error: + except Exception as error: # In the main function - except BaseException as error: + except Exception as error:Also applies to: 105-105, 157-157
Line range hint
97-97: Replace placeholder docstring with a meaningful summaryThe docstring for the
start_debuggerfunction begins with"""_summary_, which appears to be a placeholder. Please provide a meaningful summary of the function's purpose.Apply this change:
def start_debugger( node_module_name: str, host: str, port: int, wait_for_client: bool = False ): - """_summary_ + """Start the debugpy debugger to enable remote debugging of the node. Args: node_module_name (str): Name of the underlying node. Only used for logging host (str): Host the debugger binds to port (int): Debugger port wait_for_client (bool, optional): If the debugger should wait for a client to attach. Defaults to False. """
Line range hint
143-143: Set--debug_portargument as requiredThe
--debug_portargument is essential for starting the debugger. Currently, it's marked as optional, but if not provided, the script logs an error message. To prevent this issue at runtime, mark--debug_portas a required argument in the parser.Apply this change:
parser.add_argument("--debug_node", required=True, type=str) -parser.add_argument("--debug_port", required=False, type=int) +parser.add_argument("--debug_port", required=True, type=int) parser.add_argument("--debug_host", default=default_host, type=str)
Line range hint
151-152: Consider refactoring to avoid using global variablesUsing the global variable
NODE_NAMEcan make the code harder to maintain and test. Consider passingNODE_NAMEas a parameter to the functions that need it, or encapsulate related functions within a class to manage state more effectively.
Line range hint
89-90: Avoid modifyingsys.path; useimportlibto load modules dynamicallyModifying
sys.pathcan lead to unintended side effects in module loading. Consider usingimportlibto load and execute the module without alteringsys.path. This approach makes the module loading process cleaner and more robust.
Line range hint
109-109: Useloginfoinstead oflogwarnwhen debugger starts successfullyThe message indicating that the debugger has started successfully is informative and does not represent a warning. Use
loginfoto log this message at the appropriate level.Apply this change:
- logwarn(f"Started debugger on {host}:{port} for {node_module_name}") + loginfo(f"Started debugger on {host}:{port} for {node_module_name}")code/debug_wrapper.py (3)
24-25: Complete the description for--debug_portin the usage documentationThe description for
--debug_portends abruptly with "If not set," Please complete the explanation to inform users of the default behavior or requirements when this parameter is not provided.
140-140: Correct the typo in the commentIn the comment, "Yes, all exceptions should be caught and sent into rosconsole," please fix the typo "catched" to "caught" for grammatical correctness.
120-128: Complete the docstring for thestart_debuggerfunctionThe docstring for
start_debuggercontains a placeholder_summary_. Please provide a meaningful description of the function's purpose and behavior.build/docker/agent/Dockerfile (6)
168-169: Add Comment to ExplainPAF_CATKIN_CODE_ROOTEnvironment VariableConsider adding a comment to explain the purpose of the
PAF_CATKIN_CODE_ROOTenvironment variable. This will enhance maintainability and help other developers understand its role in debugging configurations.Apply this diff:
+# Environment variable for VS Code debugging configurations ENV PAF_CATKIN_CODE_ROOT=/catkin_ws/src
Line range hint
88-106: Simplify CUDA Installation Using NVIDIA's Network RepositoryInstead of using a local installer for CUDA, consider using NVIDIA's network repository. This approach simplifies the Dockerfile, ensures that you receive updates automatically, and follows best practices recommended by NVIDIA.
Apply this diff to modify the CUDA installation steps:
# Workaround/fix for using dpkg for cuda installation # Only required for the lab PCs RUN rm -f /var/lib/dpkg/info/fprintd.postinst \ && rm -f /var/lib/dpkg/info/libfprint*.postinst \ && rm -f /var/lib/dpkg/info/libpam-fprintd*.postinst \ && dpkg --configure -a -# CUDA installation -RUN wget https://developer.download.nvidia.com/compute/cuda/repos/ubuntu2004/x86_64/cuda-ubuntu2004.pin --quiet \ - && mv cuda-ubuntu2004.pin /etc/apt/preferences.d/cuda-repository-pin-600 \ - && wget https://developer.download.nvidia.com/compute/cuda/11.8.0/local_installers/cuda-repo-ubuntu2004-11-8-local_11.8.0-520.61.05-1_amd64.deb --quiet \ - && dpkg -i cuda-repo-ubuntu2004-11-8-local_11.8.0-520.61.05-1_amd64.deb \ - && cp /var/cuda-repo-ubuntu2004-11-8-local/cuda-*-keyring.gpg /usr/share/keyrings/ \ - && apt-get update \ - && DEBIAN_FRONTEND=noninteractive apt-get -y install cuda \ - && rm cuda-repo-ubuntu2004-11-8-local_11.8.0-520.61.05-1_amd64.deb \ - && apt-get -y remove cuda-repo-ubuntu2004-11-8-local \ - && rm /etc/apt/preferences.d/cuda-repository-pin-600 \ - && rm -rf /var/cuda-repo-ubuntu1804-11-8-local/ \ - && rm /etc/apt/sources.list.d/cuda-ubuntu2004-11-8-local.list \ - && apt-get clean - -ENV CUDA_HOME=/usr/local/cuda-11.8 +# Install CUDA using NVIDIA's network repository +RUN wget https://developer.download.nvidia.com/compute/cuda/repos/ubuntu2004/x86_64/cuda-keyring_1.0-1_all.deb --quiet \ + && dpkg -i cuda-keyring_1.0-1_all.deb \ + && apt-get update \ + && apt-get install -y cuda + +ENV CUDA_HOME=/usr/local/cuda
Line range hint
159-163: Prevent Potential Conflicts in Catkin WorkspaceLinking the entire
/workspace/codedirectory into/catkin_ws/srcmight cause conflicts if there are pre-existing files or directories. Consider cleaning the/catkin_ws/srcdirectory or linking only the necessary packages to avoid overwriting important files.Apply this diff to clean the
srcdirectory before linking:# Link code into catkin workspace +RUN rm -rf /catkin_ws/src/* RUN ln -s /workspace/code /catkin_ws/src
Line range hint
176-177: Combine RUN Commands to Optimize Docker Image LayersCombine consecutive
RUNcommands that modify the same file (~/.bashrc) to reduce the number of layers and improve build efficiency.Apply this diff:
-RUN echo "source /opt/ros/noetic/setup.bash" >> ~/.bashrc -RUN echo "source /catkin_ws/devel/setup.bash" >> ~/.bashrc +RUN echo "source /opt/ros/noetic/setup.bash" >> ~/.bashrc && \ + echo "source /catkin_ws/devel/setup.bash" >> ~/.bashrc
Line range hint
134-138: Verify PATH Environment Variable for RedundanciesThe
ENV PATHis being extended with/home/$USERNAME/.local/bin. Ensure that this directory is not already included in the PATH to avoid redundancies. Also, confirm that this path is necessary for your application.
Line range hint
159-160: Consider Copying Only Required Files to Reduce Image SizeInstead of copying the entire
./codedirectory, consider copying only the necessary files or directories. This can help reduce the Docker image size and improve build times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
.vscode/launch.json(1 hunks)build/agent_service.yaml(1 hunks)build/docker-compose.leaderboard.yaml(1 hunks)build/docker/agent/Dockerfile(1 hunks)code/acting/src/debug_wrapper.py(1 hunks)code/agent/launch/agent.launch(1 hunks)code/debug_wrapper.py(1 hunks)code/debugging/CMakeLists.txt(1 hunks)code/debugging/launch/debugging.launch(1 hunks)code/debugging/package.xml(1 hunks)code/debugging/setup.py(1 hunks)code/debugging/src/debug_logger.py(1 hunks)code/mock/src/debug_wrapper.py(1 hunks)code/perception/launch/perception.launch(1 hunks)code/perception/src/debug_wrapper.py(1 hunks)code/planning/src/debug_wrapper.py(1 hunks)code/requirements.txt(1 hunks)doc/development/debugging.md(1 hunks)pyrightconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- code/debugging/launch/debugging.launch
- code/debugging/package.xml
- code/debugging/setup.py
- pyrightconfig.json
🧰 Additional context used
🪛 Biome
.vscode/launch.json
[error] 22-23: Expected a property but instead found '}'.
Expected a property here.
(parse)
🪛 Ruff
code/debugging/src/debug_logger.py
87-88: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 LanguageTool
doc/development/debugging.md
[uncategorized] ~6-~6: This expression is usually spelled with a hyphen.
Context: ...ilities](#debugging-possibilities) - Message based debugging ...
(BASED_HYPHEN)
[uncategorized] ~8-~8: This expression is usually spelled with a hyphen.
Context: ...iewing-the-messages) - [Problems of message based debugging](#problems-of-message-based-d...
(BASED_HYPHEN)
[grammar] ~10-~10: This phrase is duplicated. You should probably use “required once” only once.
Context: ...](#vs-code-debugger) - Setup steps required once - [Required once for the node you want to debug](#requir...
(PHRASE_REPETITION)
[uncategorized] ~21-~21: This expression is usually spelled with a hyphen.
Context: ...s: Message based and VS Code based ### Message based debugging Messages can be logged into ...
(BASED_HYPHEN)
[formatting] ~26-~26: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ic. rospy.logdebug is not recommended, because these messages are not published to /ro...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~54-~54: This expression is usually spelled with a hyphen.
Context: ...spy/Overview/Logging) #### Problems of message based debugging - Time intensive and cumbers...
(BASED_HYPHEN)
[formatting] ~76-~76: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...* can be a port in range 53000-53100, because those are exposed from the [leaderboard...
(COMMA_BEFORE_BECAUSE)
[style] ~98-~98: As a shorter alternative for ‘able to’, consider using “can then only”.
Context: ...When you attach the debugger later, you are then only able to debug the callbacks the node receives. ...
(BE_ABLE_TO)
🔇 Additional comments (17)
.vscode/launch.json (2)
8-14: LGTM! Debug configuration is well-structured.
The configuration correctly uses:
- debugpy for Python debugging
- Attach mode for connecting to running processes
- Consistent port numbering in both name and configuration
15-20: Verify environment variable setup in Docker.
The path mapping looks correct, but let's ensure the PAF_CATKIN_CODE_ROOT environment variable is properly set in the Docker environment.
✅ Verification successful
The search results show that PAF_CATKIN_CODE_ROOT is properly defined in build/docker/agent/Dockerfile with the value /catkin_ws/src. This is a standard path for ROS/catkin workspaces in Docker containers. Let me generate the final response:
Environment variable setup verified in Docker configuration
The PAF_CATKIN_CODE_ROOT environment variable is correctly set to /catkin_ws/src in the Docker environment, which is the standard source directory for catkin workspaces. The path mapping in launch.json will work as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PAF_CATKIN_CODE_ROOT environment variable setup
# Test: Search for PAF_CATKIN_CODE_ROOT in Dockerfile and docker-compose files
echo "Checking Dockerfile and docker-compose files for PAF_CATKIN_CODE_ROOT..."
rg "PAF_CATKIN_CODE_ROOT" -A 2
Length of output: 347
build/docker-compose.leaderboard.yaml (1)
11-13: Consider reducing the exposed port range.
While exposing ports for debugging is necessary, the current range of 100 ports (53000-53100) seems excessive for debugging purposes. A smaller range would be more secure and reduce potential port conflicts.
Let's verify if we actually need this many ports:
Consider:
- Reducing to a smaller range (e.g., 53000-53010)
- Adding documentation explaining the port range requirement
- Making the port range configurable via environment variables
build/agent_service.yaml (1)
33-33: LGTM with security considerations.
The addition of DEBUG_WRAPPER_DEFAULT_HOST=0.0.0.0 aligns with the PR's debugging objectives. However, be aware that this configuration allows the debug wrapper to listen on all network interfaces, which is suitable for development but could pose security risks if used in production.
Let's verify if there are any security measures or documentation addressing this:
✅ Verification successful
Debug host configuration is properly scoped for development
The DEBUG_WRAPPER_DEFAULT_HOST=0.0.0.0 setting is correctly configured as part of the development debugging setup:
- It's used in conjunction with VS Code's debug configuration that only connects to
localhost:53000 - The debug ports (53000-53100) are documented and specifically intended for development debugging
- The setup is part of a well-structured debugging workflow documented in the repository
- The debug wrapper is only present in development-related docker compose files (leaderboard, devroute)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security documentation regarding debug configuration
# Expected: Documentation about security implications and usage guidelines
# Look for relevant documentation
echo "Searching for debug configuration documentation..."
rg -i "debug.*host|debugger.*security|debug.*production" docs/
rg -i "debug.*host|debugger.*security|debug.*production" README.md
Length of output: 290
Script:
#!/bin/bash
# Let's check for any documentation or configuration files related to debugging
# across the repository, including markdown, yaml, and json files
# Search for debug-related files
echo "=== Files containing debug in their names ==="
fd -t f "debug|debugger" -E "node_modules"
# Search for debug configuration in yaml files
echo -e "\n=== Debug configuration in yaml files ==="
rg -i "debug.*host|debug.*port|debug.*wrapper" -g "*.yaml" -g "*.yml"
# Search for debug documentation in any markdown or text files
echo -e "\n=== Debug documentation in markdown/txt files ==="
fd -e md -e txt -E "node_modules" -x rg -i "debug.*configuration|debug.*setup|debug.*environment"
# Check launch.json for debug configurations
echo -e "\n=== VS Code debug configurations ==="
cat .vscode/launch.json 2>/dev/null || echo "No launch.json found"
Length of output: 3285
code/perception/launch/perception.launch (1)
Line range hint 78-82: Verify if LIDAR node addition belongs in this PR
The addition of the LIDAR distance node appears unrelated to this PR's primary objective of enhancing VS Code debugging capabilities. Consider:
- Moving this change to a separate PR focused on LIDAR functionality
- Or clarify if this node is specifically needed for testing the new debugging features
Let's check if this node is referenced in any debugging-related changes:
✅ Verification successful
The LIDAR node addition is part of the debugging PR and should be kept
Based on the git history and code analysis:
- The LIDAR distance node was added in commit 4ef21fc with message "Basic debug wrapper working", which is directly related to the debugging feature
- The code in
lidar_distance.pypublishes distance arrays that can be visualized for debugging purposes, as evidenced by:- Multiple camera angle views (Center, Back, Left, Right) for comprehensive debugging
- Conversion to image messages that can be visualized in debugging tools
- Detailed distance information (x, y, z coordinates) useful for debugging perception
The node provides essential distance visualization capabilities that complement the PR's debugging objectives by allowing developers to visually inspect LIDAR data during debugging sessions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to lidar_distance in debug-related files
echo "Searching for lidar_distance references in debug configuration..."
rg "lidar_distance" .vscode/
echo "Searching for lidar_distance references in debug wrapper..."
rg "lidar_distance" "**/debug_wrapper.py"
Length of output: 400
Script:
#!/bin/bash
# Let's check the PR description and commit messages for context
echo "Checking PR description and commits..."
gh pr view
# Let's also check for any debug-related files that might reference LIDAR
echo -e "\nSearching for debug-related files..."
fd -e launch debug
fd -e py debug
# Check for LIDAR-related changes in the PR
echo -e "\nSearching for LIDAR-related changes..."
git diff origin/main... -- '**/lidar*.py' '**/lidar*.launch'
Length of output: 808
Script:
#!/bin/bash
# Let's check the content of debug-related files for any LIDAR references
echo "Checking debug-related files for LIDAR references..."
for file in code/debugging/launch/debugging.launch \
code/acting/src/acting/Acting_Debug_Node.py \
code/debug_wrapper.py \
code/debugging/src/debug_logger.py; do
echo -e "\nChecking $file:"
cat "$file" | grep -i "lidar"
done
# Let's also check if lidar_distance.py exists and its content
echo -e "\nChecking lidar_distance.py content:"
fd -t f "lidar_distance.py"
Length of output: 1531
Script:
#!/bin/bash
# Let's check the content of lidar_distance.py to see if it contains any debugging-related code
echo "Checking lidar_distance.py content:"
cat code/perception/src/lidar_distance.py
# Also check the git history of this file to understand when it was added
echo -e "\nChecking git history of lidar_distance.py:"
git log --full-history -- code/perception/src/lidar_distance.py
Length of output: 31158
code/acting/src/debug_wrapper.py (2)
Line range hint 130-157: LGTM! Well-implemented argument parsing.
The argument parsing implementation is robust with:
- Custom error handling
- ROS-compatible argument processing
- Environment variable configuration
- Clear parameter validation
1-1:
Remove the relative path from the first line.
The relative path "../../debug_wrapper.py" at the beginning of the file seems out of place and could cause issues with file resolution.
-../../debug_wrapper.pyLikely invalid or redundant comment.
code/debugging/CMakeLists.txt (4)
1-6: LGTM! Basic CMake configuration is appropriate.
The configuration is correctly set up for a ROS package focused on debugging functionality.
158-163:
Add installation rules for Python nodes.
The debug wrapper and logger nodes need to be installed. Uncomment and configure the Python script installation:
-# catkin_install_python(PROGRAMS
-# scripts/my_python_script
-# DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION}
-# )
+catkin_install_python(PROGRAMS
+ scripts/debug_wrapper.py
+ scripts/debug_logger.py
+ DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION}
+)#!/bin/bash
# Verify the existence of the Python scripts
for script in debug_wrapper.py debug_logger.py; do
find code/debugging -name "$script" -type f
done193-204: 🛠️ Refactor suggestion
Add test configuration for debugging tools.
Given the critical nature of debugging tools, it's important to include tests to ensure reliability:
- Add Python tests for the debug wrapper functionality
- Add tests for the debug logger
- Include integration tests with VS Code debugging
-## Add folders to be run by python nosetests
-# catkin_add_nosetests(test)
+## Add folders to be run by python nosetests
+catkin_add_nosetests(test)#!/bin/bash
# Check if test directory exists with test files
if [ -d code/debugging/test ]; then
echo "Test files found:"
find code/debugging/test -name "test_*.py"
else
echo "Test directory is missing!"
fi10-12: Consider additional dependencies for debugging functionality.
Given that this package will provide VS Code debugging capabilities, consider if additional dependencies are needed:
message_generationif you plan to define custom debug messagesstd_msgsfor basic message types- Consider declaring Python package dependencies (like
debugpy) inpackage.xml
code/mock/src/debug_wrapper.py (1)
1-1:
Unintended inclusion of file path
The line ../../debug_wrapper.py at the beginning of the file appears unintended and may lead to a syntax error. It should be removed to prevent issues during execution.
Apply this diff to fix the issue:
-../../debug_wrapper.pyLikely invalid or redundant comment.
code/perception/src/debug_wrapper.py (2)
Line range hint 46-51: Good practice: Using eprint to ensure logging to stderr
The use of eprint to log messages to stderr is a good practice, ensuring that log messages are captured reliably in agent.log.
1-1:
Remove unintended file path inclusion at the beginning of the script
The line ../../debug_wrapper.py at the very start seems unintended and could cause issues when running the script.
Apply this diff to remove the line:
-../../debug_wrapper.pyLikely invalid or redundant comment.
code/debug_wrapper.py (1)
139-141: 🛠️ Refactor suggestion
Use Exception instead of BaseException when catching exceptions
To prevent unintended behavior by capturing system-exiting exceptions, replace BaseException with Exception in the exception handling block.
Apply this diff:
- except BaseException as error:
+ except Exception as error:Likely invalid or redundant comment.
build/docker/agent/Dockerfile (2)
169-169: Verify Usage of PAF_CATKIN_CODE_ROOT in Debugging Setup
Ensure that the PAF_CATKIN_CODE_ROOT environment variable is correctly referenced in all relevant scripts and configurations, especially in the .vscode/launch.json file. This will confirm that the debugging setup functions as intended.
Line range hint 159-160: Ensure Correct Ownership and Permissions When Copying Code
Verify that the COPY command correctly sets the ownership and permissions of the /workspace/code/ directory. Ensure that the $USERNAME variable is properly defined at build time to prevent potential permission issues.
| ], | ||
| } |
There was a problem hiding this comment.
Remove trailing comma for valid JSON syntax.
The trailing comma after the configurations array should be removed.
Apply this diff to fix the JSON syntax:
"configurations": [
{
"name": "53000 leaderboard attach",
"type": "debugpy",
"request": "attach",
"connect": {
"host": "localhost",
"port": 53000
},
"pathMappings": [
{
"localRoot": "${workspaceFolder}/code",
"remoteRoot": "${env:PAF_CATKIN_CODE_ROOT}"
}
]
- }
- ],
+ }
+ ]
}📝 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.
| ], | |
| } | |
| ], | |
| } | |
| ``` | |
| ↓ | |
| ```suggestion | |
| ] | |
| } |
🧰 Tools
🪛 Biome
[error] 22-23: Expected a property but instead found '}'.
Expected a property here.
(parse)
| Adjust the launch configuration of the node to use the [debug_wrapper.py](../../code/debug_wrapper.py) node. | ||
|
|
||
| Change the type to **debug_wrapper.py** and set `args="--debug_node=<original type> --debug_port=<debug_port>"`. | ||
| *debug_port* can be a port in range **53000-53100**, because those are exposed from the [leaderboard docker configuration](../../build/docker-compose.leaderboard.yaml). | ||
|
|
There was a problem hiding this comment.
Clarify security implications of debug ports.
The documentation should warn users about the security implications of exposing debug ports (53000-53100) in production environments.
Consider adding:
> **Security Note:** The debug ports (53000-53100) should only be exposed in development environments. In production, ensure these ports are closed to prevent potential security vulnerabilities.🧰 Tools
🪛 LanguageTool
[formatting] ~76-~76: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...* can be a port in range 53000-53100, because those are exposed from the [leaderboard...
(COMMA_BEFORE_BECAUSE)
| #### Known problems of debugging with VS Code | ||
|
|
||
| - Adjusting the launch configurations is a bit cumbersome | ||
| - The side effects of just "stopping" a node with the debugger are unknown | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add troubleshooting section.
The known problems section should be expanded with troubleshooting steps for common issues.
Consider adding:
### Troubleshooting Guide
1. **Debugger won't attach**
- Verify the debug port is correctly exposed in the docker-compose file
- Check if another process is using the same debug port
- Ensure the node is running and waiting for debugger connection
2. **Node crashes after detaching debugger**
- Always use breakpoint conditions to safely detach
- Consider implementing graceful shutdown handlers
3. **Missing source maps**
- Verify the source files are mounted correctly in the container
- Check the `launch.json` configuration
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (42)
.vscode/launch.json (1)
15-20: Consider enhancing the debug configuration with additional settings.The path mapping is correctly configured, but the configuration could benefit from some additional settings for better usability and reliability:
Consider adding these optional settings:
"pathMappings": [ { "localRoot": "${workspaceFolder}/code", "remoteRoot": "${env:PAF_CATKIN_CODE_ROOT}" } - ] + ], + "justMyCode": false, + "showReturnValue": true, + "redirectOutput": true, + "justifyTooltip": "Debug configuration for attaching to ROS nodes in the leaderboard container", + "timeout": 20000These additions would:
- Allow debugging into library code
- Show function return values in debug view
- Capture output in debug console
- Provide documentation tooltip
- Set a reasonable connection timeout
code/agent/launch/agent.launch (1)
24-26: Consider enhancing the debugging configuration.The debugging setup could benefit from the following improvements:
- Add a conditional include using an arg to enable/disable debugging
- Pass relevant arguments like role_name for targeted debugging
Consider applying this enhancement:
+ <arg name="debug_enabled" default="false" /> + <!-- debugging --> - <include file="$(find debugging)/launch/debugging.launch"> + <include if="$(arg debug_enabled)" file="$(find debugging)/launch/debugging.launch"> + <arg name="role_name" value="$(arg role_name)" /> </include>code/perception/launch/perception.launch (1)
Warning: Topic parameter mismatch in the launch file and node implementation
The launch file parameters don't match the node's implementation:
- Launch file uses
range_topicbut the node doesn't use this parameter- Node uses additional parameters not specified in launch:
source_topic(defaults to "/carla/hero/LIDAR")image_distance_topic(defaults to "/paf/hero/{Direction}/dist_array")🔗 Analysis chain
Line range hint
78-82: LGTM! The parameterization of LIDAR topics follows ROS best practices.The addition of configurable parameters for point cloud and range topics enhances the reusability of the node and aligns with ROS conventions.
Let's verify the topic names are consistent with the LIDAR node implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the LIDAR node uses these topic names correctly # Test 1: Check if the node implementation references these parameters rg -A 5 "point_cloud_topic|range_topic" --type python # Test 2: Check if these specific topics are published anywhere in the CARLA integration rg -A 5 "/carla/hero/LIDAR_filtered|/carla/hero/LIDAR_range"Length of output: 2949
Script:
#!/bin/bash # Let's check for the Python node implementation fd "lidar_distance.py" --exec cat {}Length of output: 9012
code/acting/src/debug_wrapper.py (6)
Line range hint
63-82: Add retry delay and improve error handling in log function.The current implementation might cause high CPU usage during connection failures due to rapid retry attempts. Additionally, the error handling could be more robust.
def log(msg: str, level: str): error = None success = False start_time = time.monotonic() while not success and start_time + 5.0 > time.monotonic(): try: address = ("localhost", 52999) conn = Client(address, authkey=b"debug_logger") conn.send({"name": NODE_NAME, "msg": msg, "level": level}) conn.close() success = True except BaseException as e: error = e + time.sleep(0.1) # Add delay between retries if not success: eprint(msg) if error is not None: - eprint(f"Failed to send to logger: {error}") + eprint(f"Failed to send to logger after {time.monotonic() - start_time:.1f}s: {error}")
Line range hint
108-117: Improve module loading safety and cleanup.The current implementation modifies sys.path without cleanup and lacks proper error handling for module loading failures.
def run_module_at(path: str): basename = os.path.basename(path) module_dir = os.path.dirname(path) module_name = os.path.splitext(basename)[0] + original_path = sys.path.copy() sys.path.append(module_dir) - runpy.run_module(module_name, run_name="__main__") + try: + runpy.run_module(module_name, run_name="__main__") + except ImportError as e: + raise ImportError(f"Failed to import module {module_name}: {e}") + finally: + sys.path = original_path
Line range hint
119-143: Improve debugger initialization with proper typing and exception handling.The current implementation has an incomplete docstring, lacks type hints, and uses overly broad exception handling.
-def start_debugger( +def start_debugger( node_module_name: str, host: str, port: int, wait_for_client: bool = False -): +) -> None: - """_summary_ + """Initialize and start the debugpy debugger for remote debugging. Args: node_module_name (str): Name of the underlying node. Only used for logging host (str): host the debugger binds to port (int): debugger port wait_for_client (bool, optional): If the debugger should wait for a client to attach. Defaults to False. + + Raises: + ImportError: If debugpy module is not available + ConnectionError: If debugger fails to listen on specified host/port """ debugger_spec = importlib.util.find_spec("debugpy") if debugger_spec is not None: try: import debugpy - debugpy.listen((host, port)) logwarn(f"Started debugger on {host}:{port} for {node_module_name}") if wait_for_client: debugpy.wait_for_client() - except BaseException as error: - # Yes, all exceptions should be catched and sent into rosconsole + except Exception as error: logerr(f"Failed to start debugger: {error}") else: - logerr("debugpy module not found. Unable to start debugger") + msg = "debugpy module not found. Unable to start debugger" + logerr(msg) + raise ImportError(msg)
Line range hint
145-168: Add argument validation for debug port range.The argument parsing could benefit from validation to ensure the debug port is within the allowed range (53000-53100) as specified in the docker-compose configuration.
def main(argv): default_host = "localhost" if "DEBUG_WRAPPER_DEFAULT_HOST" in os.environ: default_host = os.environ["DEBUG_WRAPPER_DEFAULT_HOST"] + def port_range(value): + ivalue = int(value) + if ivalue < 53000 or ivalue > 53100: + raise ArgumentParserError( + f"Port {ivalue} is outside allowed range (53000-53100)" + ) + return ivalue + node_args = rospy.myargv(argv=argv) parser = ThrowingArgumentParser( prog="debug wrapper", ) parser.add_argument("--debug_node", required=True, type=str) - parser.add_argument("--debug_port", required=False, type=int) + parser.add_argument("--debug_port", required=False, type=port_range) parser.add_argument("--debug_host", default=default_host, type=str) parser.add_argument("--debug_wait", default=False, type=bool)
Line range hint
170-195: Improve main execution flow with proper signal handling and error management.The main execution flow could benefit from proper signal handling for graceful shutdown and more specific error handling. Additionally, the global variable usage should be reconsidered.
+import signal +from typing import Optional + +class DebugWrapper: + def __init__(self): + self.node_name: Optional[str] = None + + def handle_shutdown(self, signum, frame): + if self.node_name: + loginfo(f"Shutting down node {self.node_name}") + sys.exit(0) + def main(argv): + wrapper = DebugWrapper() + signal.signal(signal.SIGINT, wrapper.handle_shutdown) + signal.signal(signal.SIGTERM, wrapper.handle_shutdown) + debug_node = args.debug_node - global NODE_NAME - NODE_NAME = debug_node + wrapper.node_name = debug_node base_dir = os.path.abspath(os.path.dirname(__file__)) if args.debug_port is not None: start_debugger( args.debug_node, args.debug_host, args.debug_port, wait_for_client=args.debug_wait, ) else: logerr( """Missing parameter to start debugger: --debug_port Add it in the launch configuration""" ) target_type_path = os.path.join(base_dir, debug_node) loginfo(f"Node {args.debug_node} starting at {base_dir}") try: run_module_at(target_type_path) - except BaseException as error: + except (ImportError, OSError) as error: logfatal(f"Failed to run node {debug_node}: {error}") raise error + except Exception as error: + logfatal(f"Unexpected error running node {debug_node}: {error}") + raise error
Line range hint
1-195: Overall implementation successfully meets debugging requirements.The debug wrapper implementation successfully achieves the PR objectives by providing:
- Remote debugging capability using debugpy
- Comprehensive logging through debug_logger
- Proper error handling and reporting
- Configuration through environment variables and command-line arguments
While the core functionality is solid, the suggested improvements will enhance reliability and maintainability.
Consider adding integration tests to verify the debugging functionality in different scenarios, such as:
- Connection timeout handling
- Port conflict resolution
- Signal handling during debugging
code/mock/src/debug_wrapper.py (6)
Line range hint
2-32: Consider enhancing docstrings with type hints.While the documentation is comprehensive, consider adding type hints to the docstrings using the Google style format for better IDE integration and code clarity.
Example enhancement:
"""Debug wrapper node Args: debug_node (str): The filename of the node to debug debug_port (int): The port the debugger listens on debug_host (str): The host the debugger binds to debug_wait (bool): If True, waits for client connection Returns: None Raises: ArgumentParserError: When required parameters are missing Exception: If the started debug_node raises an exception """
Line range hint
34-82: Enhance logging robustness and security.Several improvements could make the logging system more robust and secure:
- The debug logger port (52999) should be configurable via environment variable
- The connection timeout (5.0) should be configurable
- Message content should be validated before sending
- Consider implementing a proper retry strategy with backoff
Here's a suggested implementation:
+import os +from typing import Optional + +DEBUG_LOGGER_PORT = int(os.getenv('DEBUG_LOGGER_PORT', '52999')) +DEBUG_LOGGER_TIMEOUT = float(os.getenv('DEBUG_LOGGER_TIMEOUT', '5.0')) + +def validate_log_message(msg: str, level: str) -> bool: + """Validate log message and level.""" + valid_levels = {'debug', 'info', 'warn', 'error', 'fatal'} + return bool(msg) and level in valid_levels + def log(msg: str, level: str): - error = None - success = False - start_time = time.monotonic() - while not success and start_time + 5.0 > time.monotonic(): + if not validate_log_message(msg, level): + eprint(f"Invalid log message or level: {msg}, {level}") + return + + last_error: Optional[Exception] = None + attempt = 0 + max_attempts = 3 + + while attempt < max_attempts: try: - address = ("localhost", 52999) + address = ("localhost", DEBUG_LOGGER_PORT) conn = Client(address, authkey=b"debug_logger") conn.send({"name": NODE_NAME, "msg": msg, "level": level}) conn.close() - success = True + return except BaseException as e: - error = e - if not success: - eprint(msg) - if error is not None: - eprint(f"Failed to send to logger: {error}") + last_error = e + attempt += 1 + if attempt < max_attempts: + time.sleep(min(0.1 * 2 ** attempt, 1.0)) # Exponential backoff + + eprint(msg) + if last_error: + eprint(f"Failed to send to logger after {max_attempts} attempts: {last_error}")
Line range hint
84-95: Secure module execution and path handling.The current implementation has potential security and reliability issues:
- Modifying
sys.pathwithout cleanup could affect module resolution- No validation of the module path could lead to security vulnerabilities
- Missing error handling for invalid modules
Here's a suggested implementation:
+import pathlib + def run_module_at(path: str): """Runs a python module based on its file path Args: path (str): python file path to run + Raises: + ValueError: If the path is invalid or outside the allowed directory """ + try: + module_path = pathlib.Path(path).resolve() + base_dir = pathlib.Path(__file__).parent.resolve() + + # Ensure module is within the allowed directory + if not str(module_path).startswith(str(base_dir)): + raise ValueError(f"Module path {path} is outside the allowed directory") + + if not module_path.is_file(): + raise ValueError(f"Module path {path} does not exist or is not a file") + basename = os.path.basename(path) module_dir = os.path.dirname(path) module_name = os.path.splitext(basename)[0] + original_path = sys.path[:] sys.path.append(module_dir) - runpy.run_module(module_name, run_name="__main__") + try: + runpy.run_module(module_name, run_name="__main__") + finally: + sys.path[:] = original_path + except Exception as e: + logerr(f"Failed to run module {path}: {e}") + raise
Line range hint
97-122: Improve debugger initialization safety and documentation.The debugger initialization needs several improvements:
- The docstring is incomplete (has "summary" placeholder)
- Missing validation for host and port values
- Too broad exception handling
Here's a suggested implementation:
+from typing import Optional +import socket + +def validate_port(port: int) -> bool: + """Validate if port number is valid.""" + return isinstance(port, int) and 1024 <= port <= 65535 + +def validate_host(host: str) -> bool: + """Validate if host is valid.""" + try: + socket.gethostbyname(host) + return True + except socket.error: + return False + def start_debugger( node_module_name: str, host: str, port: int, wait_for_client: bool = False ): - """_summary_ + """Start the debugpy debugger for remote debugging. Args: node_module_name (str): Name of the underlying node. Only used for logging host (str): host the debugger binds to port (int): debugger port wait_for_client (bool, optional): If the debugger should wait for a client to attach. Defaults to False. + + Raises: + ValueError: If host or port is invalid + ImportError: If debugpy module is not available """ + if not validate_port(port): + raise ValueError(f"Invalid port number: {port}") + if not validate_host(host): + raise ValueError(f"Invalid host: {host}") + debugger_spec = importlib.util.find_spec("debugpy") if debugger_spec is not None: try: import debugpy - debugpy.listen((host, port)) logwarn(f"Started debugger on {host}:{port} for {node_module_name}") if wait_for_client: debugpy.wait_for_client() - except BaseException as error: - # Yes, all exceptions should be catched and sent into rosconsole + except Exception as error: logerr(f"Failed to start debugger: {error}") + raise else: - logerr("debugpy module not found. Unable to start debugger") + msg = "debugpy module not found. Unable to start debugger" + logerr(msg) + raise ImportError(msg)
Line range hint
124-131: Improve argument parsing organization and type safety.The argument parsing implementation could be enhanced with:
- Type hints for better code maintainability
- Removal of global variable usage
- Better error handling structure
Here's a suggested implementation:
+from dataclasses import dataclass +from typing import Optional, Sequence + +@dataclass +class DebugArgs: + """Debug wrapper arguments.""" + debug_node: str + debug_port: Optional[int] + debug_host: str + debug_wait: bool + class ArgumentParserError(Exception): - pass + """Raised when argument parsing fails.""" class ThrowingArgumentParser(argparse.ArgumentParser): - def error(self, message): + """ArgumentParser that raises exceptions instead of exiting.""" + def error(self, message: str) -> None: logfatal(f"Wrong node arguments. Check launch config. : {message}") raise ArgumentParserError(message) + +def parse_args(argv: Sequence[str]) -> DebugArgs: + """Parse command line arguments. + + Args: + argv: Command line arguments + + Returns: + DebugArgs: Parsed arguments + + Raises: + ArgumentParserError: If argument parsing fails + """ + default_host = os.getenv("DEBUG_WRAPPER_DEFAULT_HOST", "localhost") + parser = ThrowingArgumentParser(prog="debug wrapper") + parser.add_argument("--debug_node", required=True, type=str) + parser.add_argument("--debug_port", required=False, type=int) + parser.add_argument("--debug_host", default=default_host, type=str) + parser.add_argument("--debug_wait", default=False, type=bool) + + args, _ = parser.parse_known_args(argv) + return DebugArgs( + debug_node=args.debug_node, + debug_port=args.debug_port, + debug_host=args.debug_host, + debug_wait=args.debug_wait + )
Line range hint
133-186: Restructure main execution flow for better organization and error handling.The main execution flow needs improvements in:
- Separation of concerns
- Proper cleanup in error cases
- Structured error handling
- Path validation
Here's a suggested implementation:
-def main(argv): +def main(argv: Sequence[str]) -> None: + """Main entry point for the debug wrapper. + + Args: + argv: Command line arguments + + Raises: + Exception: If any error occurs during execution + """ try: - default_host = "localhost" - if "DEBUG_WRAPPER_DEFAULT_HOST" in os.environ: - default_host = os.environ["DEBUG_WRAPPER_DEFAULT_HOST"] - - node_args = rospy.myargv(argv=argv) - parser = ThrowingArgumentParser( - prog="debug wrapper", - ) - parser.add_argument("--debug_node", required=True, type=str) - parser.add_argument("--debug_port", required=False, type=int) - parser.add_argument("--debug_host", default=default_host, type=str) - parser.add_argument("--debug_wait", default=False, type=bool) - args, unknown_args = parser.parse_known_args(node_args) - - debug_node = args.debug_node - global NODE_NAME - NODE_NAME = debug_node - base_dir = os.path.abspath(os.path.dirname(__file__)) + # Parse arguments + node_args = rospy.myargv(argv=argv) + args = parse_args(node_args) + + # Setup logging + global NODE_NAME # TODO: Remove global variable in future refactor + NODE_NAME = args.debug_node - if args.debug_port is not None: - start_debugger( - args.debug_node, - args.debug_host, - args.debug_port, - wait_for_client=args.debug_wait, - ) - else: + # Start debugger if port is specified + if args.debug_port is None: logerr( """Missing parameter to start debugger: --debug_port Add it in the launch configuration""" ) + else: + start_debugger( + args.debug_node, + args.debug_host, + args.debug_port, + wait_for_client=args.debug_wait + ) - target_type_path = os.path.join(base_dir, debug_node) - loginfo(f"Node {args.debug_node} starting at {base_dir}") - try: - run_module_at(target_type_path) - except BaseException as error: - logfatal(f"Failed to run node {debug_node}: {error}") - raise error + # Run the target node + base_dir = pathlib.Path(__file__).parent.resolve() + target_path = base_dir / args.debug_node + loginfo(f"Node {args.debug_node} starting at {base_dir}") + run_module_at(str(target_path)) + except ArgumentParserError: + # Already logged in ThrowingArgumentParser + sys.exit(1) + except Exception as error: + logfatal(f"Failed to run node {args.debug_node}: {error}") + raisedoc/development/debugging.md (5)
1-16: Well-structured documentation with clear navigation.The table of contents provides a comprehensive overview of debugging capabilities. Consider standardizing hyphenation in section titles:
- "Message based" → "Message-based"
- "VS Code based" → "VS Code-based"
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: This expression is usually spelled with a hyphen.
Context: ...ilities](#debugging-possibilities) - Message based debugging ...(BASED_HYPHEN)
[uncategorized] ~8-~8: This expression is usually spelled with a hyphen.
Context: ...iewing-the-messages) - [Problems of message based debugging](#problems-of-message-based-d...(BASED_HYPHEN)
[grammar] ~10-~10: This phrase is duplicated. You should probably use “required once” only once.
Context: ...](#vs-code-debugger) - Setup steps required once - [Required once for the node you want to debug](#requir...(PHRASE_REPETITION)
17-60: Enhance message-based debugging section with examples.Consider adding:
- Code examples for each logging function to demonstrate proper usage
- A table showing log levels and their typical use cases
- Examples of common filtering patterns in rqt_console
Would you like me to help generate these examples?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: This expression is usually spelled with a hyphen.
Context: ...s: Message based and VS Code based ### Message based debugging Messages can be logged into ...(BASED_HYPHEN)
[formatting] ~26-~26: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ic.rospy.logdebugis not recommended, because these messages are not published to /ro...(COMMA_BEFORE_BECAUSE)
[uncategorized] ~54-~54: This expression is usually spelled with a hyphen.
Context: ...spy/Overview/Logging) #### Problems of message based debugging - Time intensive and cumbers...(BASED_HYPHEN)
89-96: Enhance launch configuration example with troubleshooting tips.The launch configuration example is clear, but consider adding:
- Common configuration errors and their solutions
- How to verify the debug wrapper is running correctly
- Expected console output when debugging is properly configured
120-132: Enhance Docker rebuild instructions with safety checks.Consider adding:
- A command to verify current container state before rebuilding
- Instructions to clean up old containers and images
- Verification steps after rebuild
Example addition:
# Check current container state docker compose -f ./docker-compose.leaderboard.yaml ps docker compose -f ./docker-compose.dev.yaml ps # Clean up (optional) docker compose -f ./docker-compose.leaderboard.yaml down docker compose -f ./docker-compose.dev.yaml down # Rebuild and verify # ... existing commands ... # Verify new containers docker compose -f ./docker-compose.leaderboard.yaml ps docker compose -f ./docker-compose.dev.yaml ps
134-136: Expand sources section with relevant documentation.Consider adding links to:
- VS Code Python debugging documentation
- debugpy documentation
- ROS debugging tools overview
- Docker debugging best practices
code/debugging/src/debug_logger.py (5)
83-83: UseeprintInstead ofIn line 83, you're using
stderr, consider using theeprintfunction defined earlier.Apply this diff to use
eprint:- print(f"[debug_logger]: connection accepted from {listener.last_accepted}") + eprint(f"connection accepted from {listener.last_accepted}")
87-88: Simplify Nested If Statements by Combining ConditionsThe nested
ifstatements can be combined into a single condition for better readability and efficiency.Apply this diff to simplify the conditions:
- if isinstance(msg, str): - if msg.lower() == CLOSE_MSG: + if isinstance(msg, str) and msg.lower() == CLOSE_MSG:🧰 Tools
🪛 Ruff
87-88: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
124-124: Usesys.exit(0)Instead ofexit(0)for Proper Script TerminationThe
exit()function is intended for use in the interactive interpreter and may not work as expected in scripts. Usingsys.exit(0)is the standard way to terminate a script.Ensure that
sysis imported at the beginning of your script:+import sysThen, apply this diff to use
sys.exit(0):- exit(0) + sys.exit(0)
133-133: Avoid Using Fixed Sleep; Use ROS Mechanisms for Node InitializationUsing
time.sleep(5.0)to wait for the node to initialize may cause unnecessary delays. Consider using ROS mechanisms likerospy.wait_for_messageor checkingrospy.is_shutdown()for a more responsive and reliable approach.Here's how you might modify the code:
- time.sleep(5.0) + # Ensure the ROS master is available + while not rospy.core.is_initialized(): + time.sleep(0.1)This change waits until the ROS core is initialized without an arbitrary delay.
141-151: Handle Missing Keys in Message Dictionary SafelyIn the message processing loop, default values are assigned before checking if the keys exist in the message dictionary. Consider using the
dict.get()method with default values for cleaner code.Apply this diff to streamline key access:
if isinstance(msg, dict): - if "name" in msg: - log_name = msg["name"] - if "msg" in msg: - log_msg = msg["msg"] - if "level" in msg: - log_level = msg["level"] + log_name = msg.get("name", "NAMERR") + log_msg = msg.get("msg", "Wrong log message format") + log_level = msg.get("level", "fatal")code/perception/src/debug_wrapper.py (8)
Line range hint
46-52: Complete the docstring instart_debuggerfunctionThe docstring for
start_debuggeris incomplete; it begins with"""_summary_and lacks a proper summary of the function's purpose. Providing a clear description enhances readability and maintainability.
Line range hint
27-34: Avoid catchingBaseException; catchExceptioninsteadCatching
BaseExceptionis not recommended as it includes system-exiting exceptions likeKeyboardInterruptandSystemExit. This can lead to unexpected behavior and make the program hard to interrupt. It's better to catchExceptionto handle standard errors.Apply this diff to modify the exception handling:
def log(msg: str, level: str): # ... try: # ... conn.send({"name": NODE_NAME, "msg": msg, "level": level}) conn.close() success = True - except BaseException as e: + except Exception as e: error = e
Line range hint
56-59: Avoid catchingBaseException; catchExceptioninsteadIn the
start_debuggerfunction, catchingBaseExceptioncan suppress important exceptions. UseExceptionto catch standard errors without interfering with system-exiting exceptions.Apply this diff to modify the exception handling:
# ... if debugger_spec is not None: try: import debugpy # ... - except BaseException as error: + except Exception as error: logerr(f"Failed to start debugger: {error}")
Line range hint
106-108: Avoid catchingBaseException; catchExceptioninsteadCatching
BaseExceptionin themainfunction may prevent the program from being interrupted gracefully. It's advisable to catchExceptionto handle errors appropriately.Apply this diff to modify the exception handling:
try: run_module_at(target_type_path) - except BaseException as error: + except Exception as error: logfatal(f"Failed to run node {debug_node}: {error}") raise error
Line range hint
15-15: Avoid using global variables; consider refactoringNODE_NAMEUsing the global variable
NODE_NAMEcan lead to code that's harder to maintain and debug. Consider passingNODE_NAMEas a parameter to functions that need it or encapsulating related functionality within a class.Also applies to: 83-85
Line range hint
70-73: Be cautious when modifyingsys.pathAppending to
sys.pathcan cause module conflicts and affect module resolution globally. Instead, consider usingimportlibto load modules directly without alteringsys.path.Apply this diff to use
importlibfor module loading:def run_module_at(path: str): """Runs a python module based on its file path""" - basename = os.path.basename(path) - module_dir = os.path.dirname(path) - module_name = os.path.splitext(basename)[0] - sys.path.append(module_dir) - runpy.run_module(module_name, run_name="__main__") + spec = importlib.util.spec_from_file_location("module.name", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module)
Line range hint
91-91: Fix argument parsing for--debug_waitUsing
type=boolwithargparsecan lead to unexpected behavior because any non-empty string evaluates toTrue. Instead, useaction='store_true'to handle boolean flags correctly.Apply this diff to fix the argument parsing:
parser.add_argument("--debug_node", required=True, type=str) parser.add_argument("--debug_port", required=False, type=int) parser.add_argument("--debug_host", default=default_host, type=str) -parser.add_argument("--debug_wait", default=False, type=bool) +parser.add_argument("--debug_wait", action="store_true")
Line range hint
103-105: Adjust the formatting of the multiline string inlogerrThe multiline string passed to
logerrmay include unintended indentation in the log output. Consider using a single-line string or dedenting the text to ensure proper formatting in logs.Apply this diff to adjust the message formatting:
else: - logerr( - """Missing parameter to start debugger: --debug_port - Add it in the launch configuration""" - ) + logerr("Missing parameter to start debugger: --debug_port. Add it in the launch configuration.")code/planning/src/debug_wrapper.py (4)
Line range hint
78-81: Incomplete docstring instart_debuggerfunctionThe docstring for the
start_debuggerfunction is incomplete; it contains only"_summary_". Please provide a meaningful description of the function's purpose and how to use it.
Line range hint
118-118: Incorrect handling of the--debug_waitargumentUsing
type=boolwithargparsedoes not parse boolean command-line arguments as intended. It's better to useaction='store_true'to properly handle boolean flags.Apply the following diff to fix the argument parsing:
-parser.add_argument("--debug_wait", default=False, type=bool) +parser.add_argument("--debug_wait", action='store_true', default=False)
Line range hint
42-47: Avoid catchingBaseException; useExceptioninsteadCatching
BaseExceptionis not recommended because it includes system-exiting exceptions likeSystemExitandKeyboardInterrupt. Replaceexcept BaseException as ewithexcept Exception as eto prevent unintended behavior.Apply this change to all relevant
exceptblocks:- except BaseException as e: + except Exception as e:Also applies to: 83-88, 146-148
Line range hint
63-65: Avoid modifyingsys.pathgloballyAppending to
sys.pathglobally can lead to module conflicts and unexpected behavior. Consider loading the module without alteringsys.pathusingimportlib.Here's an alternative implementation:
-def run_module_at(path: str): - basename = os.path.basename(path) - module_dir = os.path.dirname(path) - module_name = os.path.splitext(basename)[0] - sys.path.append(module_dir) - runpy.run_module(module_name, run_name="__main__") +def run_module_at(path: str): + spec = importlib.util.spec_from_file_location("__main__", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module)code/debug_wrapper.py (5)
24-25: Complete the description for--debug_portin the docstring.The description for
--debug_portis incomplete and ends abruptly. Please provide the missing information to clarify its usage.
120-128: Provide a detailed docstring forstart_debuggerfunction.The docstring currently contains a placeholder
"_summary_". Please replace it with a meaningful description of the function's purpose and behavior.
31-35: Clarify exception types in theRaisessection of the docstring.In the
Raisessection,erroris not a specific exception type. Please specify the exact exceptions that can be raised, such asArgumentParserErrorandException.Suggested revision:
Raises: ArgumentParserError: Missing required parameters. - error: If the started debug_node raises an exception, - it is logged and then raised again. + Exception: If the started `debug_node` raises an exception, + it is logged and then re-raised.
139-139: Correct the typo in the comment: 'catched' should be 'caught'.There's a typographical error in the comment. Replace "catched" with "caught" for proper grammar.
Apply this diff to correct the typo:
- # Yes, all exceptions should be catched and sent into rosconsole + # Yes, all exceptions should be caught and sent into rosconsole
171-172: Avoid using the global variableNODE_NAME; pass it as a parameter instead.Using global variables can lead to unintended side effects and makes the code harder to maintain. Consider passing
NODE_NAMEas an argument to functions that require it.Example refactor:
- Remove the global declaration and pass
NODE_NAMEto functions likelog.- global NODE_NAME NODE_NAME = debug_node
- Modify the
logfunction to acceptnode_nameas a parameter.- def log(msg: str, level: str): + def log(msg: str, level: str, node_name: str):
- Update all calls to
logaccordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
.vscode/launch.json(1 hunks)build/agent_service.yaml(1 hunks)build/docker-compose.leaderboard.yaml(1 hunks)build/docker/agent/Dockerfile(1 hunks)code/acting/src/debug_wrapper.py(1 hunks)code/agent/launch/agent.launch(1 hunks)code/debug_wrapper.py(1 hunks)code/debugging/CMakeLists.txt(1 hunks)code/debugging/launch/debugging.launch(1 hunks)code/debugging/package.xml(1 hunks)code/debugging/setup.py(1 hunks)code/debugging/src/debug_logger.py(1 hunks)code/mock/src/debug_wrapper.py(1 hunks)code/perception/launch/perception.launch(1 hunks)code/perception/src/debug_wrapper.py(1 hunks)code/planning/src/debug_wrapper.py(1 hunks)code/requirements.txt(1 hunks)doc/development/debugging.md(1 hunks)pyrightconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- code/debugging/CMakeLists.txt
- code/debugging/launch/debugging.launch
- code/debugging/package.xml
- code/debugging/setup.py
- pyrightconfig.json
🧰 Additional context used
🪛 Biome
.vscode/launch.json
[error] 22-23: Expected a property but instead found '}'.
Expected a property here.
(parse)
🪛 Ruff
code/debugging/src/debug_logger.py
87-88: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 LanguageTool
doc/development/debugging.md
[uncategorized] ~6-~6: This expression is usually spelled with a hyphen.
Context: ...ilities](#debugging-possibilities) - Message based debugging ...
(BASED_HYPHEN)
[uncategorized] ~8-~8: This expression is usually spelled with a hyphen.
Context: ...iewing-the-messages) - [Problems of message based debugging](#problems-of-message-based-d...
(BASED_HYPHEN)
[grammar] ~10-~10: This phrase is duplicated. You should probably use “required once” only once.
Context: ...](#vs-code-debugger) - Setup steps required once - [Required once for the node you want to debug](#requir...
(PHRASE_REPETITION)
[uncategorized] ~21-~21: This expression is usually spelled with a hyphen.
Context: ...s: Message based and VS Code based ### Message based debugging Messages can be logged into ...
(BASED_HYPHEN)
[formatting] ~26-~26: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ic. rospy.logdebug is not recommended, because these messages are not published to /ro...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~54-~54: This expression is usually spelled with a hyphen.
Context: ...spy/Overview/Logging) #### Problems of message based debugging - Time intensive and cumbers...
(BASED_HYPHEN)
[formatting] ~76-~76: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...* can be a port in range 53000-53100, because those are exposed from the [leaderboard...
(COMMA_BEFORE_BECAUSE)
[style] ~98-~98: As a shorter alternative for ‘able to’, consider using “can then only”.
Context: ...When you attach the debugger later, you are then only able to debug the callbacks the node receives. ...
(BE_ABLE_TO)
🔇 Additional comments (13)
code/requirements.txt (2)
19-19: LGTM! Version 1.8.7 is appropriate.
The pinned version of debugpy is recent and stable, ensuring consistent debugging behavior across development environments.
19-19: Security Reminder: Ensure debug ports are properly secured.
Since debugpy opens debug ports for remote debugging, ensure that the exposed ports (53000-53100) are:
- Only accessible in development environments
- Not exposed in production deployments
- Protected by appropriate network security measures
Let's verify the debug port configuration:
.vscode/launch.json (2)
8-14: LGTM! Debug configuration is well-structured.
The debug configuration correctly sets up debugpy in attach mode with appropriate port settings that align with the Docker compose configuration.
17-18: Verify environment variable setup.
The path mapping relies on PAF_CATKIN_CODE_ROOT. Let's verify this environment variable is consistently set across the development environment.
✅ Verification successful
Environment variable setup verified in Docker environment
The environment variable PAF_CATKIN_CODE_ROOT is properly defined in the Docker environment:
- Set to
/catkin_ws/srcinbuild/docker/agent/Dockerfile - This matches the expected path mapping in the VS Code launch configuration
The variable is consistently defined in the Docker build environment, which ensures the debugger's path mapping will work correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PAF_CATKIN_CODE_ROOT environment variable setup
# Test: Search for PAF_CATKIN_CODE_ROOT in Dockerfiles and compose files
echo "Checking Dockerfile definitions:"
rg "PAF_CATKIN_CODE_ROOT" -A 2 "build/docker"
echo -e "\nChecking docker-compose environment definitions:"
rg "PAF_CATKIN_CODE_ROOT" -A 2 "build/*.yaml"
Length of output: 492
Script:
#!/bin/bash
# Let's check for docker-compose files with .yml extension and environment variables
echo "Checking docker-compose environment definitions (yml):"
rg "PAF_CATKIN_CODE_ROOT" -A 2 "build/*.yml"
# Let's also check for any other potential environment variable definitions
echo -e "\nChecking for environment files:"
fd -t f ".env" -x cat {}
# Check for any other files that might reference this environment variable
echo -e "\nChecking other potential configuration files:"
rg "PAF_CATKIN_CODE_ROOT" --type-not dockerfile
Length of output: 511
code/agent/launch/agent.launch (1)
24-26: LGTM! Verify debugging.launch existence.
The integration of debugging launch file follows the established pattern and is properly positioned after other components.
Let's verify the debugging.launch file exists:
✅ Verification successful
Launch file integration verified successfully
The debugging.launch file exists at the correct location in code/debugging/launch/debugging.launch, and the package structure is properly set up with the required components (debug_logger.py in src). The integration in agent.launch is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the debugging launch file exists
# Expected: File exists at the specified path
# Test 1: Check if the debugging package exists
if ! fd -t d "^debugging$"; then
echo "Warning: debugging package not found"
exit 1
fi
# Test 2: Check if the launch file exists
fd -t f "debugging.launch" -p "debugging/launch"
Length of output: 190
Script:
#!/bin/bash
# Let's check if the launch file exists in the correct path relative to repository root
fd -t f "debugging.launch" -p "code/debugging/launch"
# Also check the directory structure to understand the layout
ls -R code/debugging/
Length of output: 256
build/agent_service.yaml (2)
33-33: Verify port configuration for debugger connectivity.
The debugger requires exposed ports to function properly. According to the AI summary, ports 53000-53100 should be reserved in docker-compose.leaderboard.yaml, but no port mapping is visible in this file.
Let's verify the port configuration across files:
#!/bin/bash
# Description: Verify debugger port configuration
# Expected: Find port range 53000-53100 in docker compose files
# Search for port configuration in docker-compose files
echo "Checking docker-compose files for debug ports:"
rg "53[0-9]{3}" $(fd -e yaml -e yml | rg "docker-compose|service")33-33: LGTM with security consideration.
The addition of DEBUG_WRAPPER_DEFAULT_HOST=0.0.0.0 aligns with the debugging enhancement objectives. However, be aware that allowing connections from any IP address (0.0.0.0) should only be used in development environments.
Let's verify if this configuration is properly isolated to development environments:
✅ Verification successful
Debug host configuration is properly isolated and secure
The verification shows that:
DEBUG_WRAPPER_DEFAULT_HOSTis only used in development context (agent_service.yaml and debug_wrapper.py)- The default host is "localhost" if the environment variable is not set
- The configuration is not present in any production files
- The debug ports (53000-53100) are properly configured in the leaderboard compose file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the debug host configuration appears in production files
# Expected: No matches in production-related files
# Search for DEBUG_WRAPPER_DEFAULT_HOST in files containing 'prod' or 'production'
rg "DEBUG_WRAPPER_DEFAULT_HOST" -l | rg -i "prod"
Length of output: 602
Script:
#!/bin/bash
# Let's check the context of how DEBUG_WRAPPER_DEFAULT_HOST is used across all files
rg "DEBUG_WRAPPER_DEFAULT_HOST" -B 2 -A 2
# Also check if there are any environment-specific configurations
fd -e yaml -e yml | rg -i 'env|environment' | xargs rg "DEBUG_WRAPPER" -l
Length of output: 1292
code/acting/src/debug_wrapper.py (1)
1-1:
Remove the relative path from the first line.
The relative path "../../debug_wrapper.py" at the beginning of the file seems out of place and could cause issues with file resolution.
-../../debug_wrapper.pyLikely invalid or redundant comment.
code/mock/src/debug_wrapper.py (1)
1-1:
Remove the relative path from the first line.
The first line contains a relative path that seems out of place. This line should be removed as it could cause confusion and doesn't serve any purpose in the code.
-../../debug_wrapper.pyLikely invalid or redundant comment.
doc/development/debugging.md (3)
1-136: Documentation successfully achieves PR objectives.
The documentation provides a comprehensive guide for both message-based and VS Code debugging, aligning perfectly with the PR's goal of enhancing debugging capabilities. The structure is clear, examples are practical, and setup instructions are detailed.
While some minor improvements have been suggested, the core content effectively guides users through the new debugging features.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: This expression is usually spelled with a hyphen.
Context: ...ilities](#debugging-possibilities) - Message based debugging ...
(BASED_HYPHEN)
[uncategorized] ~8-~8: This expression is usually spelled with a hyphen.
Context: ...iewing-the-messages) - [Problems of message based debugging](#problems-of-message-based-d...
(BASED_HYPHEN)
[grammar] ~10-~10: This phrase is duplicated. You should probably use “required once” only once.
Context: ...](#vs-code-debugger) - Setup steps required once - [Required once for the node you want to debug](#requir...
(PHRASE_REPETITION)
[uncategorized] ~21-~21: This expression is usually spelled with a hyphen.
Context: ...s: Message based and VS Code based ### Message based debugging Messages can be logged into ...
(BASED_HYPHEN)
[formatting] ~26-~26: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ic. rospy.logdebug is not recommended, because these messages are not published to /ro...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~54-~54: This expression is usually spelled with a hyphen.
Context: ...spy/Overview/Logging) #### Problems of message based debugging - Time intensive and cumbers...
(BASED_HYPHEN)
[formatting] ~76-~76: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...* can be a port in range 53000-53100, because those are exposed from the [leaderboard...
(COMMA_BEFORE_BECAUSE)
[style] ~98-~98: As a shorter alternative for ‘able to’, consider using “can then only”.
Context: ...When you attach the debugger later, you are then only able to debug the callbacks the node receives. ...
(BE_ABLE_TO)
73-74: Verify referenced debug files exist and are accessible.
The documentation references two critical files:
code/debug_wrapper.pycode/debugging/src/debug_logger.py
#!/bin/bash
# Verify the existence of referenced debug files
echo "Checking debug files..."
fd -t f "debug_wrapper.py|debug_logger.py"Also applies to: 103-103
76-77: Verify port range in Docker configuration.
The documentation specifies ports 53000-53100 should be exposed in the leaderboard Docker configuration.
✅ Verification successful
Port range correctly exposed in Docker configuration
The documentation accurately reflects the port range configuration. The docker-compose.leaderboard.yaml file exposes ports 53000-53100 exactly as documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the port range is properly exposed in Docker compose
echo "Checking Docker port configuration..."
rg -A 5 "53000-53100" "build/docker-compose.leaderboard.yaml"
Length of output: 252
Script:
#!/bin/bash
# Let's first check if the file exists and see its content
echo "Checking if docker-compose.leaderboard.yaml exists..."
fd "docker-compose.leaderboard.yaml"
# Let's also check for any port configurations in the range of 53000
echo -e "\nChecking for port configurations around 53000..."
rg "530[0-9]{2}" build/
Length of output: 402
🧰 Tools
🪛 LanguageTool
[formatting] ~76-~76: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...* can be a port in range 53000-53100, because those are exposed from the [leaderboard...
(COMMA_BEFORE_BECAUSE)
build/docker/agent/Dockerfile (1)
168-170: Environment variable added for debugging is appropriate
The addition of ENV PAF_CATKIN_CODE_ROOT=/catkin_ws/src aligns with the PR objectives to enhance debugging capabilities. This environment variable facilitates the VS Code debugger in correctly mapping the local workspace to the remote container, enabling seamless debugging of ROS nodes within the Docker environment.
| ], | ||
| } |
There was a problem hiding this comment.
Remove trailing comma to fix JSON syntax.
The trailing comma before the closing brace is causing a syntax error. This could prevent VS Code from properly loading the debug configuration.
Apply this fix:
"configurations": [
{
"name": "53000 leaderboard attach",
"type": "debugpy",
"request": "attach",
"connect": {
"host": "localhost",
"port": 53000
},
"pathMappings": [
{
"localRoot": "${workspaceFolder}/code",
"remoteRoot": "${env:PAF_CATKIN_CODE_ROOT}"
}
]
- }
- ],
+ }
+ ]
}📝 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.
| ], | |
| } | |
| ], | |
| } | |
| ``` | |
| ↓ | |
| ```suggestion | |
| ] | |
| } |
🧰 Tools
🪛 Biome
[error] 22-23: Expected a property but instead found '}'.
Expected a property here.
(parse)
| ports: | ||
| # Reserved ports for the debugger | ||
| - "53000-53100:53000-53100" |
There was a problem hiding this comment.
💡 Codebase verification
Port range can be safely reduced to a single debug port
Based on the codebase analysis:
- No specific requirements for multiple debug ports were found
- The only debug configurations found are using a single debug flag (
--debug=0) - The
DEBUG_WRAPPER_DEFAULT_HOSTsetting suggests a single host configuration
Recommend:
- Reduce the port range to a single port (e.g.,
- "53000:53000") - Document the purpose of the debug port in a comment
🔗 Analysis chain
Consider reducing the exposed port range for security.
While exposing ports for debugging is necessary, the current range of 100 ports (53000-53100) seems excessive for debugging purposes. Consider limiting this to a smaller range unless there's a specific requirement for multiple debug sessions.
Let's verify if there are any specific port requirements in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for debugpy port configurations in the codebase
rg -l "connect\(.*53\d{3}\)" || rg -l "listen\(.*53\d{3}\)"
Length of output: 128
Script:
#!/bin/bash
# Search for debugger-related configurations and ports in various file types
rg -i "debug.*port|debugger.*port|port.*debug" --type yaml --type python --type javascript
# Search for VS Code launch configurations that might specify debug ports
fd -e json -x cat {} \; | grep -i "port.*53"
# Look for any documentation about debugging setup
fd "README|CONTRIBUTING" -t f -x grep -il "debug" {}
Length of output: 255
Script:
#!/bin/bash
# Check the content of the perception README for any debug port information
cat ./doc/perception/README.md
# Look for any VS Code launch configurations
fd -e vscode -d 2 -x cat {}
# Search for any debug-related configurations in yaml files
rg -i "debug" --type yaml
Length of output: 2369
| command: |- | ||
| bash -c "sleep 10 && sudo chown -R ${USER_UID}:${USER_GID} ../ && \ | ||
| sudo chmod -R a+w ../ && sudo mkdir -p $${XDG_RUNTIME_DIR} && sudo chmod 0700 $${XDG_RUNTIME_DIR} && sudo chown -R ${USER_UID}:${USER_GID} $${XDG_RUNTIME_DIR} && \ | ||
| (rqt_console &) && disown -a && \ | ||
| python3 /opt/leaderboard/leaderboard/leaderboard_evaluator.py --debug=0 --routes=$${ROUTE} --agent=/workspace/code/agent/src/agent/agent.py --host=$${CARLA_SIM_HOST} --track=MAP" |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Consider using service dependencies instead of sleep
Based on the analysis of the codebase, I'm modifying my previous recommendation. The docker-compose.cicd.yaml shows that the agent service properly depends on carla-simulator and roscore services. Instead of using a complex retry mechanism, we should leverage Docker Compose's built-in dependency handling:
command: |-
- bash -c "sleep 10 && sudo chown -R ${USER_UID}:${USER_GID} ../ && \
- sudo chmod -R a+w ../ && sudo mkdir -p $${XDG_RUNTIME_DIR} && sudo chmod 0700 $${XDG_RUNTIME_DIR} && sudo chown -R ${USER_UID}:${USER_GID} $${XDG_RUNTIME_DIR} && \
- (rqt_console &) && disown -a && \
- python3 /opt/leaderboard/leaderboard/leaderboard_evaluator.py --debug=0 --routes=$${ROUTE} --agent=/workspace/code/agent/src/agent/agent.py --host=$${CARLA_SIM_HOST} --track=MAP"
+ bash -c '
+ # Setup error handling
+ set -e
+
+ # Setup permissions
+ echo "Setting up permissions..."
+ sudo chown -R ${USER_UID}:${USER_GID} ../ || exit 1
+ sudo chmod -R a+w ../ || exit 1
+
+ # Setup runtime directory
+ echo "Setting up runtime directory..."
+ sudo mkdir -p $${XDG_RUNTIME_DIR} || exit 1
+ sudo chmod 0700 $${XDG_RUNTIME_DIR} || exit 1
+ sudo chown -R ${USER_UID}:${USER_GID} $${XDG_RUNTIME_DIR} || exit 1
+
+ # Start rqt_console in background
+ echo "Starting rqt_console..."
+ rqt_console &
+ disown -a
+
+ # Start the evaluator
+ echo "Starting leaderboard evaluator..."
+ exec python3 /opt/leaderboard/leaderboard/leaderboard_evaluator.py \
+ --debug=0 \
+ --routes=$${ROUTE} \
+ --agent=/workspace/code/agent/src/agent/agent.py \
+ --host=$${CARLA_SIM_HOST} \
+ --track=MAP
+ 'Add these service dependencies to ensure proper startup order:
depends_on:
- carla-simulator
- roscoreThis approach:
- Removes the arbitrary
sleep 10and relies on Docker's service orchestration - Maintains error handling for critical file operations
- Improves readability and logging
- Preserves the original functionality in a more robust way
🔗 Analysis chain
Enhance command robustness and readability.
The current command structure has several potential issues:
- The
sleep 10is a brittle way to handle timing dependencies - No error handling for critical operations (chmod, chown, mkdir)
- Multiple sudo operations without checking for failures
Consider refactoring to this more robust approach:
command: |-
- bash -c "sleep 10 && sudo chown -R ${USER_UID}:${USER_GID} ../ && \
- sudo chmod -R a+w ../ && sudo mkdir -p $${XDG_RUNTIME_DIR} && sudo chmod 0700 $${XDG_RUNTIME_DIR} && sudo chown -R ${USER_UID}:${USER_GID} $${XDG_RUNTIME_DIR} && \
- (rqt_console &) && disown -a && \
- python3 /opt/leaderboard/leaderboard/leaderboard_evaluator.py --debug=0 --routes=$${ROUTE} --agent=/workspace/code/agent/src/agent/agent.py --host=$${CARLA_SIM_HOST} --track=MAP"
+ bash -c '
+ # Setup error handling
+ set -e
+
+ # Function to retry operations with timeout
+ wait_for_service() {
+ local retries=30
+ local wait_time=2
+ while [ $retries -gt 0 ]; do
+ if [ -d "../" ]; then
+ return 0
+ fi
+ retries=$((retries-1))
+ sleep $wait_time
+ done
+ return 1
+ }
+
+ # Wait for services to be ready
+ echo "Waiting for services..."
+ wait_for_service || { echo "Service startup timeout"; exit 1; }
+
+ # Setup permissions
+ echo "Setting up permissions..."
+ sudo chown -R ${USER_UID}:${USER_GID} ../ || { echo "Failed to set ownership"; exit 1; }
+ sudo chmod -R a+w ../ || { echo "Failed to set permissions"; exit 1; }
+
+ # Setup runtime directory
+ echo "Setting up runtime directory..."
+ sudo mkdir -p $${XDG_RUNTIME_DIR} || { echo "Failed to create runtime dir"; exit 1; }
+ sudo chmod 0700 $${XDG_RUNTIME_DIR} || { echo "Failed to set runtime dir permissions"; exit 1; }
+ sudo chown -R ${USER_UID}:${USER_GID} $${XDG_RUNTIME_DIR} || { echo "Failed to set runtime dir ownership"; exit 1; }
+
+ # Start rqt_console in background
+ echo "Starting rqt_console..."
+ rqt_console &
+ disown -a
+
+ # Start the evaluator
+ echo "Starting leaderboard evaluator..."
+ exec python3 /opt/leaderboard/leaderboard/leaderboard_evaluator.py \
+ --debug=0 \
+ --routes=$${ROUTE} \
+ --agent=/workspace/code/agent/src/agent/agent.py \
+ --host=$${CARLA_SIM_HOST} \
+ --track=MAP
+ 'This refactored version:
- Replaces static sleep with dynamic service checking
- Adds proper error handling and logging
- Improves readability with structured sections
- Uses
execto replace shell with the Python process
Let's verify the service dependencies:
Let's search for any service dependencies and timing-related patterns in the build directory to better understand the startup requirements.
Let's examine the service dependencies and health check configurations to better understand the startup requirements and validate our recommendation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other services that might need to be ready before agent starts
rg -l "depends_on.*agent" build/
Length of output: 128
Script:
#!/bin/bash
# Check for service dependencies in docker-compose files
rg -l "depends_on" build/
# Check for any references to timing or startup order
rg "sleep|wait" build/
Length of output: 1258
Script:
#!/bin/bash
# Check the content of docker-compose.cicd.yaml to understand dependencies
cat build/docker-compose.cicd.yaml
# Check if there are any health checks defined
rg "healthcheck" build/
Length of output: 842
CodeRabbit explains it pretty well. My usage of BaseException was wrong in most cases so will be changed to Exception except for when the node is started. In this case I think it really makes sense to capture any reason the node exits. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
code/debugging/src/debug_logger.py (5)
48-70: Add type hints to improve code maintainabilityConsider adding type hints to the
levelparameter to explicitly show allowed values.-def log(name: str, msg: str, level: str): +def log(name: str, msg: str, level: Literal["debug", "info", "warn", "error", "fatal"]):Don't forget to add the import:
from typing import Literal
85-91: Simplify nested conditions and consider making timeout configurableThe nested if statements can be simplified, and the timeout value should be configurable.
- if conn.poll(timeout=2.0): - msg = conn.recv() - if isinstance(msg, str): - if msg.lower() == CLOSE_MSG: - running = False - msg = None + POLL_TIMEOUT = 2.0 # Consider moving to constants + if conn.poll(timeout=POLL_TIMEOUT) and isinstance(msg := conn.recv(), str) and msg.lower() == CLOSE_MSG: + running = False + msg = None🧰 Tools
🪛 Ruff
87-88: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
133-133: Make initialization delay configurableThe hardcoded sleep value should be configurable to accommodate different system requirements.
+ INIT_DELAY = float(os.getenv("DEBUG_LOGGER_INIT_DELAY", "5.0")) - time.sleep(5.0) + time.sleep(INIT_DELAY)Don't forget to add the import:
import os
140-150: Log malformed messages for debuggingConsider logging malformed messages to help diagnose communication issues.
log_msg = "Wrong log message format" log_name = "NAMERR" log_level = "fatal" + is_malformed = True if isinstance(msg, dict): if "name" in msg: log_name = msg["name"] if "msg" in msg: log_msg = msg["msg"] if "level" in msg: log_level = msg["level"] + is_malformed = False + if is_malformed: + eprint(f"Received malformed message: {msg}")
27-29: Consider additional security measures for IPC communicationWhile the current implementation uses an authentication key, consider:
- Using a more secure authentication mechanism
- Implementing message validation
- Adding rate limiting to prevent DoS attacks
Would you like assistance in implementing these security measures?
code/debug_wrapper.py (4)
1-35: Documentation improvements neededThe docstring is comprehensive but could be enhanced:
- Add return value documentation in the docstring
- Add specific exception types that could be raised
- Add version information and author metadata
"""Debug wrapper node Node that wraps a python ros node and is able to open a debugpy remote debugger instance. Logs any exceptions from the node and other information into the ros console via the debug_logger node. Always tries to at least start the node, even if dependencies like debugpy are missing. Usage: Already done for existing ros packages: symlink this file into the package. Example: `cd code/perception/src && ln -s ../../debug_wrapper.py debug_wrapper.py` Adjust the launch configuration to use the debug_wrapper.py instead of the node file and set the required args More info in [debugging.md](doc/development/debugging.md) Arguments: --debug_node: Required: The filename of the node to debug --debug_port: The port the debugger listens on. If not set, --debug_host: The host the debugger binds to. Defaults to the environment variable `DEBUG_WRAPPER_DEFAULT_HOST` if set, otherwise `localhost` --debug_wait: If True, the wrapper waits until a client (VS Code) is connected and only then starts node execution Raises: ArgumentParserError: Missing required parameters error: If the started debug_node raises an exception, it is logged and then raised again + +Returns: + None + +Exceptions: + ImportError: If debugpy module is not found + ConnectionError: If debug_logger connection fails + +Version: 0.1 +Author: CodeRabbit Inc. """
38-47: Organize imports according to PEP 8The imports should be grouped in the following order:
- Standard library imports
- Related third party imports
- Local application/library specific imports
-import importlib.util -import os -import runpy -import sys -import argparse -import time -from multiprocessing.connection import Client - -import rospy +# Standard library imports +import argparse +import importlib.util +import os +import runpy +import sys +import time +from multiprocessing.connection import Client + +# Third party imports +import rospy
62-86: Improve error handling in log functionThe log function has several areas for improvement:
- Hardcoded timeout value
- No type hints for the level parameter
- No validation for log levels
-def log(msg: str, level: str): +from typing import Literal + +LogLevel = Literal["debug", "info", "warn", "error", "fatal"] + +# Constants +LOG_TIMEOUT_SECONDS = 5.0 + +def log(msg: str, level: LogLevel): """Log msg via the debug_logger node Args: msg (str): Log message - level (str): Log level. One of (debug), info, warn, error, fatal. + level (LogLevel): Log level. One of (debug), info, warn, error, fatal. debug level not recommended, because these are not published to /rosout """ error = None success = False start_time = time.monotonic() - while not success and start_time + 5.0 > time.monotonic(): + while not success and start_time + LOG_TIMEOUT_SECONDS > time.monotonic(): try: address = ("localhost", 52999) conn = Client(address, authkey=b"debug_logger")
117-143: Complete the docstring for start_debugger functionThe docstring is incomplete and missing return type hint.
-def start_debugger( +def start_debugger( node_module_name: str, host: str, port: int, wait_for_client: bool = False -): +) -> None: - """_summary_ + """Initialize and start the debugpy debugger. + + Attempts to start a debugpy debugging session on the specified host and port. + If debugpy is not available or fails to start, appropriate error messages are logged. Args: node_module_name (str): Name of the underlying node. Only used for logging host (str): host the debugger binds to port (int): debugger port wait_for_client (bool, optional): If the debugger should wait for a client to attach. Defaults to False. + + Returns: + None + + Raises: + ImportError: If debugpy module is not found + Exception: If debugger fails to start """doc/development/debugging.md (3)
24-29: Enhance logging documentation with severity levels and storage details.Consider adding:
- Explanation of log levels severity and when to use each
- Information about log rotation and storage limits
Add this section after line 29:
The log levels follow this severity hierarchy: - DEBUG: Detailed information for debugging - INFO: General information about program execution - WARN: Warning messages for potentially harmful situations - ERROR: Error messages for serious problems - FATAL: Critical errors that may prevent the system from running Note: Log files are automatically rotated when they reach 100MB. By default, ROS keeps the last 5 log files.🧰 Tools
🪛 LanguageTool
[formatting] ~27-~27: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ic.rospy.logdebugis not recommended, because these messages are not published to /ro...(COMMA_BEFORE_BECAUSE)
106-110: Document debugger timeout settings.Add information about connection timeouts and how to adjust them for slower systems.
Add after line 110:
Note: By default, the debugger waits 30 seconds for a connection. For slower systems, you can increase this timeout: ```python # In debug_wrapper.py debugpy.listen(("0.0.0.0", port)) debugpy.wait_for_client(timeout=60) # Increase timeout to 60 seconds--- `134-140`: **Add cleanup instructions for failed builds.** Consider adding cleanup steps for when builds fail or containers need to be reset. Add before line 134: ```markdown ### Cleanup (if needed) If you encounter build issues, clean up the environment first: ```bash # Stop and remove containers docker compose -f ./docker-compose.leaderboard.yaml down docker compose -f ./docker-compose.dev.yaml down # Remove old images docker image prune -f # Clean build cache docker builder prune -f</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 95912ca96c153cdc706758ce02024824d8cd6d98 and 2302dc9d968cba94a4c22d367b4c161555cbea6c. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `build/docker/agent/Dockerfile` (1 hunks) * `code/debug_wrapper.py` (1 hunks) * `code/debugging/src/debug_logger.py` (1 hunks) * `doc/development/debugging.md` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * build/docker/agent/Dockerfile </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Learnings (1)</summary> <details> <summary>code/debug_wrapper.py (1)</summary>Learnt from: Zelberor
PR: #478
File: code/debug_wrapper.py:165-165
Timestamp: 2024-11-12T13:46:38.887Z
Learning: In the Python ROS nodedebug_wrapper.py(incode/debug_wrapper.py), the--debug_portargument is optional, and the node is designed to start even when the port is not provided.</details> </details><details> <summary>🪛 Ruff</summary> <details> <summary>code/debugging/src/debug_logger.py</summary> 87-88: Use a single `if` statement instead of nested `if` statements Combine `if` statements using `and` (SIM102) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>doc/development/debugging.md</summary> [uncategorized] ~6-~6: This expression is usually spelled with a hyphen. Context: ...ilities](#debugging-possibilities) - [Message based debugging](#message-based-debugging) ... (BASED_HYPHEN) --- [uncategorized] ~8-~8: This expression is usually spelled with a hyphen. Context: ...iewing-the-messages) - [Problems of message based debugging](#problems-of-message-based-d... (BASED_HYPHEN) --- [grammar] ~10-~10: This phrase is duplicated. You should probably use “required once” only once. Context: ...](#vs-code-debugger) - [Setup steps required once](#setup-steps-required-once) - [Required once for the node you want to debug](#requir... (PHRASE_REPETITION) --- [uncategorized] ~22-~22: This expression is usually spelled with a hyphen. Context: ...s: Message based and VS Code based ### Message based debugging Messages can be logged into ... (BASED_HYPHEN) --- [formatting] ~27-~27: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause. Context: ...ic. `rospy.logdebug` is not recommended, because these messages are not published to /ro... (COMMA_BEFORE_BECAUSE) --- [uncategorized] ~55-~55: This expression is usually spelled with a hyphen. Context: ...spy/Overview/Logging) #### Problems of message based debugging - Time intensive and cumbers... (BASED_HYPHEN) --- [formatting] ~77-~77: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause. Context: ...* can be a port in range **53000-53100**, because those are exposed from the [leaderboard... (COMMA_BEFORE_BECAUSE) --- [style] ~99-~99: As a shorter alternative for ‘able to’, consider using “can then only”. Context: ...When you attach the debugger later, you are then only able to debug the callbacks the node receives. ... (BE_ABLE_TO) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| def run_module_at(path: str): | ||
| """Runs a python module based on its file path | ||
|
|
||
| Args: | ||
| path (str): python file path to run | ||
| """ | ||
| basename = os.path.basename(path) | ||
| module_dir = os.path.dirname(path) | ||
| module_name = os.path.splitext(basename)[0] | ||
| sys.path.append(module_dir) | ||
| runpy.run_module(module_name, run_name="__main__") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle sys.path modifications safely
The current implementation modifies sys.path without cleanup, which could lead to unintended side effects.
def run_module_at(path: str):
"""Runs a python module based on its file path
Args:
path (str): python file path to run
"""
basename = os.path.basename(path)
module_dir = os.path.dirname(path)
module_name = os.path.splitext(basename)[0]
+ original_path = sys.path.copy()
sys.path.append(module_dir)
- runpy.run_module(module_name, run_name="__main__")
+ try:
+ runpy.run_module(module_name, run_name="__main__")
+ finally:
+ sys.path = original_path📝 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.
| def run_module_at(path: str): | |
| """Runs a python module based on its file path | |
| Args: | |
| path (str): python file path to run | |
| """ | |
| basename = os.path.basename(path) | |
| module_dir = os.path.dirname(path) | |
| module_name = os.path.splitext(basename)[0] | |
| sys.path.append(module_dir) | |
| runpy.run_module(module_name, run_name="__main__") | |
| def run_module_at(path: str): | |
| """Runs a python module based on its file path | |
| Args: | |
| path (str): python file path to run | |
| """ | |
| basename = os.path.basename(path) | |
| module_dir = os.path.dirname(path) | |
| module_name = os.path.splitext(basename)[0] | |
| original_path = sys.path.copy() | |
| sys.path.append(module_dir) | |
| try: | |
| runpy.run_module(module_name, run_name="__main__") | |
| finally: | |
| sys.path = original_path |
| 1. Make sure the docker images are up-to-date: [Rebuild docker containers](#rebuild-docker-containers) | ||
| 2. Start the `build/docker-compose.dev.yaml` containers and attach VS Code to the **build-agent-dev** container. Always use the VS Code remote for debugging | ||
| 3. Make sure the recommended extensions are installed in the VS Code remote. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add setup validation steps.
Consider adding validation steps to ensure the debugging environment is correctly configured:
Add after line 71:
4. Validate your setup:
- Check that no other services are using the debug ports (53000-53100)
- Verify VS Code can connect to the container:
```bash
docker exec build-agent-dev ls /
```
- Confirm debugpy installation:
```python
python3 -c "import debugpy; print(debugpy.__version__)"
```
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
code/debugging/src/debug_logger.py (5)
27-29: Consider making the authentication key configurable.The hardcoded authentication key (
AUTHKEY) could pose a security risk. Consider making it configurable through environment variables or a secure configuration file.-AUTHKEY = b"debug_logger" +AUTHKEY = os.getenv('DEBUG_LOGGER_AUTH_KEY', b'debug_logger').encode()
48-70: Improve log level validation and default behavior.The current implementation silently defaults to fatal level for unknown log levels, which might mask configuration errors.
+VALID_LOG_LEVELS = {'debug', 'info', 'warn', 'error', 'fatal'} + def log(name: str, msg: str, level: str): msg = f"[debug_logger for {name}]: {msg}" level = level.lower() + if level not in VALID_LOG_LEVELS: + rospy.logwarn(f"Invalid log level '{level}' specified, defaulting to 'error'") + level = 'error' + if level == "debug": rospy.logdebug(msg) elif level == "info": rospy.loginfo(msg) elif level == "warn": rospy.logwarn(msg) elif level == "error": rospy.logerr(msg) - else: + elif level == "fatal": rospy.logfatal(msg)
85-91: Simplify nested conditions as suggested by static analysis.The nested if statements can be combined for better readability.
- if conn.poll(timeout=2.0): - msg = conn.recv() - if isinstance(msg, str): - if msg.lower() == CLOSE_MSG: - running = False - msg = None + if conn.poll(timeout=2.0) and (msg := conn.recv()) and isinstance(msg, str) and msg.lower() == CLOSE_MSG: + running = False + msg = None🧰 Tools
🪛 Ruff
87-88: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
107-113: Add cleanup status logging.The cleanup function should log its status to help with debugging shutdown issues.
def exit_cleanup(signum=None, frame=None): + eprint("Starting cleanup process...") close_listener() if LISTENER_THREAD is not None: LISTENER_THREAD.join() global NODE_RUNNING NODE_RUNNING = False + eprint("Cleanup completed")
136-151: Optimize message processing loop.The current implementation polls the message queue every 0.5 seconds, which might not be optimal for all use cases.
- Consider making the polling interval configurable:
+MESSAGE_POLL_INTERVAL = float(os.getenv('DEBUG_LOGGER_POLL_INTERVAL', '0.5'))
- Consider using a condition variable instead of polling:
- time.sleep(0.5) + time.sleep(MESSAGE_POLL_INTERVAL)
- Consider processing messages in batches for better performance when there are many messages:
def process_messages(messages): for msg in messages: log_msg = "Wrong log message format" log_name = "NAMERR" log_level = "fatal" if isinstance(msg, dict): log_name = msg.get("name", log_name) log_msg = msg.get("msg", log_msg) log_level = msg.get("level", log_level) log(log_name, log_msg, log_level)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
code/debugging/src/debug_logger.py(1 hunks)
🧰 Additional context used
🪛 Ruff
code/debugging/src/debug_logger.py
87-88: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🔇 Additional comments (2)
code/debugging/src/debug_logger.py (2)
1-17: LGTM! Well-documented module purpose and implementation details.
The documentation clearly explains the purpose of the debug logger and its integration with the debug wrapper.
37-46: Acknowledged: Duplicate eprint function
As discussed in previous comments, while the eprint function is duplicated, the current approach is acceptable given the symlink constraints.
code/debugging/src/debug_logger.py
Outdated
| if conn.poll(timeout=2.0): | ||
| msg = conn.recv() | ||
| if isinstance(msg, str): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for message reception.
The message reception lacks error handling which could lead to unhandled exceptions.
if conn.poll(timeout=2.0):
- msg = conn.recv()
+ try:
+ msg = conn.recv()
+ except EOFError as e:
+ eprint(f"Error receiving message: {e}")
+ msg = None📝 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.
| if conn.poll(timeout=2.0): | |
| msg = conn.recv() | |
| if isinstance(msg, str): | |
| if conn.poll(timeout=2.0): | |
| try: | |
| msg = conn.recv() | |
| except EOFError as e: | |
| eprint(f"Error receiving message: {e}") | |
| msg = None | |
| if isinstance(msg, str): |
| # We need to wait a bit until the node is fully initialized to send log messages | ||
| time.sleep(5.0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded sleep with proper initialization check.
The 5-second sleep is a brittle way to wait for node initialization.
- # We need to wait a bit until the node is fully initialized to send log messages
- time.sleep(5.0)
+ # Wait for node to be fully initialized
+ retry_count = 0
+ while not rospy.is_initialized() and retry_count < 10:
+ time.sleep(0.5)
+ retry_count += 1
+ if not rospy.is_initialized():
+ eprint("Failed to initialize ROS node after 5 seconds")
+ exit_cleanup()
+ exit(1)📝 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.
| # We need to wait a bit until the node is fully initialized to send log messages | |
| time.sleep(5.0) | |
| # Wait for node to be fully initialized | |
| retry_count = 0 | |
| while not rospy.is_initialized() and retry_count < 10: | |
| time.sleep(0.5) | |
| retry_count += 1 | |
| if not rospy.is_initialized(): | |
| eprint("Failed to initialize ROS node after 5 seconds") | |
| exit_cleanup() | |
| exit(1) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
code/debugging/src/debug_logger.py (4)
48-70: Enhance error handling in the log functionThe log function defaults to fatal level without warning when an invalid level is provided. Consider adding input validation and using a more appropriate default level.
def log(name: str, msg: str, level: str): + valid_levels = {"debug", "info", "warn", "error", "fatal"} + if level.lower() not in valid_levels: + rospy.logwarn(f"[debug_logger]: Invalid log level '{level}', defaulting to 'error'") + level = "error" + msg = f"[debug_logger for {name}]: {msg}" level = level.lower()
88-92: Simplify nested conditionsThe nested if statements can be combined for better readability.
- if isinstance(msg, str): - if msg.lower() == CLOSE_MSG: - running = False - msg = None + if isinstance(msg, str) and msg.lower() == CLOSE_MSG: + running = False + msg = None🧰 Tools
🪛 Ruff
88-89: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
103-110: Specify exception types in close_listenerThe broad exception handling could mask important issues. Consider catching specific exceptions.
def close_listener(): try: conn = Client(ADDRESS, authkey=AUTHKEY) conn.send(CLOSE_MSG) conn.close() - except Exception: + except (ConnectionRefusedError, ConnectionError) as e: + eprint(f"Failed to close listener: {e}") pass
141-156: Optimize message processing loopThe current implementation processes messages one at a time with frequent lock acquisitions. Consider processing messages in batches to reduce lock contention.
while NODE_RUNNING: with MESSAGES_MUTEX: - while len(MESSAGES) > 0: - msg = MESSAGES.pop(0) + # Process all messages in queue at once + messages_to_process = MESSAGES.copy() + MESSAGES.clear() + + # Process messages outside the lock + for msg in messages_to_process: log_msg = "Wrong log message format" log_name = "NAMERR" log_level = "fatal"code/debug_wrapper.py (3)
24-28: Improve docstring clarity for arguments and exceptionsThe docstring could be more specific:
- The
--debug_portdescription is incomplete- The
--debug_hostdescription should clarify the default value format- The
errorin raises section should specify the exception typeApply this diff:
- --debug_port: The port the debugger listens on. If not set, + --debug_port: The port the debugger listens on. Optional. --debug_host: The host the debugger binds to. Defaults to the environment variable `DEBUG_WRAPPER_DEFAULT_HOST` if set, - otherwise `localhost` + otherwise defaults to 'localhost' - error: If the started debug_node raises an exception, - it is logged and then raised again + Exception: If the started debug_node raises an exception, + it is logged and then re-raisedAlso applies to: 33-34
120-128: Complete the debugger function docstringThe docstring has a placeholder and missing information:
- Replace
_summary_with a proper description- Add a Returns section
Apply this diff:
- """_summary_ + """Starts a debugpy debugging server for remote debugging. + + If debugpy is not available, logs an error but allows the node to start. + If the debugger fails to start, logs an error but allows the node to start. Args: node_module_name (str): Name of the underlying node. Only used for logging host (str): host the debugger binds to port (int): debugger port wait_for_client (bool, optional): If the debugger should wait for a client to attach. Defaults to False. + + Returns: + None """
184-187: Improve error message formattingThe error message for missing debug port has unnecessary indentation and could be more informative.
Apply this diff:
- logerr( - """Missing parameter to start debugger: --debug_port - Add it in the launch configuration""" - ) + logerr( + "Debug port not specified. Add --debug_port in the launch configuration " + "if you want to enable debugging." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
build/docker/agent/Dockerfile(1 hunks)code/debug_wrapper.py(1 hunks)code/debugging/src/debug_logger.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build/docker/agent/Dockerfile
🧰 Additional context used
📓 Learnings (1)
code/debug_wrapper.py (1)
Learnt from: Zelberor
PR: una-auxme/paf#478
File: code/debug_wrapper.py:165-165
Timestamp: 2024-11-12T13:46:38.887Z
Learning: In the Python ROS node `debug_wrapper.py` (in `code/debug_wrapper.py`), the `--debug_port` argument is optional, and the node is designed to start even when the port is not provided.
🪛 Ruff
code/debugging/src/debug_logger.py
88-89: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🔇 Additional comments (2)
code/debugging/src/debug_logger.py (1)
1-35: Well-structured initialization and documentation!
The file header provides clear documentation of the purpose, message format, and use case. The imports and global constants are appropriately organized.
code/debug_wrapper.py (1)
134-134: Consider security implications of exposed debug ports
The debugger is listening on a network port without authentication. While this might be acceptable in a development environment, it's important to ensure that:
- The debug port is not exposed in production
- The host binding is restricted appropriately
Run this script to check if the debug ports are exposed in production configurations:
| try: | ||
| conn = listener.accept() | ||
| print(f"[debug_logger]: connection accepted from {listener.last_accepted}") | ||
| msg = None | ||
| if conn.poll(timeout=2.0): | ||
| msg = conn.recv() | ||
| if isinstance(msg, str): | ||
| if msg.lower() == CLOSE_MSG: | ||
| running = False | ||
| msg = None | ||
| conn.close() | ||
| except Exception as error: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure connection is always closed
Move the connection close call to a finally block to ensure it's always executed.
while running:
try:
conn = listener.accept()
print(f"[debug_logger]: connection accepted from {listener.last_accepted}")
msg = None
if conn.poll(timeout=2.0):
msg = conn.recv()
if isinstance(msg, str) and msg.lower() == CLOSE_MSG:
running = False
msg = None
- conn.close()
except Exception as error:
eprint(f"Failed to receive message: {error}")
continue
+ finally:
+ conn.close()📝 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.
| try: | |
| conn = listener.accept() | |
| print(f"[debug_logger]: connection accepted from {listener.last_accepted}") | |
| msg = None | |
| if conn.poll(timeout=2.0): | |
| msg = conn.recv() | |
| if isinstance(msg, str): | |
| if msg.lower() == CLOSE_MSG: | |
| running = False | |
| msg = None | |
| conn.close() | |
| except Exception as error: | |
| try: | |
| conn = listener.accept() | |
| print(f"[debug_logger]: connection accepted from {listener.last_accepted}") | |
| msg = None | |
| if conn.poll(timeout=2.0): | |
| msg = conn.recv() | |
| if isinstance(msg, str) and msg.lower() == CLOSE_MSG: | |
| running = False | |
| msg = None | |
| except Exception as error: | |
| eprint(f"Failed to receive message: {error}") | |
| continue | |
| finally: | |
| conn.close() |
🧰 Tools
🪛 Ruff
88-89: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
There was a problem hiding this comment.
What happens if the connection is not closed?
There was a problem hiding this comment.
@ll7, if the connection is not closed after each client interaction, it can lead to resource leaks in the application. Specifically, open connections may remain active indefinitely, consuming system resources like file descriptors and network sockets. Over time, this can exhaust the available resources, potentially causing the application to fail in accepting new connections or leading to unexpected behavior.
By ensuring that conn.close() is called in a finally block, we make certain that the connection is closed regardless of whether an exception occurs during message handling. This practice helps maintain the stability and efficiency of the application by preventing resource leaks.
There was a problem hiding this comment.
@Zelberor do you think this is a real issue, or is this not relevant for our limited debugging use case.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I don't know if it is really relevant, because an exception should only occur if the debug_wrapper fails while sending a log message which I deem unlikely.
But ignoring that, the multiprocessing documentation tells me that the close() functions is automatically called when conn is garbage collected.
Now due to pythons weird scoping I think conn is not garbage collected before the listener tries to accept another connection. But the listener should be able to accept multiple connections (linux socket) and thus the accept fn will not fail due to conn still being active. At that point conn is reassigned and the old conn is garbage collected closing the connection.
In case the loops exists for some reason the garbage collection occurs when exiting the function.
So should be fine in all cases (speculation though, I don't know python that well)
| error = None | ||
| success = False | ||
| start_time = time.monotonic() | ||
| while not success and start_time + 5.0 > time.monotonic(): | ||
| try: | ||
| address = ("localhost", 52999) | ||
| conn = Client(address, authkey=b"debug_logger") | ||
| conn.send({"name": NODE_NAME, "msg": msg, "level": level}) | ||
| conn.close() | ||
| success = True | ||
| except Exception as e: | ||
| error = e | ||
| if not success: | ||
| eprint(msg) | ||
| if error is not None: | ||
| eprint(f"Failed to send to logger: {error}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve logging reliability and error handling
The logging implementation has several areas for improvement:
- The 5-second retry loop could delay node startup
- The connection is not properly closed in error cases
- The error handling could be more informative
Apply this diff:
error = None
success = False
start_time = time.monotonic()
+ conn = None
while not success and start_time + 5.0 > time.monotonic():
try:
address = ("localhost", 52999)
conn = Client(address, authkey=b"debug_logger")
conn.send({"name": NODE_NAME, "msg": msg, "level": level})
- conn.close()
success = True
except Exception as e:
error = e
+ finally:
+ if conn is not None:
+ conn.close()
if not success:
eprint(msg)
if error is not None:
- eprint(f"Failed to send to logger: {error}")
+ eprint(f"Failed to send to logger after {time.monotonic() - start_time:.1f}s: {error}")📝 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.
| error = None | |
| success = False | |
| start_time = time.monotonic() | |
| while not success and start_time + 5.0 > time.monotonic(): | |
| try: | |
| address = ("localhost", 52999) | |
| conn = Client(address, authkey=b"debug_logger") | |
| conn.send({"name": NODE_NAME, "msg": msg, "level": level}) | |
| conn.close() | |
| success = True | |
| except Exception as e: | |
| error = e | |
| if not success: | |
| eprint(msg) | |
| if error is not None: | |
| eprint(f"Failed to send to logger: {error}") | |
| error = None | |
| success = False | |
| start_time = time.monotonic() | |
| conn = None | |
| while not success and start_time + 5.0 > time.monotonic(): | |
| try: | |
| address = ("localhost", 52999) | |
| conn = Client(address, authkey=b"debug_logger") | |
| conn.send({"name": NODE_NAME, "msg": msg, "level": level}) | |
| success = True | |
| except Exception as e: | |
| error = e | |
| finally: | |
| if conn is not None: | |
| conn.close() | |
| if not success: | |
| eprint(msg) | |
| if error is not None: | |
| eprint(f"Failed to send to logger after {time.monotonic() - start_time:.1f}s: {error}") |
| try: | ||
| conn = listener.accept() | ||
| print(f"[debug_logger]: connection accepted from {listener.last_accepted}") | ||
| msg = None | ||
| if conn.poll(timeout=2.0): | ||
| msg = conn.recv() | ||
| if isinstance(msg, str): | ||
| if msg.lower() == CLOSE_MSG: | ||
| running = False | ||
| msg = None | ||
| conn.close() | ||
| except Exception as error: |
There was a problem hiding this comment.
What happens if the connection is not closed?
Description
This pull request makes debugging individual nodes with VS Code possible. This should improve debugging efficiency.
It also documents existing and new debugging possibilities.
Fixes #398
Type of change
Does this PR introduce a breaking change?
Most important changes
Checklist:
I have added tests that prove my fix is effective or that my feature works (might be obsolete with CI later on)Summary by CodeRabbit
New Features
debug_wrapperanddebug_loggerfor enhanced logging and debugging capabilities.debugpy.agent.launchfile.lidar_distancenode in the perception launch configuration.Bug Fixes
Documentation
Chores
requirements.txtto includedebugpyas a dependency.