Cleans up the docker container interface and utilities (#823)

# Description

Mainly documentation fixes and ensuring the code quality remains
consistent. Also adds the `container.sh` script which calls the python
script for compatibility reasons. We can remove it in later releases.

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- This change requires a documentation update

## 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
- [ ] 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

docker/.env.ros2

@@ -3,7 +3,7 @@
 ###
 # Set the version of the ROS2 apt package to install (ros-base, desktop, desktop-full)
 ROS2_APT_PACKAGE=ros-base
-# Se t ROS2 middleware implementation to use (e.g. rmw_fastrtps_cpp, rmw_cyclonedds_cpp)
+# Set ROS2 middleware implementation to use (e.g. rmw_fastrtps_cpp, rmw_cyclonedds_cpp)
 RMW_IMPLEMENTATION=rmw_fastrtps_cpp
 # Path to fastdds.xml file to use (only needed when using fastdds)
 FASTRTPS_DEFAULT_PROFILES_FILE=${DOCKER_USER_HOME}/.ros/fastdds.xml

docker/container.py

@@ -9,24 +9,32 @@ import argparse
 import shutil
 from pathlib import Path
 
-from utils import x11_utils
-from utils.isaaclab_container_interface import IsaacLabContainerInterface
+from utils import ContainerInterface, x11_utils
 
 
-def main():
+def parse_cli_args() -> argparse.Namespace:
+    """Parse command line arguments.
+
+    This function creates a parser object and adds subparsers for each command. The function then parses the
+    command line arguments and returns the parsed arguments.
+
+    Returns:
+        The parsed command line arguments.
+    """
     parser = argparse.ArgumentParser(description="Utility for using Docker with Isaac Lab.")
-    subparsers = parser.add_subparsers(dest="command", required=True)
 
     # We have to create separate parent parsers for common options to our subparsers
     parent_parser = argparse.ArgumentParser(add_help=False)
-    parent_parser.add_argument("profile", nargs="?", default="base", help="Optional container profile specification.")
+    parent_parser.add_argument(
+        "profile", nargs="?", default="base", help="Optional container profile specification. Example: 'base' or 'ros'."
+    )
     parent_parser.add_argument(
         "--files",
         nargs="*",
         default=None,
         help=(
-            "Allows additional .yaml files to be passed to the docker compose command. Files will be merged with"
-            " docker-compose.yaml in the order in which they are provided."
+            "Allows additional '.yaml' files to be passed to the docker compose command. These files will be merged"
+            " with 'docker-compose.yaml' in their provided order."
         ),
     )
     parent_parser.add_argument(
@@ -34,12 +42,13 @@ def main():
         nargs="*",
         default=None,
         help=(
-            "Allows additional .env files to be passed to the docker compose command. Files will be merged with"
-            " .env.base in the order in which they are provided."
+            "Allows additional '.env' files to be passed to the docker compose command. These files will be merged with"
+            " '.env.base' in their provided order."
         ),
     )
 
     # Actual command definition begins here
+    subparsers = parser.add_subparsers(dest="command", required=True)
     subparsers.add_parser(
         "start",
         help="Build the docker image and create the container in detached mode.",
@@ -64,37 +73,55 @@ def main():
     )
     subparsers.add_parser("stop", help="Stop the docker container and remove it.", parents=[parent_parser])
 
+    # parse the arguments to determine the command
     args = parser.parse_args()
 
+    return args
+
+
+def main(args: argparse.Namespace):
+    """Main function for the Docker utility."""
+    # check if docker is installed
     if not shutil.which("docker"):
-        raise RuntimeError("Docker is not installed! Please check the 'Docker Guide' for instruction.")
+        raise RuntimeError(
+            "Docker is not installed! Please check the 'Docker Guide' for instruction: "
+            "https://isaac-sim.github.io/IsaacLab/source/deployment/docker.html"
+        )
 
-    # Creating container interface
-    ci = IsaacLabContainerInterface(
+    # creating container interface
+    ci = ContainerInterface(
         context_dir=Path(__file__).resolve().parent, profile=args.profile, yamls=args.files, envs=args.env_files
     )
 
     print(f"[INFO] Using container profile: {ci.profile}")
     if args.command == "start":
+        # check if x11 forwarding is enabled
         x11_outputs = x11_utils.x11_check(ci.statefile)
+        # if x11 forwarding is enabled, add the x11 yaml and environment variables
         if x11_outputs is not None:
             (x11_yaml, x11_envar) = x11_outputs
             ci.add_yamls += x11_yaml
             ci.environ.update(x11_envar)
+        # start the container
         ci.start()
     elif args.command == "enter":
+        # refresh the x11 forwarding
         x11_utils.x11_refresh(ci.statefile)
+        # enter the container
         ci.enter()
     elif args.command == "config":
         ci.config(args.output_yaml)
     elif args.command == "copy":
         ci.copy()
     elif args.command == "stop":
+        # stop the container
         ci.stop()
+        # cleanup the x11 forwarding
         x11_utils.x11_cleanup(ci.statefile)
     else:
-        raise RuntimeError(f"Invalid command provided: {args.command}")
+        raise RuntimeError(f"Invalid command provided: {args.command}. Please check the help message.")
 
 
 if __name__ == "__main__":
-    main()
+    args_cli = parse_cli_args()
+    main(args_cli)

docker/container.sh

+#!/usr/bin/env bash
+
+# Copyright (c) 2022-2024, The Isaac Lab Project Developers.
+# All rights reserved.
+#
+# SPDX-License-Identifier: BSD-3-Clause
+
+# print warning of deprecated script in yellow
+echo -e "\e[33m------------------------------------------------------------"
+echo -e "WARNING: This script is deprecated and will be removed in the future. Please use 'docker/container.py' instead."
+echo -e "------------------------------------------------------------\e[0m\n"
+
+# obtain current directory
+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
+
+# call the python script
+python3 "${SCRIPT_DIR}/container.py" "${@:1}"

docker/utils/__init__.py

@@ -2,3 +2,7 @@
 # All rights reserved.
 #
 # SPDX-License-Identifier: BSD-3-Clause
+
+from .container_interface import ContainerInterface
+
+__all__ = ["ContainerInterface"]

docker/utils/statefile.py → docker/utils/state_file.py

@@ -2,6 +2,7 @@
 # All rights reserved.
 #
 # SPDX-License-Identifier: BSD-3-Clause
+
 from __future__ import annotations
 
 import configparser
@@ -10,124 +11,141 @@ from pathlib import Path
 from typing import Any
 
 
-def load_cfg_file(path: Path) -> ConfigParser:
-    """
-    Load the contents of a config file.
-
-    If the file exists, read its contents and return them as a dictionary.
-    If the file does not exist, create an empty file and return an empty dictionary.
-
-    Args:
-        statefile: The path to the config file.
-
-    Returns:
-        dict: The contents of the config file as a dictionary.
-    """
-    cfg = ConfigParser()
-    cfg.read(path)
-    return cfg
-
+class StateFile:
+    """A class to manage state variables parsed from a configuration file.
 
-def save_cfg_file(path: Path, cfg: ConfigParser):
-    """
-    Save a dictionary to a config file.
+    This class provides a simple interface to set, get, and delete variables from a configuration
+    object. It also provides the ability to save the configuration object to a file.
 
-    Args:
-        path: The path to the config file.
-        data: The data to be saved to the config file.
-    """
-    with open(path, "w+") as file:
-        cfg.write(file)
-
-
-class Statefile:
-    """
-    A class to manage state variables stored in a cfg file.
+    It thinly wraps around the ConfigParser class from the configparser module.
     """
 
     def __init__(self, path: Path, namespace: str | None = None):
-        """
-        Initialize the Statefile object with the path to the cfg file.
+        """Initialize the class instance and load the configuration file.
 
         Args:
-            path: The path to the cfg file.
-            namespace: Namespace a section of the cfg.
-                Defaults to None, and all member functions will have
-                to specify section or else set Statefile.namespace directly.
+            path: The path to the configuration file.
+            namespace: The default namespace to use when setting and getting variables.
+                Namespace corresponds to a section in the configuration file. Defaults to None,
+                meaning  all member functions will have to specify the section explicitly,
+                or :attr:`StateFile.namespace` must be set manually.
         """
         self.path = path
         self.namespace = namespace
-        self.load_cfg()
+
+        # load the configuration file
+        self.load()
 
     def __del__(self):
         """
-        Save self.loaded_cfg to self.path upon deconstruction
+        Save the loaded configuration to the initial file path upon deconstruction. This helps
+        ensure that the configuration file is always up to date.
         """
-        self.save_cfg()
+        # save the configuration file
+        self.save()
+
+    """
+    Operations.
+    """
 
     def set_variable(self, key: str, value: Any, section: str | None = None):
-        """
-        Set a variable in the cfg file.
+        """Set a variable into the configuration object.
+
+        Note:
+            Since we use the ConfigParser class, the section names are case-sensitive but the keys are not.
 
         Args:
             key: The key of the variable to be set.
             value: The value of the variable to be set.
-            section: section of the cfg. Defaults to the self.namespace
+            section: The section of the configuration object to set the variable in.
+                Defaults to None, in which case the default section is used.
+
+        Raises:
+            configparser.Error: If no section is specified and the default section is None.
         """
+        # resolve the section
         if section is None:
             if self.namespace is None:
-                raise configparser.Error("No section specified")
+                raise configparser.Error("No section specified. Please specify a section or set StateFile.namespace.")
             section = self.namespace
+
+        # create section if it does not exist
         if section not in self.loaded_cfg.sections():
             self.loaded_cfg.add_section(section)
+        # set the variable
         self.loaded_cfg.set(section, key, value)
 
-    def load_variable(self, key: str, section: str | None = None) -> Any:
-        """
-        Load a variable from the cfg file.
+    def get_variable(self, key: str, section: str | None = None) -> Any:
+        """Get a variable from the configuration object.
+
+        Note:
+            Since we use the ConfigParser class, the section names are case-sensitive but the keys are not.
 
         Args:
             key: The key of the variable to be loaded.
-            section: section of the cfg. Defaults to the self.namespace
+            section: The section of the configuration object to read the variable from.
+                Defaults to None, in which case the default section is used.
 
         Returns:
-            any: The value of the variable, or None if the key does not exist.
+            The value of the variable. It is None if the key does not exist.
+
+        Raises:
+            configparser.Error: If no section is specified and the default section is None.
         """
+        # resolve the section
         if section is None:
             if self.namespace is None:
-                raise configparser.Error("No section specified")
+                raise configparser.Error("No section specified. Please specify a section or set StateFile.namespace.")
             section = self.namespace
+
         return self.loaded_cfg.get(section, key, fallback=None)
 
     def delete_variable(self, key: str, section: str | None = None):
-        """
-        Delete a variable from the cfg file.
+        """Delete a variable from the configuration object.
+
+        Note:
+            Since we use the ConfigParser class, the section names are case-sensitive but the keys are not.
 
         Args:
             key: The key of the variable to be deleted.
-            section: section of the cfg. Defaults to self.namespace
+            section: The section of the configuration object to remove the variable from.
+                Defaults to None, in which case the default section is used.
+
+        Raises:
+            configparser.Error: If no section is specified and the default section is None.
+            configparser.NoSectionError: If the section does not exist in the configuration object.
+            configparser.NoOptionError: If the key does not exist in the section.
         """
+        # resolve the section
         if section is None:
             if self.namespace is None:
-                raise configparser.Error("No section specified")
+                raise configparser.Error("No section specified. Please specify a section or set StateFile.namespace.")
             section = self.namespace
+
+        # check if the section exists
         if section not in self.loaded_cfg.sections():
-            raise configparser.NoSectionError(f"Section {section} does not exist in {self.path}")
+            raise configparser.NoSectionError(f"Section '{section}' does not exist in the file: {self.path}")
+
+        # check if the key exists
         if self.loaded_cfg.has_option(section, key):
             self.loaded_cfg.remove_option(section, key)
         else:
             raise configparser.NoOptionError(option=key, section=section)
 
-    def load_cfg(self):
-        """
-        Calls load_cfg_file() to populate self.loaded_cfg with the
-        data stored at self.path
-        """
-        self.loaded_cfg = load_cfg_file(self.path)
+    """
+    Operations - File I/O.
+    """
 
-    def save_cfg(self):
-        """
-        Calls save_cfg_file() to write the contents of self.loaded_cfg
-        to the file at self.path
+    def load(self):
+        """Load the configuration file into memory.
+
+        This function reads the contents of the configuration file into memory.
+        If the file does not exist, it creates an empty file.
         """
-        save_cfg_file(self.path, self.loaded_cfg)
+        self.loaded_cfg = ConfigParser()
+        self.loaded_cfg.read(self.path)
+
+    def save(self):
+        """Save the configuration file to disk."""
+        with open(self.path, "w+") as f:
+            self.loaded_cfg.write(f)

docs/source/deployment/run_docker_example.rst

@@ -105,7 +105,7 @@ the following command to retrieve our logs from the Docker container and put the
 
 .. code-block:: console
 
-  python container.py copy
+  ./container.py copy
 
 
 We will see a terminal readout reporting the artifacts we have retrieved from the container. If we navigate to
@@ -120,12 +120,12 @@ but we have retrieved our logs and wish to go inspect them. We can stop the Isaa
 
 .. code-block:: console
 
-  python container.py stop
+  ./container.py stop
 
 
 This will bring down the Docker Isaac Lab container. The image will persist and remain available for further use, as will
 the contents of any `volumes`_. If we wish to free up the disk space taken by the image, (~20.1GB), and do not mind repeating
-the build process when we next run ``python container.py start``, we may enter the following command to delete the **isaac-lab-base** image:
+the build process when we next run ``./container.py start``, we may enter the following command to delete the **isaac-lab-base** image:
 
 .. code-block:: console