-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: O3DE test benchmark #426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good job, I have left a few comments regarding code structure, and I am unsure if all the committed files should be included.
from pathlib import Path | ||
|
||
|
||
class GrabCarrotTask(Task): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a good idea to create a module with Tasks so they can be imported into different benchmark runtimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to create a separate folder still inside o3de_test_bench? or to define specific tasks in rai_bench package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I mean create a separate folder with init.py (i.e. a submodule) inside the o3de_test_bench
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i moved tasks to separate folder, also moved whole o3de_test_branch to rai_bench package so the benchamark/ tasks can be imported across project. main.py to setup and run benchmark moved to src/rai_bench/main.py, so still outside package
one_carrot_scene_config = O3DExROS2SimulationConfig.load_config( | ||
base_config_path=Path("src/rai_bench/o3de_test_bench/scene1.yaml"), | ||
connector_config_path=Path("src/rai_bench/o3de_test_bench/o3de_config.yaml"), | ||
) | ||
multiple_carrot_scene_config = O3DExROS2SimulationConfig.load_config( | ||
base_config_path=Path("src/rai_bench/o3de_test_bench/scene2.yaml"), | ||
connector_config_path=Path("src/rai_bench/o3de_test_bench/o3de_config.yaml"), | ||
) | ||
red_cubes_scene_config = O3DExROS2SimulationConfig.load_config( | ||
base_config_path=Path("src/rai_bench/o3de_test_bench/scene3.yaml"), | ||
connector_config_path=Path("src/rai_bench/o3de_test_bench/o3de_config.yaml"), | ||
) | ||
multiple_cubes_scene_config = O3DExROS2SimulationConfig.load_config( | ||
base_config_path=Path("src/rai_bench/o3de_test_bench/scene4.yaml"), | ||
connector_config_path=Path("src/rai_bench/o3de_test_bench/o3de_config.yaml"), | ||
) | ||
# combine different scene configs with the tasks to create various scenarios | ||
scenarios = [ | ||
Scenario( | ||
task=GrabCarrotTask(logger=bench_logger), | ||
scene_config=one_carrot_scene_config, | ||
), | ||
Scenario( | ||
task=GrabCarrotTask(logger=bench_logger), | ||
scene_config=multiple_carrot_scene_config, | ||
), | ||
Scenario( | ||
task=GrabCarrotTask(logger=bench_logger), | ||
scene_config=red_cubes_scene_config, | ||
), | ||
Scenario( | ||
task=RedCubesTask(logger=bench_logger), scene_config=red_cubes_scene_config | ||
), | ||
Scenario( | ||
task=RedCubesTask(logger=bench_logger), | ||
scene_config=multiple_cubes_scene_config, | ||
), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a good idea to have these load dynamically to allow for easy configuration of scenes/tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean loading from cmd arguments, from config file, or what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean a functionality to auto create scenarios given a list of Tasks and a list of scene paths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR. I have left some comments.
Apart from that:
-
Please apply new naming convention for simulation connectors (see feat: rai_sim #415, SimulationConnector was renamed to SimulationBridge to avoid confusion with the BaseConnector and its subclasses.
-
Please fix typing where possible
Ideas for the future improvement:
-
I think it would be very beneficial to save output from camera (from the whole scenario or at least beginning and end of the scenario) to be able to verify metrics with human assessment. It also could be useful for presenting and reporting the results.
-
When the final implementation of benchmark interface will be established, the next step could be to design and implement some structure for saving and storing the results (results + info about what was benchmarked + camera images + metadata).
super().__init__(message) | ||
|
||
|
||
class Task(ABC, Generic[SimulationConnectorT]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Task class should not be bound with any specific SimulationBridge, it should be reused with different implementations of SimulationBridge. Please apply appropriate typing for this class and the calculate_result method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so i will modify the Task to take SimulationConfig as argument, and from this i will extract initial_positions of objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second thing is - as spawned_entities are unique to O3De , methods of SimulaitonBride should return Entities, not spawnedEntities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that Task does not need to take SimulationConfig as argument - Scenario uses SimulationConfig and bounds it with Task (btw please rename scene_config to simulation_config according to the changes in naming convention). Task itself should be SimulationConfig-agnostic, current interface is ok, just typing is inappropriate.
Second, yes, good point. I noticed it yesterday when reviewing your PR and I decided to move spawned_entities to base SimulationBridge class because it is needed for all simulations if we want to compare them. Please rebase to these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i refactored it to match your new code, task now uses spawned_entities attribute from Bridges
"Number of initially spawned entities does not match number of entities present at the end." | ||
) | ||
|
||
for ini_carrot in initial_carrots: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is strictly matching our simulation where we know how the table is located with respect to the origin. Maybe it's overkill for now but I would consider parametrizing it somehow. Maybe the calculate_result method should take some kwargs to be able to reuse the task for simulations with different parameters key for computing result (e.g. in this case the info about coordinates and size of the table top).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how the cooridinates work in other simulation engines, so it's difficult to make it work. I think for now leave it as this is, maybe in future when we will work with different engines, I will take this into cosideration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean other simulation engines but e.g. another o3de robotic arm simulation where the table is located in another way with respect to origin than the current simulation we use, e.g. (x,y) = 0 is not on the middle of the table as it is assumed in this example. I think we can leave it for now, general differentiating where is left and right is quite hard problem IMO, please just leave a NOTE with info that it is example for particular demo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
the additiional files propably come from rebases, but you are right, they are not part of this PR, so I will remove them |
add new occuring issue to the main comment of this PR |
28b2a5b
to
ae2b33d
Compare
``` 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 167, in submit raise RuntimeError('cannot schedule new futures after shutdown') RuntimeError: cannot schedule new futures after shutdown ```
Co-authored-by: Jakub Matejczyk <[email protected]> chore: naming changes Co-authored-by: Jakub Matejczyk <[email protected]> chore: naming changes Co-authored-by: Jakub Matejczyk <[email protected]> chore: redefine benchmark model Co-authored-by: Jakub Matejczyk <[email protected]> Co-authored-by: Kacper Dąbrowski <[email protected]> refactor: SceneConfig to BaseModel class feat: add SceneSetup to store initial scene setup build: poetry initialization of rai_benchmarks and rai_simulations chore: add licence lines build: create packages from rai_benchmarks and rai_simulations chore: removed mistakenly added file feat: scene config implementation Add O3DEEngineConnector features Signed-off-by: Kacper Dąbrowski <[email protected]> Remove an unused file Signed-off-by: Kacper Dąbrowski <[email protected]> Add binary path caching Signed-off-by: Kacper Dąbrowski <[email protected]> Add two example scenes Signed-off-by: Kacper Dąbrowski <[email protected]> feat: replace binary path with ros2 launch command + binary path refactor: renamed rai_sim and rai_bench fix: fixed shutdown of binary chore: make pose mandatory chore: remove rai_bench because it is developed on another branch ci: add missing license lines laoding and spawning frm benchmark rabsed naming change grab xyz benchmark benchmarks run new sim every scenario for now
Co-authored-by: Jakub Matejczyk <[email protected]> chore: naming changes Co-authored-by: Jakub Matejczyk <[email protected]> chore: naming changes Co-authored-by: Jakub Matejczyk <[email protected]> chore: redefine benchmark model Co-authored-by: Jakub Matejczyk <[email protected]> Co-authored-by: Kacper Dąbrowski <[email protected]> refactor: SceneConfig to BaseModel class feat: add SceneSetup to store initial scene setup build: poetry initialization of rai_benchmarks and rai_simulations chore: add licence lines build: create packages from rai_benchmarks and rai_simulations chore: removed mistakenly added file feat: scene config implementation Add O3DEEngineConnector features Signed-off-by: Kacper Dąbrowski <[email protected]> Remove an unused file Signed-off-by: Kacper Dąbrowski <[email protected]> Add binary path caching Signed-off-by: Kacper Dąbrowski <[email protected]> Add two example scenes Signed-off-by: Kacper Dąbrowski <[email protected]> feat: replace binary path with ros2 launch command + binary path refactor: renamed rai_sim and rai_bench fix: fixed shutdown of binary chore: make pose mandatory chore: remove rai_bench because it is developed on another branch ci: add missing license lines laoding and spawning frm benchmark rabsed naming change grab xyz benchmark benchmarks run new sim every scenario for now
found new issue with getting object position from simulation, part of it was due to changes introduced in feat/benchmarking , resolved here: 702bb00 Now the transform is good, but the position of object is always same as starting position, even when object was moved on simulation |
903906b
to
a33e500
Compare
fixed here: 871f707 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When attempting to run the according to PR description I get an error:
Traceback (most recent call last):
File "/home/krachwal/projects/internal/rai/src/rai_bench/rai_bench/main.py", line 21, in <module>
from rai_bench.benchmark_model import (
ModuleNotFoundError: No module named 'rai_bench'
It seems that poetry install doesn't install the new package.
added package to pyproject.toml |
task defined more clearly
This fixes bug when get_transform returns always the same, first transform Add logs to track retrieved positions
e5f93ad
to
82ed909
Compare
added log when all scenarios finished
refactored scenario to inlucde config path so it can be logged easily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Maciej Majek <[email protected]> Co-authored-by: Bartłomiej Boczek <[email protected]> Co-authored-by: MagdalenaKotynia <[email protected]>
Purpose
Add Test Benchmark for O3DE simulation
Add framework for easier benchmark creation
Proposed Changes
Majority of file changes are only formatting changes, main changes are in src/rai_bench package and src/rai_sim
This PR contains new package rai_bench. It introduces classes like Task, Scenario and Benchmark(src/rai_bench/rai_bench/benchmark_model.py), which are frame for creating benchmarks and it also contains O3DE Test Benchmark which is a test implementation of a benchmark for O3DE engine simulation.
O3DE Test Benchmark (
src/rai_bench/rai_bench/o3de_test_bench/
), contains 2 Tasks(tasks/
) - GrabCarrotTask and PlaceCubesTask (these tasks implement calculating scores) and 4 scenes(configs/
) for O3DE robotic arm simulation.Example of how can scenarios and benchmark be defined and run can be found in
rai_bench/rai_bench/main.py
Additional changes:
Issues
Testing
the binary to simulation can be downloaded from here: humble
To run this benchmark you need o3de bridge config file in
src/rai_bench/rai_bench/o3de_test_bench/configs/o3de_config.yaml
, this config file should include fields:to install and run:
Logs about the progess of benchmark can be found in 'src/rai_bench/o3de_test_bench/benchmark_agent.log'
You should be able to see:
After finishing all scenarios, simulation should close and all nodes and services should also shutdown after no more than 30 seconds