Re: [PATCH 3/5] dts: use Pydantic in the configuration

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

Hi Luca! See my comments below, thanks!

On Thu, Aug 22, 2024 at 12:40 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This change brings in Pydantic in place of Warlock. Pydantic offers
> a built-in model validation system in the classes, which allows for
> a more resilient and simpler code. As a consequence of this change:
>
> - most validation is now built-in
> - further validation is added to verify:
>   - cross referencing of node names and ports
>   - test suite and test cases names
> - dictionaries representing the config schema are removed
> - the config schema is no longer used for validation but kept as an
>   alternative format for the developer
> - the config schema can now be generated automatically from the
>   Pydantic models
> - the TrafficGeneratorType enum has been changed from inheriting
>   StrEnum to the native str and Enum. This change was necessary to
>   enable the discriminator for object unions
> - the structure of the classes has been slightly changed to perfectly
>   match the structure of the configuration files
> - updates the test suite argument to catch the ValidationError that
>   TestSuiteConfig can now raise
>
> Bugzilla ID: 1508
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
>  dts/framework/config/__init__.py              | 588 +++++++++---------
>  dts/framework/config/types.py                 | 132 ----
>  dts/framework/runner.py                       |  35 +-
>  dts/framework/settings.py                     |  16 +-
>  dts/framework/testbed_model/sut_node.py       |   2 +-
>  .../traffic_generator/__init__.py             |   4 +-
>  .../traffic_generator/traffic_generator.py    |   2 +-
>  7 files changed, 325 insertions(+), 454 deletions(-)
>  delete mode 100644 dts/framework/config/types.py
>
> diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> index df60a5030e..013c529829 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -2,17 +2,19 @@
>  # Copyright(c) 2010-2021 Intel Corporation
>  # Copyright(c) 2022-2023 University of New Hampshire
>  # Copyright(c) 2023 PANTHEON.tech s.r.o.
> +# Copyright(c) 2024 Arm Limited
>
>  """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 and validates it against the :class:`Configuration` Pydantic
> +dataclass model. The Pydantic model is also available as

Out of curiosity, what is the reason for maintaining use of
dataclasses here as opposed to creating BaseModel subclasses for the
Pydantic library? I suppose both implementations would lead to the
same result, but is it mostly for the sake of familiarity and
consistency that we're still using dataclasses here?

> +:download:`JSON 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.
> +this package. The allowed keys and types inside this dictionary map directly to the
> +:class:`Configuration` model, its fields and sub-models.
>
>  The test run configuration has two main sections:
>
> @@ -24,7 +26,7 @@
>
>  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`.
> +The classes defined in this package make heavy use of :mod:`pydantic.dataclasses`.
>  All of them use slots and are frozen:
>
>      * Slots enables some optimizations, by pre-allocating space for the defined
> @@ -33,29 +35,31 @@
>        and makes it thread safe should we ever want to move in that direction.
>  """
>
> -import json
> -import os.path
> -from dataclasses import dataclass, fields
> -from enum import auto, unique
> +from enum import Enum, auto, unique
> +from functools import cached_property
>  from pathlib import Path
> -from typing import Union
> +from typing import TYPE_CHECKING, Annotated, Any, Literal, NamedTuple, Protocol
>
> -import warlock  # type: ignore[import-untyped]
>  import yaml
> +from pydantic import (
> +    ConfigDict,
> +    Field,
> +    StringConstraints,
> +    TypeAdapter,
> +    ValidationError,
> +    field_validator,
> +    model_validator,
> +)
> +from pydantic.config import JsonDict
> +from pydantic.dataclasses import dataclass
>  from typing_extensions import Self
>
> -from framework.config.types import (
> -    BuildTargetConfigDict,
> -    ConfigurationDict,
> -    NodeConfigDict,
> -    PortConfigDict,
> -    TestRunConfigDict,
> -    TestSuiteConfigDict,
> -    TrafficGeneratorConfigDict,
> -)
>  from framework.exception import ConfigurationError
>  from framework.utils import StrEnum
>
> +if TYPE_CHECKING:
> +    from framework.test_suite import TestSuiteSpec
> +
>
>  @unique
>  class Architecture(StrEnum):
> @@ -116,14 +120,14 @@ class Compiler(StrEnum):
>
>
>  @unique
> -class TrafficGeneratorType(StrEnum):
> +class TrafficGeneratorType(str, Enum):
>      """The supported traffic generators."""
>
>      #:
> -    SCAPY = auto()
> +    SCAPY = "SCAPY"

Going off of Juraj's comments, would you be able to provide an deeper
explanation as how this new parameterization of str and enum works
with respect to the Pydantic field discriminators?
<snip>

The rest of what you've provided is more straightforward, but I'd
still like to look at this deeper and provide review. The comments
asked here are probably my most significant.