-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Simplifies device setting in SimulationCfg and AppLauncher #696
Conversation
Adding hunter to review AppLauncher or any docker related 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.
Looks great and is much needed to simplify things - still wonder if we're losing some testability in the future by removing the ability to specify GPU vs CPU for the various parts of pipeline.
An alternative approach (which would obviously complicate things, but make it a little more flexible) is to enable user to pass in either a string based device as you've implemented here or alternatively pass in a dictionary, containing keys/ values like:
device:
{
"use_gpu_pipeline": False,
"physx_use_gpu": False,
"tensor_device": "cuda" # Or any of the options we've created e.g. "cuda:0", "cpu"
}
Again, this is dependent on the underlying Isaac Sim's Simulation Context being able to still handle these permutations, which I think @Mayankm96 mentioned had been removed.
I double checked through tutorials / docs if this was used elsewhere and couldn't find anything too.
source/extensions/omni.isaac.lab/omni/isaac/lab/sim/simulation_cfg.py
Outdated
Show resolved
Hide resolved
...extensions/omni.isaac.lab_tasks/omni/isaac/lab_tasks/utils/wrappers/rsl_rl/vecenv_wrapper.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/envs/manager_based_env.py
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/sim/simulation_cfg.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/sim/simulation_cfg.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/app/app_launcher.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/app/app_launcher.py
Outdated
Show resolved
Hide resolved
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 is a breaking change. I think once we merge it, we should bump up the Isaac Lab version and add the migration guide for this change. What do you think?
Also need to update version in changelog and extension.toml :)
…_cfg.py Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com> Signed-off-by: David Hoeller <dhoeller@nvidia.com>
…_cfg.py Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com> Signed-off-by: David Hoeller <dhoeller@nvidia.com>
…er.py Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com> Signed-off-by: David Hoeller <dhoeller@nvidia.com>
…er.py Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com> Signed-off-by: David Hoeller <dhoeller@nvidia.com>
…_env.py Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com> Signed-off-by: David Hoeller <dhoeller@nvidia.com>
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.
Hello,
Thanks for all your effort!
I just noticed that this PR creates conflicts for the two scripts that already use --device
CLI argument for a different purpose. Corresponding examples in docs/source/setup/sample.rst
will also need to be adjusted.
(commented here instead of opening a new issue because this PR is fresh)
@@ -13,7 +13,6 @@ | |||
|
|||
# add argparse arguments | |||
parser = argparse.ArgumentParser(description="Keyboard teleoperation for Isaac Lab environments.") |
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 script already uses --device
to select the teleoperation device. I believe it has to be renamed in order to avoid conflicts with the changes from this PR.
@@ -13,7 +13,6 @@ | |||
|
|||
# add argparse arguments | |||
parser = argparse.ArgumentParser(description="Collect demonstrations for Isaac Lab environments.") |
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 script already uses --device
to select the teleoperation device. I believe it has to be renamed in order to avoid conflicts with the changes from this PR.
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.
Hi @AndrejOrsula, thanks a lot for the catch. Fix incoming very soon.
…#696) # Description Before setting up the device was tedious and redundant, and you would see code like: ```python sim_utils.SimulationCfg(device="cpu", use_gpu_pipeline=False, dt=0.01, physx=sim_utils.PhysxCfg(use_gpu=False)) ``` With this MR, we simply set the desired device, and all the parameters, such as `use_gpu_pipeline` and `use_gpu` propagate automatically, resulting in the following: ```python sim_utils.SimulationCfg(device="cpu", dt=0.01, physx=sim_utils.PhysxCfg()) ``` The command line input `--cpu` has been removed in favor of `--device device_name`, where valid options for `device_name` are `cpu` (run on CPU), `cuda` (run on GPU with device id 0), or `cuda:N` (run on GPU with device id N, for example `cuda:0`). ## Type of change - Breaking Change ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
Description
Before setting up the device was tedious and redundant, and you would see code like:
With this MR, we simply set the desired device, and all the parameters, such as
use_gpu_pipeline
anduse_gpu
propagate automatically, resulting in the following:The command line input
--cpu
has been removed in favor of--device device_name
, where valid options fordevice_name
arecpu
(run on CPU),cuda
(run on GPU with device id 0),cuda:N
(run on GPU with device id N, for examplecuda:0
).Type of change
Checklist
pre-commit
checks with./isaaclab.sh --format
config/extension.toml
fileCONTRIBUTORS.md
or my name already exists there