Re: [PATCH v7 10/21] dts: config docstring update

[Yoan Picchi <yoan.picchi@foss.arm.com>]

On 11/15/23 13:09, Juraj Linkeš wrote:
> Format according to the Google format and PEP257, with slight
> deviations.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>   dts/framework/config/__init__.py | 371 ++++++++++++++++++++++++++-----
>   dts/framework/config/types.py    | 132 +++++++++++
>   2 files changed, 446 insertions(+), 57 deletions(-)
>   create mode 100644 dts/framework/config/types.py
> 
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index 2044c82611..0aa149a53d 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -3,8 +3,34 @@
>   # Copyright(c) 2022-2023 University of New Hampshire
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   
> -"""
> -Yaml config parsing methods
> +"""Testbed configuration and test suite specification.
> +
> +This package offers classes that hold real-time information about the testbed, hold test run
> +configuration describing the tested testbed and a loader function, :func:`load_config`, which loads
> +the YAML test run configuration file
> +and validates it according to :download:`the schema <conf_yaml_schema.json>`.
> +
> +The YAML test run configuration file is parsed into a dictionary, parts of which are used throughout
> +this package. The allowed keys and types inside this dictionary are defined in
> +the :doc:`types <framework.config.types>` module.
> +
> +The test run configuration has two main sections:
> +
> +    * The :class:`ExecutionConfiguration` which defines what tests are going to be run
> +      and how DPDK will be built. It also references the testbed where these tests and DPDK
> +      are going to be run,
> +    * The nodes of the testbed are defined in the other section,
> +      a :class:`list` of :class:`NodeConfiguration` objects.
> +
> +The real-time information about testbed is supposed to be gathered at runtime.
> +
> +The classes defined in this package make heavy use of :mod:`dataclasses`.
> +All of them use slots and are frozen:
> +
> +    * Slots enables some optimizations, by pre-allocating space for the defined
> +      attributes in the underlying data structure,
> +    * Frozen makes the object immutable. This enables further optimizations,
> +      and makes it thread safe should we every want to move in that direction.

every -> ever ?

>   """
>   
>   import json
> @@ -12,11 +38,20 @@
>   import pathlib
>   from dataclasses import dataclass
>   from enum import auto, unique
> -from typing import Any, TypedDict, Union
> +from typing import Union
>   
>   import warlock  # type: ignore[import]
>   import yaml
>   
> +from framework.config.types import (
> +    BuildTargetConfigDict,
> +    ConfigurationDict,
> +    ExecutionConfigDict,
> +    NodeConfigDict,
> +    PortConfigDict,
> +    TestSuiteConfigDict,
> +    TrafficGeneratorConfigDict,
> +)
>   from framework.exception import ConfigurationError
>   from framework.settings import SETTINGS
>   from framework.utils import StrEnum
> @@ -24,55 +59,97 @@
>   
>   @unique
>   class Architecture(StrEnum):
> +    r"""The supported architectures of :class:`~framework.testbed_model.node.Node`\s."""
> +
> +    #:
>       i686 = auto()
> +    #:
>       x86_64 = auto()
> +    #:
>       x86_32 = auto()
> +    #:
>       arm64 = auto()
> +    #:
>       ppc64le = auto()
>   
>   
>   @unique
>   class OS(StrEnum):
> +    r"""The supported operating systems of :class:`~framework.testbed_model.node.Node`\s."""
> +
> +    #:
>       linux = auto()
> +    #:
>       freebsd = auto()
> +    #:
>       windows = auto()
>   
>   
>   @unique
>   class CPUType(StrEnum):
> +    r"""The supported CPUs of :class:`~framework.testbed_model.node.Node`\s."""
> +
> +    #:
>       native = auto()
> +    #:
>       armv8a = auto()
> +    #:
>       dpaa2 = auto()
> +    #:
>       thunderx = auto()
> +    #:
>       xgene1 = auto()
>   
>   
>   @unique
>   class Compiler(StrEnum):
> +    r"""The supported compilers of :class:`~framework.testbed_model.node.Node`\s."""
> +
> +    #:
>       gcc = auto()
> +    #:
>       clang = auto()
> +    #:
>       icc = auto()
> +    #:
>       msvc = auto()
>   
>   
>   @unique
>   class TrafficGeneratorType(StrEnum):
> +    """The supported traffic generators."""
> +
> +    #:
>       SCAPY = auto()
>   
>   
> -# Slots enables some optimizations, by pre-allocating space for the defined
> -# attributes in the underlying data structure.
> -#
> -# Frozen makes the object immutable. This enables further optimizations,
> -# and makes it thread safe should we every want to move in that direction.
>   @dataclass(slots=True, frozen=True)
>   class HugepageConfiguration:
> +    r"""The hugepage configuration of :class:`~framework.testbed_model.node.Node`\s.
> +
> +    Attributes:
> +        amount: The number of hugepages.
> +        force_first_numa: If :data:`True`, the hugepages will be configured on the first NUMA node.
> +    """
> +
>       amount: int
>       force_first_numa: bool
>   
>   
>   @dataclass(slots=True, frozen=True)
>   class PortConfig:
> +    r"""The port configuration of :class:`~framework.testbed_model.node.Node`\s.
> +
> +    Attributes:
> +        node: The :class:`~framework.testbed_model.node.Node` where this port exists.
> +        pci: The PCI address of the port.
> +        os_driver_for_dpdk: The operating system driver name for use with DPDK.
> +        os_driver: The operating system driver name when the operating system controls the port.
> +        peer_node: The :class:`~framework.testbed_model.node.Node` of the port
> +            connected to this port.
> +        peer_pci: The PCI address of the port connected to this port.
> +    """
> +
>       node: str
>       pci: str
>       os_driver_for_dpdk: str
> @@ -81,18 +158,44 @@ class PortConfig:
>       peer_pci: str
>   
>       @staticmethod
> -    def from_dict(node: str, d: dict) -> "PortConfig":
> +    def from_dict(node: str, d: PortConfigDict) -> "PortConfig":
> +        """A convenience method that creates the object from fewer inputs.
> +
> +        Args:
> +            node: The node where this port exists.
> +            d: The configuration dictionary.
> +
> +        Returns:
> +            The port configuration instance.
> +        """
>           return PortConfig(node=node, **d)
>   
>   
>   @dataclass(slots=True, frozen=True)
>   class TrafficGeneratorConfig:
> +    """The configuration of traffic generators.
> +
> +    The class will be expanded when more configuration is needed.
> +
> +    Attributes:
> +        traffic_generator_type: The type of the traffic generator.
> +    """
> +
>       traffic_generator_type: TrafficGeneratorType
>   
>       @staticmethod
> -    def from_dict(d: dict) -> "ScapyTrafficGeneratorConfig":
> -        # This looks useless now, but is designed to allow expansion to traffic
> -        # generators that require more configuration later.
> +    def from_dict(d: TrafficGeneratorConfigDict) -> "ScapyTrafficGeneratorConfig":
> +        """A convenience method that produces traffic generator config of the proper type.
> +
> +        Args:
> +            d: The configuration dictionary.
> +
> +        Returns:
> +            The traffic generator configuration instance.
> +
> +        Raises:
> +            ConfigurationError: An unknown traffic generator type was encountered.
> +        """
>           match TrafficGeneratorType(d["type"]):
>               case TrafficGeneratorType.SCAPY:
>                   return ScapyTrafficGeneratorConfig(
> @@ -106,11 +209,31 @@ def from_dict(d: dict) -> "ScapyTrafficGeneratorConfig":
>   
>   @dataclass(slots=True, frozen=True)
>   class ScapyTrafficGeneratorConfig(TrafficGeneratorConfig):
> +    """Scapy traffic generator specific configuration."""
> +
>       pass
>   
>   
>   @dataclass(slots=True, frozen=True)
>   class NodeConfiguration:
> +    r"""The configuration of :class:`~framework.testbed_model.node.Node`\s.
> +
> +    Attributes:
> +        name: The name of the :class:`~framework.testbed_model.node.Node`.
> +        hostname: The hostname of the :class:`~framework.testbed_model.node.Node`.
> +            Can be an IP or a domain name.
> +        user: The name of the user used to connect to
> +            the :class:`~framework.testbed_model.node.Node`.
> +        password: The password of the user. The use of passwords is heavily discouraged.
> +            Please use keys instead.
> +        arch: The architecture of the :class:`~framework.testbed_model.node.Node`.
> +        os: The operating system of the :class:`~framework.testbed_model.node.Node`.
> +        lcores: A comma delimited list of logical cores to use when running DPDK.
> +        use_first_core: If :data:`True`, the first logical core won't be used.
> +        hugepages: An optional hugepage configuration.
> +        ports: The ports that can be used in testing.
> +    """
> +
>       name: str
>       hostname: str
>       user: str
> @@ -123,57 +246,91 @@ class NodeConfiguration:
>       ports: list[PortConfig]
>   
>       @staticmethod
> -    def from_dict(d: dict) -> Union["SutNodeConfiguration", "TGNodeConfiguration"]:
> -        hugepage_config = d.get("hugepages")
> -        if hugepage_config:
> -            if "force_first_numa" not in hugepage_config:
> -                hugepage_config["force_first_numa"] = False
> -            hugepage_config = HugepageConfiguration(**hugepage_config)
> -
> -        common_config = {
> -            "name": d["name"],
> -            "hostname": d["hostname"],
> -            "user": d["user"],
> -            "password": d.get("password"),
> -            "arch": Architecture(d["arch"]),
> -            "os": OS(d["os"]),
> -            "lcores": d.get("lcores", "1"),
> -            "use_first_core": d.get("use_first_core", False),
> -            "hugepages": hugepage_config,
> -            "ports": [PortConfig.from_dict(d["name"], port) for port in d["ports"]],
> -        }
> -
> +    def from_dict(
> +        d: NodeConfigDict,
> +    ) -> Union["SutNodeConfiguration", "TGNodeConfiguration"]:
> +        """A convenience method that processes the inputs before creating a specialized instance.
> +
> +        Args:
> +            d: The configuration dictionary.
> +
> +        Returns:
> +            Either an SUT or TG configuration instance.
> +        """
> +        hugepage_config = None
> +        if "hugepages" in d:
> +            hugepage_config_dict = d["hugepages"]
> +            if "force_first_numa" not in hugepage_config_dict:
> +                hugepage_config_dict["force_first_numa"] = False
> +            hugepage_config = HugepageConfiguration(**hugepage_config_dict)
> +
> +        # The calls here contain duplicated code which is here because Mypy doesn't
> +        # properly support dictionary unpacking with TypedDicts
>           if "traffic_generator" in d:
>               return TGNodeConfiguration(
> +                name=d["name"],
> +                hostname=d["hostname"],
> +                user=d["user"],
> +                password=d.get("password"),
> +                arch=Architecture(d["arch"]),
> +                os=OS(d["os"]),
> +                lcores=d.get("lcores", "1"),
> +                use_first_core=d.get("use_first_core", False),
> +                hugepages=hugepage_config,
> +                ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
>                   traffic_generator=TrafficGeneratorConfig.from_dict(
>                       d["traffic_generator"]
>                   ),
> -                **common_config,
>               )
>           else:
>               return SutNodeConfiguration(
> -                memory_channels=d.get("memory_channels", 1), **common_config
> +                name=d["name"],
> +                hostname=d["hostname"],
> +                user=d["user"],
> +                password=d.get("password"),
> +                arch=Architecture(d["arch"]),
> +                os=OS(d["os"]),
> +                lcores=d.get("lcores", "1"),
> +                use_first_core=d.get("use_first_core", False),
> +                hugepages=hugepage_config,
> +                ports=[PortConfig.from_dict(d["name"], port) for port in d["ports"]],
> +                memory_channels=d.get("memory_channels", 1),
>               )
>   
>   
>   @dataclass(slots=True, frozen=True)
>   class SutNodeConfiguration(NodeConfiguration):
> +    """:class:`~framework.testbed_model.sut_node.SutNode` specific configuration.
> +
> +    Attributes:
> +        memory_channels: The number of memory channels to use when running DPDK.
> +    """
> +
>       memory_channels: int
>   
>   
>   @dataclass(slots=True, frozen=True)
>   class TGNodeConfiguration(NodeConfiguration):
> +    """:class:`~framework.testbed_model.tg_node.TGNode` specific configuration.
> +
> +    Attributes:
> +        traffic_generator: The configuration of the traffic generator present on the TG node.
> +    """
> +
>       traffic_generator: ScapyTrafficGeneratorConfig
>   
>   
>   @dataclass(slots=True, frozen=True)
>   class NodeInfo:
> -    """Class to hold important versions within the node.
> -
> -    This class, unlike the NodeConfiguration class, cannot be generated at the start.
> -    This is because we need to initialize a connection with the node before we can
> -    collect the information needed in this class. Therefore, it cannot be a part of
> -    the configuration class above.
> +    """Supplemental node information.
> +
> +    Attributes:
> +        os_name: The name of the running operating system of
> +            the :class:`~framework.testbed_model.node.Node`.
> +        os_version: The version of the running operating system of
> +            the :class:`~framework.testbed_model.node.Node`.
> +        kernel_version: The kernel version of the running operating system of
> +            the :class:`~framework.testbed_model.node.Node`.
>       """
>   
>       os_name: str
> @@ -183,6 +340,20 @@ class NodeInfo:
>   
>   @dataclass(slots=True, frozen=True)
>   class BuildTargetConfiguration:
> +    """DPDK build configuration.
> +
> +    The configuration used for building DPDK.
> +
> +    Attributes:
> +        arch: The target architecture to build for.
> +        os: The target os to build for.
> +        cpu: The target CPU to build for.
> +        compiler: The compiler executable to use.
> +        compiler_wrapper: This string will be put in front of the compiler when
> +            executing the build. Useful for adding wrapper commands, such as ``ccache``.
> +        name: The name of the compiler.
> +    """
> +
>       arch: Architecture
>       os: OS
>       cpu: CPUType
> @@ -191,7 +362,18 @@ class BuildTargetConfiguration:
>       name: str
>   
>       @staticmethod
> -    def from_dict(d: dict) -> "BuildTargetConfiguration":
> +    def from_dict(d: BuildTargetConfigDict) -> "BuildTargetConfiguration":
> +        r"""A convenience method that processes the inputs before creating an instance.
> +
> +        `arch`, `os`, `cpu` and `compiler` are converted to :class:`Enum`\s and
> +        `name` is constructed from `arch`, `os`, `cpu` and `compiler`.
> +
> +        Args:
> +            d: The configuration dictionary.
> +
> +        Returns:
> +            The build target configuration instance.
> +        """
>           return BuildTargetConfiguration(
>               arch=Architecture(d["arch"]),
>               os=OS(d["os"]),
> @@ -204,23 +386,29 @@ def from_dict(d: dict) -> "BuildTargetConfiguration":
>   
>   @dataclass(slots=True, frozen=True)
>   class BuildTargetInfo:
> -    """Class to hold important versions within the build target.
> +    """Various versions and other information about a build target.
>   
> -    This is very similar to the NodeInfo class, it just instead holds information
> -    for the build target.
> +    Attributes:
> +        dpdk_version: The DPDK version that was built.
> +        compiler_version: The version of the compiler used to build DPDK.
>       """
>   
>       dpdk_version: str
>       compiler_version: str
>   
>   
> -class TestSuiteConfigDict(TypedDict):
> -    suite: str
> -    cases: list[str]
> -
> -
>   @dataclass(slots=True, frozen=True)
>   class TestSuiteConfig:
> +    """Test suite configuration.
> +
> +    Information about a single test suite to be executed.
> +
> +    Attributes:
> +        test_suite: The name of the test suite module without the starting ``TestSuite_``.
> +        test_cases: The names of test cases from this test suite to execute.
> +            If empty, all test cases will be executed.
> +    """
> +
>       test_suite: str
>       test_cases: list[str]
>   
> @@ -228,6 +416,14 @@ class TestSuiteConfig:
>       def from_dict(
>           entry: str | TestSuiteConfigDict,
>       ) -> "TestSuiteConfig":
> +        """Create an instance from two different types.
> +
> +        Args:
> +            entry: Either a suite name or a dictionary containing the config.
> +
> +        Returns:
> +            The test suite configuration instance.
> +        """
>           if isinstance(entry, str):
>               return TestSuiteConfig(test_suite=entry, test_cases=[])
>           elif isinstance(entry, dict):
> @@ -238,19 +434,49 @@ def from_dict(
>   
>   @dataclass(slots=True, frozen=True)
>   class ExecutionConfiguration:
> +    """The configuration of an execution.
> +
> +    The configuration contains testbed information, what tests to execute
> +    and with what DPDK build.
> +
> +    Attributes:
> +        build_targets: A list of DPDK builds to test.
> +        perf: Whether to run performance tests.
> +        func: Whether to run functional tests.
> +        skip_smoke_tests: Whether to skip smoke tests.
> +        test_suites: The names of test suites and/or test cases to execute.
> +        system_under_test_node: The SUT node to use in this execution.
> +        traffic_generator_node: The TG node to use in this execution.
> +        vdevs: The names of virtual devices to test.
> +    """
> +
>       build_targets: list[BuildTargetConfiguration]
>       perf: bool
>       func: bool
> +    skip_smoke_tests: bool
>       test_suites: list[TestSuiteConfig]
>       system_under_test_node: SutNodeConfiguration
>       traffic_generator_node: TGNodeConfiguration
>       vdevs: list[str]
> -    skip_smoke_tests: bool
>   
>       @staticmethod
>       def from_dict(
> -        d: dict, node_map: dict[str, Union[SutNodeConfiguration | TGNodeConfiguration]]
> +        d: ExecutionConfigDict,
> +        node_map: dict[str, Union[SutNodeConfiguration | TGNodeConfiguration]],
>       ) -> "ExecutionConfiguration":
> +        """A convenience method that processes the inputs before creating an instance.
> +
> +        The build target and the test suite config is transformed into their respective objects.

is -> are

> +        SUT and TG configuration are taken from `node_map`. The other (:class:`bool`) attributes are

configuration*s*

> +        just stored.
> +
> +        Args:
> +            d: The configuration dictionary.
> +            node_map: A dictionary mapping node names to their config objects.
> +
> +        Returns:
> +            The execution configuration instance.
> +        """
>           build_targets: list[BuildTargetConfiguration] = list(
>               map(BuildTargetConfiguration.from_dict, d["build_targets"])
>           )
> @@ -291,10 +517,31 @@ def from_dict(
>   
>   @dataclass(slots=True, frozen=True)
>   class Configuration:
> +    """DTS testbed and test configuration.
> +
> +    The node configuration is not stored in this object. Rather, all used node configurations
> +    are stored inside the execution configuration where the nodes are actually used.
> +
> +    Attributes:
> +        executions: Execution configurations.
> +    """
> +
>       executions: list[ExecutionConfiguration]
>   
>       @staticmethod
> -    def from_dict(d: dict) -> "Configuration":
> +    def from_dict(d: ConfigurationDict) -> "Configuration":
> +        """A convenience method that processes the inputs before creating an instance.
> +
> +        Build target and test suite config is transformed into their respective objects.

is -> are

> +        SUT and TG configuration are taken from `node_map`. The other (:class:`bool`) attributes are

configuration*s*

> +        just stored.
> +
> +        Args:
> +            d: The configuration dictionary.
> +
> +        Returns:
> +            The whole configuration instance.
> +        """
>           nodes: list[Union[SutNodeConfiguration | TGNodeConfiguration]] = list(
>               map(NodeConfiguration.from_dict, d["nodes"])
>           )
> @@ -313,9 +560,17 @@ def from_dict(d: dict) -> "Configuration":
>   
>   
>   def load_config() -> Configuration:
> -    """
> -    Loads the configuration file and the configuration file schema,
> -    validates the configuration file, and creates a configuration object.
> +    """Load DTS test run configuration from a file.
> +
> +    Load the YAML test run configuration file
> +    and :download:`the configuration file schema <conf_yaml_schema.json>`,
> +    validate the test run configuration file, and create a test run configuration object.
> +
> +    The YAML test run configuration file is specified in the :option:`--config-file` command line
> +    argument or the :envvar:`DTS_CFG_FILE` environment variable.
> +
> +    Returns:
> +        The parsed test run configuration.
>       """
>       with open(SETTINGS.config_file_path, "r") as f:
>           config_data = yaml.safe_load(f)
> @@ -326,6 +581,8 @@ def load_config() -> Configuration:
>   
>       with open(schema_path, "r") as f:
>           schema = json.load(f)
> -    config: dict[str, Any] = warlock.model_factory(schema, name="_Config")(config_data)
> -    config_obj: Configuration = Configuration.from_dict(dict(config))
> +    config = warlock.model_factory(schema, name="_Config")(config_data)
> +    config_obj: Configuration = Configuration.from_dict(
> +        dict(config)  # type: ignore[arg-type]
> +    )
>       return config_obj
> diff --git a/dts/framework/config/types.py b/dts/framework/config/types.py
> new file mode 100644
> index 0000000000..1927910d88
> --- /dev/null
> +++ b/dts/framework/config/types.py
> @@ -0,0 +1,132 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 PANTHEON.tech s.r.o.
> +
> +"""Configuration dictionary contents specification.
> +
> +These type definitions serve as documentation of the configuration dictionary contents.
> +
> +The definitions use the built-in :class:`~typing.TypedDict` construct.
> +"""
> +
> +from typing import TypedDict
> +
> +
> +class PortConfigDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    pci: str
> +    #:
> +    os_driver_for_dpdk: str
> +    #:
> +    os_driver: str
> +    #:
> +    peer_node: str
> +    #:
> +    peer_pci: str
> +
> +
> +class TrafficGeneratorConfigDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    type: str
> +
> +
> +class HugepageConfigurationDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    amount: int
> +    #:
> +    force_first_numa: bool
> +
> +
> +class NodeConfigDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    hugepages: HugepageConfigurationDict
> +    #:
> +    name: str
> +    #:
> +    hostname: str
> +    #:
> +    user: str
> +    #:
> +    password: str
> +    #:
> +    arch: str
> +    #:
> +    os: str
> +    #:
> +    lcores: str
> +    #:
> +    use_first_core: bool
> +    #:
> +    ports: list[PortConfigDict]
> +    #:
> +    memory_channels: int
> +    #:
> +    traffic_generator: TrafficGeneratorConfigDict
> +
> +
> +class BuildTargetConfigDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    arch: str
> +    #:
> +    os: str
> +    #:
> +    cpu: str
> +    #:
> +    compiler: str
> +    #:
> +    compiler_wrapper: str
> +
> +
> +class TestSuiteConfigDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    suite: str
> +    #:
> +    cases: list[str]
> +
> +
> +class ExecutionSUTConfigDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    node_name: str
> +    #:
> +    vdevs: list[str]
> +
> +
> +class ExecutionConfigDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    build_targets: list[BuildTargetConfigDict]
> +    #:
> +    perf: bool
> +    #:
> +    func: bool
> +    #:
> +    skip_smoke_tests: bool
> +    #:
> +    test_suites: TestSuiteConfigDict
> +    #:
> +    system_under_test_node: ExecutionSUTConfigDict
> +    #:
> +    traffic_generator_node: str
> +
> +
> +class ConfigurationDict(TypedDict):
> +    """Allowed keys and values."""
> +
> +    #:
> +    nodes: list[NodeConfigDict]
> +    #:
> +    executions: list[ExecutionConfigDict]