Skip to content

refactor: running and killing ros2 processes #581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 15, 2025

Conversation

jmatejcz
Copy link
Contributor

@jmatejcz jmatejcz commented May 14, 2025

Purpose

Current implementation of running and shutting down ros2 stack gives little control from python level.

Besides that i refactored simulation config logic as it was not best suited for current code.
the SimulationConfig contained both entities which is required for scene setup and settings for simulation and robotic stack which are needed only when launching. This was not the cleanest by itself but also resulted in confusing configurations:

  • Every single Scenario needed entites but had also all the configuration for simulation
  • The ifs to run simulation were checked at every scenario

Proposed Changes

Introduced Ros2LaunchManager which manages the launching and shutting down of ros nodes via LaunchService object from ros2 api.
Field robotic_stack_command removed from o3de_config as it was weird that user need to provide this. Now it is LaunchDescription is assigned to benchmarks.

Refactored Simulation config that into:

  • SimulationConfig that retained simulation configs
  • SceneConfig - with entites

O3DExROS2SimulationConfig is now without robotic_stack_command field as it no longer needed

Issues

#571

Testing

  1. Remove robotic_stack_command from your o3de_config.yaml
  2. Run python src/rai_bench/rai_bench/examples/test_models.py
    with settings:
 models_name = ["qwen2.5:7b", "llama3.2"]
    vendors = ["ollama", "ollama"]
    benchmarks = ["manipulation_o3de"]
    extra_tool_calls = [5]
    repeats = 1
  1. Check if nodes are properly closed after finish:
ros2 node list

@jmatejcz jmatejcz force-pushed the jm/refactor/closing-ros2-processes branch from eba7bd7 to ca59440 Compare May 14, 2025 10:42
@jmatejcz jmatejcz marked this pull request as ready for review May 14, 2025 10:42
@jmatejcz jmatejcz requested a review from maciejmajek May 14, 2025 10:43
@jmatejcz
Copy link
Contributor Author

I' m not sure where to put Ros2LaunchManager, for its in rai_sim/launch_manager.py

@jmatejcz jmatejcz force-pushed the jm/refactor/closing-ros2-processes branch from 5ea6767 to 01aa40d Compare May 15, 2025 07:29
Copy link
Member

@maciejmajek maciejmajek left a comment

Choose a reason for hiding this comment

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

Please rename Ros2LaunchManager to ROS2LaunchManager, other than that LGTM and GJ

@jmatejcz jmatejcz force-pushed the jm/refactor/closing-ros2-processes branch 3 times, most recently from f23c9a5 to 982fa64 Compare May 15, 2025 13:28
@jmatejcz jmatejcz force-pushed the jm/refactor/closing-ros2-processes branch from 982fa64 to 768b55d Compare May 15, 2025 13:30
@jmatejcz jmatejcz merged commit 1b3f204 into development May 15, 2025
6 checks passed
@jmatejcz jmatejcz deleted the jm/refactor/closing-ros2-processes branch May 15, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants