feat: rai_sim #415
feat: rai_sim #415
Conversation
There was a problem hiding this comment.
@MagdalenaKotynia Thank you for this PR!
- I left some comments to the code.
- Also I was able to run the example from the PR description, but it didn't work in the first run - I think the binary was loading too long. See my error below. Do you check if the binary is ready to spawn objects?
- it would be good to add an info to the description of the PR that example will only work if you setup rai-manipulation-demo first (because for e.g. this line)
(rai-framework-py3.10) robo-pc-005 ➜ rai git:(feat/benchmarking) ✗ python run.py
/home/bboczek/.cache/pypoetry/virtualenvs/rai-framework-2NHnaPvx-py3.10/lib/python3.10/site-packages/pydub/utils.py:170: RuntimeWarning: Couldn't find ffmpeg or avconv - defaulting to ffmpeg, but may not work
warn("Couldn't find ffmpeg or avconv - defaulting to ffmpeg, but may not work", RuntimeWarning)
2025-02-12 16:21:02 robo-pc-005 rai_sim.o3de.o3de_connector[159409] INFO Running command: ['ros2', 'launch', 'examples/manipulation-demo.launch.py', 'game_launcher:=/home/bboczek/Downloads/RAIManipulationDemo/RAIManipulationDemo.GameLauncher']
Traceback (most recent call last):
File "/u24/home/bboczek/projects_u24/01_internal/01_repos/rai/run.py", line 16, in <module>
o3de.setup_scene(scene_config)
File "/u24/home/bboczek/projects_u24/01_internal/01_repos/rai/src/rai_sim/rai_sim/o3de/o3de_connector.py", line 161, in setup_scene
self._spawn_entity(entity)
File "/u24/home/bboczek/projects_u24/01_internal/01_repos/rai/src/rai_sim/rai_sim/o3de/o3de_connector.py", line 90, in _spawn_entity
entity.pose, self.connector.get_transform("odom", "world")
File "/u24/home/bboczek/projects_u24/01_internal/01_repos/rai/src/rai_core/rai/communication/ros2/connectors.py", line 176, in get_transform
raise LookupException(
tf2.LookupException: Could not find transform from world to odom in 5.0 seconds
Exception in thread Thread-1 (spin):
Traceback (most recent call last):
File "/usr/lib/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "/usr/lib/python3.10/threading.py", line 953, in run
self._target(*self._args, **self._kwargs)
File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 294, in spin
self.spin_once()
File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 794, in spin_once
self._spin_once_impl(timeout_sec)
File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/executors.py", line 786, in _spin_once_impl
self._executor.submit(handler)
File "/usr/lib/python3.10/concurrent/futures/thread.py", line 169, in submit
raise RuntimeError('cannot schedule new futures after '
RuntimeError: cannot schedule new futures after interpreter shutdown
Exception ignored in: <function O3DEngineConnector.__del__ at 0x7e176e350790>
Traceback (most recent call last):
File "/u24/home/bboczek/projects_u24/01_internal/01_repos/rai/src/rai_sim/rai_sim/o3de/o3de_connector.py", line 75, in __del__
File "/u24/home/bboczek/projects_u24/01_internal/01_repos/rai/src/rai_sim/rai_sim/o3de/o3de_connector.py", line 64, in shutdown
AttributeError: 'NoneType' object has no attribute 'warning'
| Setup of scene - arrangmenet of objects, interactions, environment etc. | ||
| """ | ||
|
|
||
| binary_path: Optional[str] |
There was a problem hiding this comment.
binary_path seem to be restrictive. How about allowing a command or script.
Looking at the implementation it seems like the rest of the code should handle it, but variable name suggests that only a binary file is supported
There was a problem hiding this comment.
Currently, base SimulationConfig has only the common part for all simulations so it does not have binary_path parameter. When implementing the SimulationBridge interface, it is needed to create a custom config and define parameters for running/setup of the simulation. Therefore, binary_path parameter was moved to O3DExROS2Bridge.
| return SceneSetup(entities=scene_config.entities) | ||
|
|
||
| def launch_binary(self, binary_path: str): | ||
| # NOTE (mkotynia) ros2 launch command with binary path, to be refactored |
There was a problem hiding this comment.
Connected to my previous comments
|
@coderabbitai full review |
WalkthroughThe pull request introduces the new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Application
participant O3D as O3DEngineConnector
participant ROS as ROS2 Service
participant PM as Process Manager
U->>O3D: load_config(file_path)
U->>O3D: setup_scene(scene_config)
alt Binary path has changed
O3D->>PM: shutdown previous process
O3D->>PM: launch_binary(binary_path)
O3D->>PM: has_process_started(timeout)
end
loop For each entity in scene_config
O3D->>ROS: _spawn_entity(entity)
end
O3D-->>U: return SceneSetup
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/rai_sim/rai_sim/engine_connector.py (3)
46-46: Fix typo in docstring.“arrangmenet” should be corrected to “arrangement.”
- Setup of scene - arrangmenet of objects, interactions, environment etc. + Setup of scene - arrangement of objects, interactions, environment etc.
66-67: Remove or annotate the empty constructor.A constructor that does nothing in an abstract base class can be removed or marked as abstract if future child classes are expected to handle initialization themselves.
-class EngineConnector(ABC): - def __init__(self): - pass +class EngineConnector(ABC): + @abstractmethod + def __init__(self): + """Child classes should perform specialized initialization."""🧰 Tools
🪛 Ruff (0.8.2)
66-67:
EngineConnector.__init__is an empty method in an abstract base class, but has no abstract decorator(B027)
96-97: Avoid re-raising the same exception directly.Having an empty
exceptblock that simply re-raises the same exception is redundant. Either remove thetry-exceptentirely or add additional exception handling logic.- except Exception as e: - raise e + except Exception: + logger.error("Failed to load config file.", exc_info=True) + raisesrc/rai_sim/rai_sim/o3de/o3de_connector.py (2)
40-65: Consider a more graceful shutdown strategy.The current approach forcefully kills processes and children, which may abruptly terminate ROS2. A more elegant solution would be to invoke ROS2 shutdown procedures or use graceful signals with retries before forcing termination.
167-168: Flatten nested if statements.Combine the nested conditions into a single check to enhance readability.
-if self.current_process is not None: - if self.current_process.poll() is None: - return True +if self.current_process is not None and self.current_process.poll() is None: + return True🧰 Tools
🪛 Ruff (0.8.2)
167-168: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
pyproject.toml(1 hunks)src/rai_sim/pyproject.toml(1 hunks)src/rai_sim/rai_sim/__init__.py(1 hunks)src/rai_sim/rai_sim/engine_connector.py(1 hunks)src/rai_sim/rai_sim/o3de/o3de_connector.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/rai_sim/rai_sim/engine_connector.py
66-67: EngineConnector.__init__ is an empty method in an abstract base class, but has no abstract decorator
(B027)
src/rai_sim/rai_sim/o3de/o3de_connector.py
167-168: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (6)
src/rai_sim/rai_sim/engine_connector.py (1)
20-20: Use a more generic data structure to avoid ROS2 dependency.Relying on
geometry_msgs.msgat this abstraction level prevents theEngineConnectorfrom being engine-neutral. This concern was previously raised and remains relevant.src/rai_sim/rai_sim/o3de/o3de_connector.py (1)
69-75: Verify service usage mismatch.Using
gazebo_msgs/srv/GetWorldPropertiesto retrieve spawnable names in O3DE might be unintended. Confirm that the target engine and message types correspond correctly, and ensure the backend is actually O3DE-compatible.src/rai_sim/rai_sim/__init__.py (1)
1-17: No issues found.The file is well-documented, and versioning is clearly defined.
src/rai_sim/pyproject.toml (2)
1-14: LGTM! Well-structured package metadata.The package metadata is well-defined with appropriate name, version, description, authors, and classifiers.
21-23: LGTM! Standard build system configuration.The build system configuration follows Poetry's best practices.
pyproject.toml (1)
20-20: LGTM! Consistent dependency declaration.The
rai_simdependency is correctly declared following the same pattern as other local modules.
|
|
||
| [tool.poetry.dependencies] | ||
| python = "^3.10, <3.13" | ||
| PyYAML = "*" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add version constraint for PyYAML dependency.
Using an unconstrained version (*) for dependencies can lead to compatibility issues when new versions are released. Consider adding a version constraint to ensure compatibility.
-PyYAML = "*"
+PyYAML = "^6.0.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.
| PyYAML = "*" | |
| PyYAML = "^6.0.1" |
| for entity in self.entity_ids: | ||
| self._despawn_entity_by_id(self.entity_ids[entity]) | ||
| self.entity_ids = {} | ||
| time.sleep(3) |
There was a problem hiding this comment.
Hard coded sleep looks so wrong. We should be able to detect somehow that the binary has started and is ready to receive commands
I can guarantee that some sims will need more than N seconds to start.
There was a problem hiding this comment.
I removed this hard-coded sleep, I added a check if the process has started inside launch_binary/launch_robotic_stack defs. I am wondering whether this check is enough to be sure that the binary has been launched. Maybe there should be some additional check whether some topics started to send messages, e.g. clock, what do you think?
src/rai_sim/README.md
Outdated
There was a problem hiding this comment.
Please fill the readme. At least some core information.
There was a problem hiding this comment.
Done in 13bf358
| for child in alive: | ||
| logger.warning(f"Force killing child process PID: {child.pid}") | ||
| child.kill() | ||
| # NOTE (mkotynia) terminating ros2 launch |
There was a problem hiding this comment.
This is engine connector, why do you mention ros2 launch? what do ros2 launch have to do with O3DEngineConnector?
There was a problem hiding this comment.
it is related to this discussion #415 (comment). Currently, it is a simulation connector.
| msg = ROS2ARIMessage(payload=msg_content) | ||
|
|
||
| response = self.connector.service_call( | ||
| msg, target="spawn_entity", msg_type="gazebo_msgs/srv/SpawnEntity" |
There was a problem hiding this comment.
I'm not feeling coupling the ROS 2 interface with O3DEngineConnector. Semantically, talking about "o3de connector" - shouldn't we care about connecting rai with o3de only (o3de interfaces, launch arguments, parameters, configuration)?
We assumed here that the O3DE comes with ROS 2 interface and the simulation follows certain standard/implementation.
What if someone have different API to control O3DE simulation?
Maybe we should make a distinguish between engine, and provided simulation interface?
If we bring a concrete implementation of the "connector" (in this case, an O3DE based sim that follows a standard and uses ROS 2 gem), then we should consider changing a name to something more concrete.
There was a problem hiding this comment.
we should consider changing a name to something more concrete
Right. We have discussed this topic outside of git, our O3DE + ROS2 combo relies on a specific O3DE setup. @knicked is currently working on a o3de requirements document.
There was a problem hiding this comment.
I renamed EngineConnector with SimulationConnector and O3DEngineConnector to O3DExROS2Connector to better reflect the meaning - that it is O3DE with ROS2 interface.
| command, | ||
| ) | ||
| if not self.has_process_started(): | ||
| raise RuntimeError("Process did not start in time.") |
There was a problem hiding this comment.
Inconsistent use of exception types. setup_scene throws an Exception, but here we are using RuntimeError. Both are "on the same level":
if scene_config.binary_path:
self.launch_binary(scene_config.binary_path) # can raise RuntimeError
else:
raise Exception("No binary path provided")There was a problem hiding this comment.
I added the check if scene_config.binary_path outside the launch_binary method because it seemed for me to be a clearer solution than allowing the call of the launch_binary method with an optional binary_path argument and checking it inside this method. Currently the problem is outdated because after yesterday discussion with @maciejmajek we decided to separate common config attributes to base SimulationConfig class and to define other attributes in custom classes for each connector (implemented in ab5bf1f). As a result, the binary_path is now mandatory for this connector and there is no need to check it inside the connector (it is handled when loading config content to pydantic model).
4e6b6f3 to
eeb3e14
Compare
| response = self._try_service_call( | ||
| msg, target="spawn_entity", msg_type="gazebo_msgs/srv/SpawnEntity" |
There was a problem hiding this comment.
Is it possible for this service to return a value, while failing to spawn the entity? Or does getting a response guarantee success on the spawning task?
There was a problem hiding this comment.
TODO: handle if spawn fails
There was a problem hiding this comment.
I added errors handling in 2a97f9f.
Services used in this task may return value with success == False (e.g. when not existing prefab name is given) or may raise an error (e.g. when the service with given name is not running or does not exist).
| raise RuntimeError("Process did not start in time.") | ||
|
|
||
| def _has_process_started( | ||
| self, process: subprocess.Popen[Any] | None, timeout: int = 15 |
There was a problem hiding this comment.
Why is None a valid argument for this method? If process=None this method is equivalent to sleep(15), so even if it's required to handle such an argument it would be warranted to early quit instead of waiting for 15 seconds.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
applied in f439bde
|
|
||
|
|
||
| class O3DExROS2SimulationConfig(SimulationConfig): | ||
| binary_path: Path |
There was a problem hiding this comment.
@MagdalenaKotynia How about this comment?
There was a problem hiding this comment.
I've already replied #415 (comment), sorry, I missed this comment.
d4ac769 to
db30465
Compare
d312a60 to
702bb00
Compare
Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai>
Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai>
Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai>
Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> Co-authored-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai>
…suggest having something common with ROS2ARIConnector
…get rid of dependency of bridge interface from ros2
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
702bb00 to
6b07135
Compare
Signed-off-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai> Co-authored-by: Jakub Matejczyk <jakub.matejczyk@robotec.ai> Co-authored-by: Kacper Dąbrowski <kacper.dabrowski@robotec.ai> Co-authored-by: Piotr Jaroszek <10896036+pijaro@users.noreply.github.com>
Purpose
To implement package for running simulations with different setups.
SimulationBridge - the interface to connect with specific engine. It is responsible for scene setup, spawning, despawning objects, etc.
def setup_scenelogic is based on the provided simulation_config of type specific for given bridge. There is a common configuration part that should be provided for each bridge, it is implemented as SimulationConfig class.SceneState is going to store the initial scene setup.
O3DExROS2Bridge - implementation of the SimulationBridge for O3DE with ROS2
Issues
#405 - This PR covers the part responsible for connection with simulation (EngineConnector - currently named SimulationBridge).
Testing
Summary by CodeRabbit
New Features
Documentation