-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adds optional suffix parameter for docker name #2172
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
base: main
Are you sure you want to change the base?
Conversation
… and container names for development purposes.
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.
Thanks a lot for the contribution! Some minor feedback from my side.
@hhansen-bdai could you also take a look please?
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 good to me, thanks @zoemcc !
I pushed a new commit to address the review comments. I decided to keep the verbose env variable name for the docker side since verbosity is the norm there, but on the python side it uniformly uses the shorter "suffix" variable name. I insert the hyphen into the user's input suffix directly before using it, since I didn't want to add any logic about whether or not there should be a hyphen on the environment variable side. I have tested the new changes locally with base and ros2, with and without suffix. I expanded the docs and made it more clear and gave it a full paragraph, with the old explanation still valid and clear if the user does not add a suffix: Thanks! |
I added some barebones docker start/stop tests in the 4 standard configurations (base/ros2)/(no suffix/suffix) to be run on computers that have docker configured correctly already. |
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.
Pull Request Overview
This PR adds an optional suffix parameter for docker image and container names to allow custom naming. It also bumps the extension version, adjusts naming logic in the container interface and docker-compose files accordingly, and updates the tests to cover both cases with and without the suffix.
Reviewed Changes
Copilot reviewed 6 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
source/isaaclab/config/extension.toml | Version bump to reflect the new feature |
docker/utils/container_interface.py | Adds a suffix parameter and updates naming for docker containers |
docker/test_docker.py | Adds tests to cover both suffix and non-suffix cases |
docker/docker-compose.yaml | Updates service definitions to use the optional suffix |
docker/container.py | Updates CLI argument parsing and passes suffix to the container interface |
CONTRIBUTORS.md | Adds a new contributor |
Files not reviewed (5)
- docker/.env.base: Language not supported
- docker/.env.ros2: Language not supported
- docker/Dockerfile.ros2: Language not supported
- docs/source/deployment/docker.rst: Language not supported
- source/isaaclab/docs/CHANGELOG.rst: Language not supported
Signed-off-by: Mayank Mittal <[email protected]>
Signed-off-by: Mayank Mittal <[email protected]>
Thanks alot for the MR and going over the feedback! It looks good now. Will merge once the checks pass. |
I addressed the docker folder suggestion, but I think the .env file comments are correct as is due to how they're processed and used. I mentioned why in the suggested change up above but I'm happy to explain further if desired. |
docker/docker-compose.yaml
Outdated
@@ -87,8 +87,8 @@ services: | |||
- ISAACSIM_ROOT_PATH_ARG=${DOCKER_ISAACSIM_ROOT_PATH} | |||
- ISAACLAB_PATH_ARG=${DOCKER_ISAACLAB_PATH} | |||
- DOCKER_USER_HOME_ARG=${DOCKER_USER_HOME} | |||
image: isaac-lab-base | |||
container_name: isaac-lab-base | |||
image: isaac-lab-base${DOCKER_NAME_SUFFIX} |
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.
Essentially, here and also in the ros2 dockerfile itself there would need to be extra logic about the hyphen if the env variable doesn't contain the hyphen by this point, which is cumbersome in these files. I think it's better if the env variable can just already have the hyphen so that logic doesn't have to exist.
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.
So that the DOCKER_NAME_SUFFIX env variable is either completely empty in which case this resolves to just isaac-lab-base, or it contains the hyphen (which was inserted by python, after the user gives the non-hyphen version as input, before the environment variable was set) and also the suffix string. So DOCKER_NAME_SUFFIX would indeed have "-custom" or "".
Description
Fixes #2149
Type of change
Screenshots
Before:

After:

Checklist
pre-commit
checks with./isaaclab.sh --format
config/extension.toml
fileCONTRIBUTORS.md
or my name already exists thereComments
I couldn't find any tests for the docker container building, so I'm not sure where to add some. I'm happy to do so, and can add tests for some of the other docker building parameters if desired. I have tested locally for base and ros2 docker profiles with and without the new suffix parameter. The default setting is an empty string for the suffix, so it should be a non breaking change and most users will not notice it being there. I did not touch the cluster docker environment deployment since I do not have a cluster to test it with, so this new optional parameter should not be used in the cluster deployment case.
I noticed that there was some redundant docker image building if the profile is
base
, so I added a check to only build base explicitly if the docker profile is not base, since the ros2 profile needs base built in order to build upon it, which is what I assume the explicit base building is there for.