feat: rai_sim #415
feat: rai_sim #415
Conversation
|
|
||
| _, alive = psutil.wait_procs(processes, timeout=3) | ||
|
|
||
| # NOTE (mkotynia) kill ros2 |
There was a problem hiding this comment.
ros2 should never be killed
we can kill the robotic stack when sim has finished
There was a problem hiding this comment.
Call rclpy shutdown or similar, please do it standard way not brute force hacks
| def __del__(self): | ||
| self.shutdown() | ||
|
|
||
| def get_available_spawnable_names(self) -> list[str]: |
There was a problem hiding this comment.
Please add a try except for n_of possible retries. Same applies to other methods.
There was a problem hiding this comment.
applied in 2b4286e
| process.kill() | ||
| except Exception as e: | ||
| logger.warning(f"Failed to kill process {process.pid}: {e}") | ||
| continue |
There was a problem hiding this comment.
Is the continue needed here?
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'
| from typing import Any, Dict, List, Optional | ||
|
|
||
| import yaml | ||
| from geometry_msgs.msg import Point, Pose, Quaternion |
There was a problem hiding this comment.
I wouldn't rely on ros2 messages on this abstraction level.
As I understand this is a generic interface to connect to simulation engines.
Some simulation engines have a build-in ros2 support (o3de, gazebo, isaacsim through bridge), but some don't (Mujoco, Genesis).
|
|
||
| @field_validator("pose", mode="after") | ||
| @classmethod | ||
| def convert_to_pose(cls, value: Dict[str, Any]) -> Pose: |
There was a problem hiding this comment.
I think this workaround is not needed if we skip using geometry_msgs.Pose
There was a problem hiding this comment.
Correct, the pose/orientation should not depend on ros2.
| 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
|
|
||
| class SceneConfig(BaseModel): | ||
| """ | ||
| Setup of scene - arrangmenet of objects, interactions, environment etc. |
There was a problem hiding this comment.
I don't know if I understand the purpose of adding both SceneSetup and SceneConfig. This is what I assume based on the implementation:
SceneConfigcontains initial objects arrangementSceneSetupcontains current objects arrangement
For example in a basic benchmark as a part of task (so initial state) we would have SceneConfig and some language instruction.
Then we run the rai agent and check if SceneSetup is as expected. If it's fine, then benchmark is passed.
I am not sure what are interactions.
If my understanding is correct I propose to:
- Rename
SceneConfigtoSimulationConfig - Adjust docs -
Setup of scene - set initial arrangement of objects in the environment- be aware of typos
- Rename
SceneSetuptoSceneState
| 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
| except Exception as e: | ||
| raise e |
There was a problem hiding this comment.
I think this code has no effect. It catches exceptions and then raises them again
There was a problem hiding this comment.
true, removed in: d09cbbd
| class SceneSetup(BaseModel): | ||
| """ | ||
| Info about entities in the scene (positions, collisions, etc.) | ||
| """ |
There was a problem hiding this comment.
Consider renaming to SceneState (more info in previous comment).
As for collisions, I think they are not included in the current implementation. How about adding a NOTE that they are meant to be added here?
|
@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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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" |
| Setup of scene - arrangmenet of objects, interactions, environment etc. | ||
| """ | ||
|
|
||
| binary_path: Optional[str] |
There was a problem hiding this comment.
Consider using pathlib to store paths.
There was a problem hiding this comment.
applied in 8ed84b3
| logger.debug(f"Terminating child process {child.pid}, {child.name()}") | ||
| child.terminate() | ||
|
|
||
| _, alive = psutil.wait_procs(children, timeout=3) |
There was a problem hiding this comment.
Increase the timeout, some applications might take up considerable amount of time to clean itself and shutdown.
There was a problem hiding this comment.
|
|
||
| def launch_binary(self, binary_path: str): | ||
| # NOTE (mkotynia) ros2 launch command with binary path, to be refactored | ||
| command = shlex.split(binary_path) |
There was a problem hiding this comment.
This looks like you allow the binary_path to have the start arguments as well.
Is it safe? Shouldn't we treat binary path and arguments separately?
binary_path should be just a path. Otherwise, more appropriate name should be used, like command or something similar.
| 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.
src/rai_sim/README.md
Outdated
There was a problem hiding this comment.
Please fill the readme. At least some core information.
src/rai_sim/rai_sim/__init__.py
Outdated
| @@ -0,0 +1,17 @@ | |||
| # Copyright (C) 2024 Robotec.AI | |||
There was a problem hiding this comment.
Uber nit: shouldn't we change the date to 2025? :)
There was a problem hiding this comment.
applied in ac7fd21
src/rai_sim/rai_sim/__init__.py
Outdated
|
|
||
| """RAI Simulations package.""" | ||
|
|
||
| __version__ = "0.0.1" |
There was a problem hiding this comment.
Just a thought: can we grab this from .toml file? Or should we keep track version number in two different places?
There was a problem hiding this comment.
+1, track with pyproject.toml only
There was a problem hiding this comment.
applied in 1ca1203
| def __init__(self): | ||
| pass |
There was a problem hiding this comment.
There are no shared attributes across the connectors, therefore this can be removed.
| def __init__(self): | |
| pass |
| Translation, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Is this consistent across all rai packages? Looks a bit too barebone to me.
There was a problem hiding this comment.
@maciejmajek do you have some convention about logging handling? I see that some classes have their logger attribute but some don't.
| 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?
| 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.
| 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).
Co-authored-by: Jakub Matejczyk <[email protected]>
Co-authored-by: Jakub Matejczyk <[email protected]>
Co-authored-by: Jakub Matejczyk <[email protected]>
Co-authored-by: Jakub Matejczyk <[email protected]> Co-authored-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
…converting pose to ros2 pose and in inverse direction
…tes common for all future connectors
…ies to SpawnedEntity objects
MagdalenaKotynia
force-pushed
the
feat/benchmarking
branch
from
4e6b6f3 to
eeb3e14
Compare
Purpose
To implement package for running simulations with different setups.
EngineConnector - the interface to connect with specific engine. It is responsible for scene setup, spawning, despawning objects, etc.
def setup_scenelogic is based on the providedSceneConfig. Currently, it is assumed that SceneConfig should be a common class for all of the engines so it should be flexible (that is why binary_path is optional because not all engines may need it to setup scene).SceneSetup is going to store the initial scene setup (more info about spawned entities than SceneConfig provides etc.). It is not handled yet.
O3DEngineConnector - implementation of the EngineConnector for O3DE
Issues
#405 - This PR covers the part responsible for connection with simulation (EngineConnector).
Testing
Summary by CodeRabbit
New Features
Documentation