Re: [PATCH v4 7/8] dts: improve configuration API docs

[Nicholas Pratte <npratte@iol.unh.edu>]

Reviewed-by: Nicholas Pratte <npratte@iol.unh.edu>

On Mon, Oct 28, 2024 at 1:51 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Pydantic models are not treated the same way as dataclasses by autodoc.
> As a consequence the docstrings need to be applied directly to each
> field. Otherwise the generated API documentation page would present two
> entries per each field with each their own differences.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
>  doc/guides/tools/dts.rst         |   5 +-
>  dts/framework/config/__init__.py | 253 +++++++++++--------------------
>  2 files changed, 88 insertions(+), 170 deletions(-)
>
> diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
> index 7ccca63ae8..ac12c5c4fa 100644
> --- a/doc/guides/tools/dts.rst
> +++ b/doc/guides/tools/dts.rst
> @@ -1,5 +1,6 @@
>  ..  SPDX-License-Identifier: BSD-3-Clause
>      Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
> +    Copyright(c) 2024 Arm Limited
>
>  DPDK Test Suite
>  ===============
> @@ -327,8 +328,8 @@ where we deviate or where some additional clarification is helpful:
>     * The ``dataclass.dataclass`` decorator changes how the attributes are processed.
>       The dataclass attributes which result in instance variables/attributes
>       should also be recorded in the ``Attributes:`` section.
> -   * Class variables/attributes, on the other hand, should be documented with ``#:``
> -     above the type annotated line.
> +   * Class variables/attributes and Pydantic model fields, on the other hand, should be documented
> +     with ``#:`` above the type annotated line.
>       The description may be omitted if the meaning is obvious.
>     * The ``Enum`` and ``TypedDict`` also process the attributes in particular ways
>       and should be documented with ``#:`` as well.
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index c86bfaaabf..d7d3907a33 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -116,54 +116,34 @@ class TrafficGeneratorType(str, Enum):
>
>
>  class HugepageConfiguration(BaseModel, frozen=True, extra="forbid"):
> -    r"""The hugepage configuration of :class:`~framework.testbed_model.node.Node`\s.
> -
> -    Attributes:
> -        number_of: The number of hugepages to allocate.
> -        force_first_numa: If :data:`True`, the hugepages will be configured on the first NUMA node.
> -    """
> +    r"""The hugepage configuration of :class:`~framework.testbed_model.node.Node`\s."""
>
> +    #: The number of hugepages to allocate.
>      number_of: int
> +    #: If :data:`True`, the hugepages will be configured on the first NUMA node.
>      force_first_numa: bool
>
>
>  class PortConfig(BaseModel, frozen=True, extra="forbid"):
> -    r"""The port configuration of :class:`~framework.testbed_model.node.Node`\s.
> -
> -    Attributes:
> -        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.
> -    """
> +    r"""The port configuration of :class:`~framework.testbed_model.node.Node`\s."""
>
> -    pci: str = Field(
> -        description="The local PCI address of the port.", pattern=REGEX_FOR_PCI_ADDRESS
> -    )
> -    os_driver_for_dpdk: str = Field(
> -        description="The driver that the kernel should bind this device to for DPDK to use it.",
> -        examples=["vfio-pci", "mlx5_core"],
> -    )
> -    os_driver: str = Field(
> -        description="The driver normally used by this port", examples=["i40e", "ice", "mlx5_core"]
> -    )
> -    peer_node: str = Field(description="The name of the peer node this port is connected to.")
> -    peer_pci: str = Field(
> -        description="The PCI address of the peer port this port is connected to.",
> -        pattern=REGEX_FOR_PCI_ADDRESS,
> -    )
> +    #: The PCI address of the port.
> +    pci: str = Field(pattern=REGEX_FOR_PCI_ADDRESS)
> +    #: The driver that the kernel should bind this device to for DPDK to use it.
> +    os_driver_for_dpdk: str = Field(examples=["vfio-pci", "mlx5_core"])
> +    #: The operating system driver name when the operating system controls the port.
> +    os_driver: str = Field(examples=["i40e", "ice", "mlx5_core"])
> +    #: The name of the peer node this port is connected to.
> +    peer_node: str
> +    #: The PCI address of the peer port connected to this port.
> +    peer_pci: str = Field(pattern=REGEX_FOR_PCI_ADDRESS)
>
>
>  class TrafficGeneratorConfig(BaseModel, frozen=True, extra="forbid"):
> -    """A protocol required to define traffic generator types.
> -
> -    Attributes:
> -        type: The traffic generator type, the child class is required to define to be distinguished
> -            among others.
> -    """
> +    """A protocol required to define traffic generator types."""
>
> +    #: The traffic generator type the child class is required to define to be distinguished among
> +    #: others.
>      type: TrafficGeneratorType
>
>
> @@ -176,13 +156,10 @@ class ScapyTrafficGeneratorConfig(TrafficGeneratorConfig, frozen=True, extra="fo
>  #: A union type discriminating traffic generators by the `type` field.
>  TrafficGeneratorConfigTypes = Annotated[ScapyTrafficGeneratorConfig, Field(discriminator="type")]
>
> -
> -#: A field representing logical core ranges.
> +#: Comma-separated list of logical cores to use. An empty string means use all lcores.
>  LogicalCores = Annotated[
>      str,
>      Field(
> -        description="Comma-separated list of logical cores to use. "
> -        "An empty string means use all lcores.",
>          examples=["1,2,3,4,5,18-22", "10-15"],
>          pattern=r"^(([0-9]+|([0-9]+-[0-9]+))(,([0-9]+|([0-9]+-[0-9]+)))*)?$",
>      ),
> @@ -190,61 +167,41 @@ class ScapyTrafficGeneratorConfig(TrafficGeneratorConfig, frozen=True, extra="fo
>
>
>  class NodeConfiguration(BaseModel, frozen=True, extra="forbid"):
> -    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 = Field(description="A unique identifier for this node.")
> -    hostname: str = Field(description="The hostname or IP address of the node.")
> -    user: str = Field(description="The login user to use to connect to this node.")
> -    password: str | None = Field(
> -        default=None,
> -        description="The login password to use to connect to this node. "
> -        "SSH keys are STRONGLY preferred, use only as last resort.",
> -    )
> +    r"""The configuration of :class:`~framework.testbed_model.node.Node`\s."""
> +
> +    #: The name of the :class:`~framework.testbed_model.node.Node`.
> +    name: str
> +    #: The hostname of the :class:`~framework.testbed_model.node.Node`. Can also be an IP address.
> +    hostname: str
> +    #: The name of the user used to connect to the :class:`~framework.testbed_model.node.Node`.
> +    user: str
> +    #: The password of the user. The use of passwords is heavily discouraged, please use SSH keys.
> +    password: str | None = None
> +    #: The architecture of the :class:`~framework.testbed_model.node.Node`.
>      arch: Architecture
> +    #: The operating system of the :class:`~framework.testbed_model.node.Node`.
>      os: OS
> +    #: A comma delimited list of logical cores to use when running DPDK.
>      lcores: LogicalCores = "1"
> -    use_first_core: bool = Field(
> -        default=False, description="DPDK won't use the first physical core if set to False."
> -    )
> +    #: If :data:`True`, the first logical core won't be used.
> +    use_first_core: bool = False
> +    #: An optional hugepage configuration.
>      hugepages: HugepageConfiguration | None = Field(None, alias="hugepages_2mb")
> +    #: The ports that can be used in testing.
>      ports: list[PortConfig] = Field(min_length=1)
>
>
>  class SutNodeConfiguration(NodeConfiguration, frozen=True, extra="forbid"):
> -    """:class:`~framework.testbed_model.sut_node.SutNode` specific configuration.
> +    """: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 = Field(
> -        default=1, description="Number of memory channels to use when running DPDK."
> -    )
> +    #: The number of memory channels to use when running DPDK.
> +    memory_channels: int = 1
>
>
>  class TGNodeConfiguration(NodeConfiguration, frozen=True, extra="forbid"):
> -    """:class:`~framework.testbed_model.tg_node.TGNode` specific configuration.
> -
> -    Attributes:
> -        traffic_generator: The configuration of the traffic generator present on the TG node.
> -    """
> +    """:class:`~framework.testbed_model.tg_node.TGNode` specific configuration."""
>
> +    #: The configuration of the traffic generator present on the TG node.
>      traffic_generator: TrafficGeneratorConfigTypes
>
>
> @@ -258,20 +215,18 @@ def resolve_path(path: Path) -> Path:
>
>
>  class BaseDPDKLocation(BaseModel, frozen=True, extra="forbid"):
> -    """DPDK location.
> +    """DPDK location base class.
>
> -    The path to the DPDK sources, build dir and type of location.
> -
> -    Attributes:
> -        remote: Optional, defaults to :data:`False`. If :data:`True`, `dpdk_tree` or `tarball` is
> -            located on the SUT node, instead of the execution host.
> +    The path to the DPDK sources and type of location.
>      """
>
> +    #: Specifies whether to find DPDK on the SUT node or on the local host. Which are respectively
> +    #: represented by :class:`RemoteDPDKLocation` and :class:`LocalDPDKTreeLocation`.
>      remote: bool = False
>
>
>  class LocalDPDKLocation(BaseDPDKLocation, frozen=True, extra="forbid"):
> -    """Local DPDK location parent class.
> +    """Local DPDK location base class.
>
>      This class is meant to represent any location that is present only locally.
>      """
> @@ -284,14 +239,12 @@ class LocalDPDKTreeLocation(LocalDPDKLocation, frozen=True, extra="forbid"):
>
>      This class makes a distinction from :class:`RemoteDPDKTreeLocation` by enforcing on the fly
>      validation.
> -
> -    Attributes:
> -        dpdk_tree: The path to the DPDK source tree directory.
>      """
>
> +    #: The path to the DPDK source tree directory on the local host passed as string.
>      dpdk_tree: Path
>
> -    #: Resolve the local DPDK tree path
> +    #: Resolve the local DPDK tree path.
>      resolve_dpdk_tree_path = field_validator("dpdk_tree")(resolve_path)
>
>      @model_validator(mode="after")
> @@ -307,14 +260,12 @@ class LocalDPDKTarballLocation(LocalDPDKLocation, frozen=True, extra="forbid"):
>
>      This class makes a distinction from :class:`RemoteDPDKTarballLocation` by enforcing on the fly
>      validation.
> -
> -    Attributes:
> -        tarball: The path to the DPDK tarball.
>      """
>
> +    #: The path to the DPDK tarball on the local host passed as string.
>      tarball: Path
>
> -    #: Resolve the local tarball path
> +    #: Resolve the local tarball path.
>      resolve_tarball_path = field_validator("tarball")(resolve_path)
>
>      @model_validator(mode="after")
> @@ -326,7 +277,7 @@ def validate_tarball_path(self) -> Self:
>
>
>  class RemoteDPDKLocation(BaseDPDKLocation, frozen=True, extra="forbid"):
> -    """Remote DPDK location parent class.
> +    """Remote DPDK location base class.
>
>      This class is meant to represent any location that is present only remotely.
>      """
> @@ -338,11 +289,9 @@ class RemoteDPDKTreeLocation(RemoteDPDKLocation, frozen=True, extra="forbid"):
>      """Remote DPDK tree location.
>
>      This class is distinct from :class:`LocalDPDKTreeLocation` which enforces on the fly validation.
> -
> -    Attributes:
> -        dpdk_tree: The path to the DPDK source tree directory.
>      """
>
> +    #: The path to the DPDK source tree directory on the remote node passed as string.
>      dpdk_tree: PurePath
>
>
> @@ -351,11 +300,9 @@ class RemoteDPDKTarballLocation(LocalDPDKLocation, frozen=True, extra="forbid"):
>
>      This class is distinct from :class:`LocalDPDKTarballLocation` which enforces on the fly
>      validation.
> -
> -    Attributes:
> -        tarball: The path to the DPDK tarball.
>      """
>
> +    #: The path to the DPDK tarball on the remote node passed as string.
>      tarball: PurePath
>
>
> @@ -372,23 +319,17 @@ class BaseDPDKBuildConfiguration(BaseModel, frozen=True, extra="forbid"):
>      """The base configuration for different types of build.
>
>      The configuration contain the location of the DPDK and configuration used for building it.
> -
> -    Attributes:
> -        dpdk_location: The location of the DPDK tree.
>      """
>
> +    #: The location of the DPDK tree.
>      dpdk_location: DPDKLocation
>
>
>  class DPDKPrecompiledBuildConfiguration(BaseDPDKBuildConfiguration, frozen=True, extra="forbid"):
> -    """DPDK precompiled build configuration.
> -
> -    Attributes:
> -        precompiled_build_dir: If it's defined, DPDK has been pre-compiled and the build directory
> -            is located in a subdirectory of `dpdk_tree` or `tarball` root directory. Otherwise, will
> -            be using `dpdk_build_config` from configuration to build the DPDK from source.
> -    """
> +    """DPDK precompiled build configuration."""
>
> +    #: If it's defined, DPDK has been pre-compiled and the build directory is located in a
> +    #: subdirectory of `~dpdk_location.dpdk_tree` or `~dpdk_location.tarball` root directory.
>      precompiled_build_dir: str = Field(min_length=1)
>
>
> @@ -396,20 +337,18 @@ class DPDKBuildOptionsConfiguration(BaseModel, frozen=True, extra="forbid"):
>      """DPDK build options configuration.
>
>      The build options 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``.
>      """
>
> +    #: The target architecture to build for.
>      arch: Architecture
> +    #: The target OS to build for.
>      os: OS
> +    #: The target CPU to build for.
>      cpu: CPUType
> +    #: The compiler executable to use.
>      compiler: Compiler
> +    #: This string will be put in front of the compiler when executing the build. Useful for adding
> +    #: wrapper commands, such as ``ccache``.
>      compiler_wrapper: str = ""
>
>      @cached_property
> @@ -419,12 +358,9 @@ def name(self) -> str:
>
>
>  class DPDKUncompiledBuildConfiguration(BaseDPDKBuildConfiguration, frozen=True, extra="forbid"):
> -    """DPDK uncompiled build configuration.
> -
> -    Attributes:
> -        build_options: The build options to compile DPDK.
> -    """
> +    """DPDK uncompiled build configuration."""
>
> +    #: The build options to compiled DPDK with.
>      build_options: DPDKBuildOptionsConfiguration
>
>
> @@ -448,24 +384,13 @@ class TestSuiteConfig(BaseModel, frozen=True, extra="forbid"):
>              # or as model fields:
>              - test_suite: hello_world
>                test_cases: [hello_world_single_core] # without this field all test cases are run
> -
> -    Attributes:
> -        test_suite_name: The name of the test suite module without the starting ``TestSuite_``.
> -        test_cases_names: The names of test cases from this test suite to execute.
> -            If empty, all test cases will be executed.
>      """
>
> -    test_suite_name: str = Field(
> -        title="Test suite name",
> -        description="The identifying module name of the test suite without the prefix.",
> -        alias="test_suite",
> -    )
> -    test_cases_names: list[str] = Field(
> -        default_factory=list,
> -        title="Test cases by name",
> -        description="The identifying name of the test cases of the test suite.",
> -        alias="test_cases",
> -    )
> +    #: The name of the test suite module without the starting ``TestSuite_``.
> +    test_suite_name: str = Field(alias="test_suite")
> +    #: The names of test cases from this test suite to execute. If empty, all test cases will be
> +    #: executed.
> +    test_cases_names: list[str] = Field(default_factory=list, alias="test_cases")
>
>      @cached_property
>      def test_suite_spec(self) -> "TestSuiteSpec":
> @@ -507,14 +432,11 @@ def validate_names(self) -> Self:
>
>
>  class TestRunSUTNodeConfiguration(BaseModel, frozen=True, extra="forbid"):
> -    """The SUT node configuration of a test run.
> -
> -    Attributes:
> -        node_name: The SUT node to use in this test run.
> -        vdevs: The names of virtual devices to test.
> -    """
> +    """The SUT node configuration of a test run."""
>
> +    #: The SUT node to use in this test run.
>      node_name: str
> +    #: The names of virtual devices to test.
>      vdevs: list[str] = Field(default_factory=list)
>
>
> @@ -523,25 +445,23 @@ class TestRunConfiguration(BaseModel, frozen=True, extra="forbid"):
>
>      The configuration contains testbed information, what tests to execute
>      and with what DPDK build.
> -
> -    Attributes:
> -        dpdk_config: The DPDK configuration used 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 configuration to use in this test run.
> -        traffic_generator_node: The TG node name to use in this test run.
> -        random_seed: The seed to use for pseudo-random generation.
>      """
>
> +    #: The DPDK configuration used to test.
>      dpdk_config: DPDKBuildConfiguration = Field(alias="dpdk_build")
> -    perf: bool = Field(description="Enable performance testing.")
> -    func: bool = Field(description="Enable functional testing.")
> +    #: Whether to run performance tests.
> +    perf: bool
> +    #: Whether to run functional tests.
> +    func: bool
> +    #: Whether to skip smoke tests.
>      skip_smoke_tests: bool = False
> +    #: The names of test suites and/or test cases to execute.
>      test_suites: list[TestSuiteConfig] = Field(min_length=1)
> +    #: The SUT node configuration to use in this test run.
>      system_under_test_node: TestRunSUTNodeConfiguration
> +    #: The TG node name to use in this test run.
>      traffic_generator_node: str
> +    #: The seed to use for pseudo-random generation.
>      random_seed: int | None = None
>
>
> @@ -557,14 +477,11 @@ class TestRunWithNodesConfiguration(NamedTuple):
>
>
>  class Configuration(BaseModel, extra="forbid"):
> -    """DTS testbed and test configuration.
> -
> -    Attributes:
> -        test_runs: Test run configurations.
> -        nodes: Node configurations.
> -    """
> +    """DTS testbed and test configuration."""
>
> +    #: Test run configurations.
>      test_runs: list[TestRunConfiguration] = Field(min_length=1)
> +    #: Node configurations.
>      nodes: list[NodeConfigurationTypes] = Field(min_length=1)
>
>      @cached_property
> --
> 2.43.0
>