Re: [PATCH v5 01/23] dts: code adjustments for doc generation

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Wed, Nov 8, 2023 at 2:35 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>
> On 11/6/23 17:15, Juraj Linkeš wrote:
> > The standard Python tool for generating API documentation, Sphinx,
> > imports modules one-by-one when generating the documentation. This
> > requires code changes:
> > * properly guarding argument parsing in the if __name__ == '__main__'
> >    block,
> > * the logger used by DTS runner underwent the same treatment so that it
> >    doesn't create log files outside of a DTS run,
> > * however, DTS uses the arguments to construct an object holding global
> >    variables. The defaults for the global variables needed to be moved
> >    from argument parsing elsewhere,
> > * importing the remote_session module from framework resulted in
> >    circular imports because of one module trying to import another
> >    module. This is fixed by reorganizing the code,
> > * some code reorganization was done because the resulting structure
> >    makes more sense, improving documentation clarity.
> >
> > The are some other changes which are documentation related:
> > * added missing type annotation so they appear in the generated docs,
> > * reordered arguments in some methods,
> > * removed superfluous arguments and attributes,
> > * change private functions/methods/attributes to private and vice-versa.
> >
> > The above all appear in the generated documentation and the with them,
> > the documentation is improved.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> >   dts/framework/config/__init__.py              | 10 ++-
> >   dts/framework/dts.py                          | 33 +++++--
> >   dts/framework/exception.py                    | 54 +++++-------
> >   dts/framework/remote_session/__init__.py      | 41 ++++-----
> >   .../interactive_remote_session.py             |  0
> >   .../{remote => }/interactive_shell.py         |  0
> >   .../{remote => }/python_shell.py              |  0
> >   .../remote_session/remote/__init__.py         | 27 ------
> >   .../{remote => }/remote_session.py            |  0
> >   .../{remote => }/ssh_session.py               | 12 +--
> >   .../{remote => }/testpmd_shell.py             |  0
> >   dts/framework/settings.py                     | 87 +++++++++++--------
> >   dts/framework/test_result.py                  |  4 +-
> >   dts/framework/test_suite.py                   |  7 +-
> >   dts/framework/testbed_model/__init__.py       | 12 +--
> >   dts/framework/testbed_model/{hw => }/cpu.py   | 13 +++
> >   dts/framework/testbed_model/hw/__init__.py    | 27 ------
> >   .../linux_session.py                          |  6 +-
> >   dts/framework/testbed_model/node.py           | 26 ++++--
> >   .../os_session.py                             | 22 ++---
> >   dts/framework/testbed_model/{hw => }/port.py  |  0
> >   .../posix_session.py                          |  4 +-
> >   dts/framework/testbed_model/sut_node.py       |  8 +-
> >   dts/framework/testbed_model/tg_node.py        | 30 +------
> >   .../traffic_generator/__init__.py             | 24 +++++
> >   .../capturing_traffic_generator.py            |  6 +-
> >   .../{ => traffic_generator}/scapy.py          | 23 ++---
> >   .../traffic_generator.py                      | 16 +++-
> >   .../testbed_model/{hw => }/virtual_device.py  |  0
> >   dts/framework/utils.py                        | 46 +++-------
> >   dts/main.py                                   |  9 +-
> >   31 files changed, 259 insertions(+), 288 deletions(-)
> >   rename dts/framework/remote_session/{remote => }/interactive_remote_session.py (100%)
> >   rename dts/framework/remote_session/{remote => }/interactive_shell.py (100%)
> >   rename dts/framework/remote_session/{remote => }/python_shell.py (100%)
> >   delete mode 100644 dts/framework/remote_session/remote/__init__.py
> >   rename dts/framework/remote_session/{remote => }/remote_session.py (100%)
> >   rename dts/framework/remote_session/{remote => }/ssh_session.py (91%)
> >   rename dts/framework/remote_session/{remote => }/testpmd_shell.py (100%)
> >   rename dts/framework/testbed_model/{hw => }/cpu.py (95%)
> >   delete mode 100644 dts/framework/testbed_model/hw/__init__.py
> >   rename dts/framework/{remote_session => testbed_model}/linux_session.py (97%)
> >   rename dts/framework/{remote_session => testbed_model}/os_session.py (95%)
> >   rename dts/framework/testbed_model/{hw => }/port.py (100%)
> >   rename dts/framework/{remote_session => testbed_model}/posix_session.py (98%)
> >   create mode 100644 dts/framework/testbed_model/traffic_generator/__init__.py
> >   rename dts/framework/testbed_model/{ => traffic_generator}/capturing_traffic_generator.py (96%)
> >   rename dts/framework/testbed_model/{ => traffic_generator}/scapy.py (95%)
> >   rename dts/framework/testbed_model/{ => traffic_generator}/traffic_generator.py (80%)
> >   rename dts/framework/testbed_model/{hw => }/virtual_device.py (100%)
> >
> > diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__init__.py
> > index cb7e00ba34..2044c82611 100644
> > --- a/dts/framework/config/__init__.py
> > +++ b/dts/framework/config/__init__.py
> > @@ -17,6 +17,7 @@
> >   import warlock  # type: ignore[import]
> >   import yaml
> >
> > +from framework.exception import ConfigurationError
> >   from framework.settings import SETTINGS
> >   from framework.utils import StrEnum
> >
> > @@ -89,7 +90,7 @@ class TrafficGeneratorConfig:
> >       traffic_generator_type: TrafficGeneratorType
> >
> >       @staticmethod
> > -    def from_dict(d: dict):
> > +    def from_dict(d: dict) -> "ScapyTrafficGeneratorConfig":
>
> This function looks to be designed to support more trafic generator than
> just scapy, so setting its return type to scapy specifically looks
> wrong. Shouldn't it be a more generic traffic generator type? Like you
> did in create_traffic_generator()
>

The reason is the type in the constructor of the scapy traffic
generator - the type there should be ScapyTrafficGeneratorConfig and
if I change it anywhere in the chain, mypy reports an error. I don't
want to do any extra refactoring in this patch if we don't have to, so
we need to rethink this when adding a new traffic generator.

> >           # This looks useless now, but is designed to allow expansion to traffic
> >           # generators that require more configuration later.
> >           match TrafficGeneratorType(d["type"]):
> > @@ -97,6 +98,10 @@ def from_dict(d: dict):
> >                   return ScapyTrafficGeneratorConfig(
> >                       traffic_generator_type=TrafficGeneratorType.SCAPY
> >                   )
> > +            case _:
> > +                raise ConfigurationError(
> > +                    f'Unknown traffic generator type "{d["type"]}".'
> > +                )
> >
> >
> >   @dataclass(slots=True, frozen=True)

<snip>

> > --- a/dts/framework/settings.py
> > +++ b/dts/framework/settings.py
<small snip>
> > @@ -162,23 +176,22 @@ def _get_parser() -> argparse.ArgumentParser:
> >       return parser
> >
> >
> > -def _get_settings() -> _Settings:
> > +def get_settings() -> Settings:
> >       parsed_args = _get_parser().parse_args()
> > -    return _Settings(
> > +    return Settings(
>
> That means we're parsing and creating a new setting object everytime
> we're trying to read the setting? Shouldn't we just save it and return a
> copy? That seems to be the old behavior, any reason to change it?
>

By old behavior, do you mean the behavior from the previous version?

I want the Settings object to be immutable, as much as it can be in
Python (that's why the dataclass if frozen), so that it's clear it
shouldn't be changed during runtime, as the object represents user
choices (any modifications would violate that). More below.

> Related to this, this do mean that the previously created setting
> variable is only used to set up the parser, so it might need to be
> renamed to default_setting if it doesnt get reused.
>

It is used. The reason the SETTINGS variable is implemented this way
is mostly because of Sphinx. Sphinx imports everything file by file:
When it imports a module that uses the SETTINGS variable (such as
node.py), the variable needs to be defined. On top of that, when
Sphinx accesses command line arguments, it sees it's own command line
arguments (which are incompatible with DTS), so we need to guard the
command line parsing against imports (we have it in if __name__ ==
"main" in main.py). This is why the defaults are split from the
command line parsing - when Sphinx imports the module, it uses the
object with defaults and during runtime we replace the object with
user-defined values.

There are other ways to do this, but I didn't find a better one with
all the constraints and requirements outlined above.

> >           config_file_path=parsed_args.config_file,
> >           output_dir=parsed_args.output_dir,
> >           timeout=parsed_args.timeout,
> > -        verbose=(parsed_args.verbose == "Y"),
> > -        skip_setup=(parsed_args.skip_setup == "Y"),
> > +        verbose=parsed_args.verbose,
> > +        skip_setup=parsed_args.skip_setup,
> >           dpdk_tarball_path=Path(
> > -            DPDKGitTarball(parsed_args.tarball, parsed_args.output_dir)
> > -        )
> > -        if not os.path.exists(parsed_args.tarball)
> > -        else Path(parsed_args.tarball),
> > +            Path(DPDKGitTarball(parsed_args.tarball, parsed_args.output_dir))
> > +            if not os.path.exists(parsed_args.tarball)
> > +            else Path(parsed_args.tarball)
> > +        ),
> >           compile_timeout=parsed_args.compile_timeout,
> > -        test_cases=parsed_args.test_cases.split(",") if parsed_args.test_cases else [],
> > +        test_cases=(
> > +            parsed_args.test_cases.split(",") if parsed_args.test_cases else []
> > +        ),
> >           re_run=parsed_args.re_run,
> >       )
> > -
> > -
> > -SETTINGS: _Settings = _get_settings()

<snip>

> > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> > index fc01e0bf8e..7571e7b98d 100644
> > --- a/dts/framework/testbed_model/node.py
> > +++ b/dts/framework/testbed_model/node.py
> > @@ -12,23 +12,26 @@
> >   from typing import Any, Callable, Type, Union
> >
> >   from framework.config import (
> > +    OS,
> >       BuildTargetConfiguration,
> >       ExecutionConfiguration,
> >       NodeConfiguration,
> >   )
> > +from framework.exception import ConfigurationError
> >   from framework.logger import DTSLOG, getLogger
> > -from framework.remote_session import InteractiveShellType, OSSession, create_session
> >   from framework.settings import SETTINGS
> >
> > -from .hw import (
> > +from .cpu import (
> >       LogicalCore,
> >       LogicalCoreCount,
> >       LogicalCoreList,
> >       LogicalCoreListFilter,
> > -    VirtualDevice,
> >       lcore_filter,
> >   )
> > -from .hw.port import Port
> > +from .linux_session import LinuxSession
> > +from .os_session import InteractiveShellType, OSSession
> > +from .port import Port
> > +from .virtual_device import VirtualDevice
> >
> >
> >   class Node(ABC):
> > @@ -69,6 +72,7 @@ def __init__(self, node_config: NodeConfiguration):
> >       def _init_ports(self) -> None:
> >           self.ports = [Port(self.name, port_config) for port_config in self.config.ports]
> >           self.main_session.update_ports(self.ports)
> > +
>
> Is the newline intended?
>

Hm, I don't really remember or see a reason for it, really. I can remove it.

> >           for port in self.ports:
> >               self.configure_port_state(port)
> >
> > @@ -172,9 +176,9 @@ def create_interactive_shell(
> >
> >           return self.main_session.create_interactive_shell(
> >               shell_cls,
> > -            app_args,
> >               timeout,
> >               privileged,
> > +            app_args,
> >           )
> >
> >       def filter_lcores(
> > @@ -205,7 +209,7 @@ def _get_remote_cpus(self) -> None:
> >           self._logger.info("Getting CPU information.")
> >           self.lcores = self.main_session.get_remote_cpus(self.config.use_first_core)
> >
> > -    def _setup_hugepages(self):
> > +    def _setup_hugepages(self) -> None:
> >           """
> >           Setup hugepages on the Node. Different architectures can supply different
> >           amounts of memory for hugepages and numa-based hugepage allocation may need
> > @@ -249,3 +253,13 @@ def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]:
> >               return lambda *args: None
> >           else:
> >               return func
> > +
> > +
> > +def create_session(
> > +    node_config: NodeConfiguration, name: str, logger: DTSLOG
> > +) -> OSSession:
> > +    match node_config.os:
> > +        case OS.linux:
> > +            return LinuxSession(node_config, name, logger)
> > +        case _:
> > +            raise ConfigurationError(f"Unsupported OS {node_config.os}")
> > diff --git a/dts/framework/remote_session/os_session.py b/dts/framework/testbed_model/os_session.py
> > similarity index 95%
> > rename from dts/framework/remote_session/os_session.py
> > rename to dts/framework/testbed_model/os_session.py
> > index 8a709eac1c..76e595a518 100644
> > --- a/dts/framework/remote_session/os_session.py
> > +++ b/dts/framework/testbed_model/os_session.py
> > @@ -10,19 +10,19 @@
> >
> >   from framework.config import Architecture, NodeConfiguration, NodeInfo
> >   from framework.logger import DTSLOG
> > -from framework.remote_session.remote import InteractiveShell
> > -from framework.settings import SETTINGS
> > -from framework.testbed_model import LogicalCore
> > -from framework.testbed_model.hw.port import Port
> > -from framework.utils import MesonArgs
> > -
> > -from .remote import (
> > +from framework.remote_session import (
> >       CommandResult,
> >       InteractiveRemoteSession,
> > +    InteractiveShell,
> >       RemoteSession,
> >       create_interactive_session,
> >       create_remote_session,
> >   )
> > +from framework.settings import SETTINGS
> > +from framework.utils import MesonArgs
> > +
> > +from .cpu import LogicalCore
> > +from .port import Port
> >
> >   InteractiveShellType = TypeVar("InteractiveShellType", bound=InteractiveShell)
> >
> > @@ -85,9 +85,9 @@ def send_command(
> >       def create_interactive_shell(
> >           self,
> >           shell_cls: Type[InteractiveShellType],
> > -        eal_parameters: str,
> >           timeout: float,
> >           privileged: bool,
> > +        app_args: str,
>
> Is there a reason why the argument position got changed? I'd guess
> because it's more idomatic to have the extra arg at the end, but I just
> want to make sure it's intended.
>

Yes, this is very much intended. It's here to unite the method
signature with the signatures of the rest of the methods called down
the line.
I made this API change during API documentation as the different
signatures of basically the same methods would look terrible in the
docs.

> >       ) -> InteractiveShellType:
> >           """
> >           See "create_interactive_shell" in SutNode

<snip>

> > diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> > index d27c2c5b5f..f0c916471c 100644
> > --- a/dts/framework/utils.py
> > +++ b/dts/framework/utils.py
> > @@ -7,7 +7,6 @@
> >   import json
> >   import os
> >   import subprocess
> > -import sys
> >   from enum import Enum
> >   from pathlib import Path
> >   from subprocess import SubprocessError
> > @@ -16,35 +15,7 @@
> >
> >   from .exception import ConfigurationError
> >
> > -
> > -class StrEnum(Enum):
> > -    @staticmethod
> > -    def _generate_next_value_(
> > -        name: str, start: int, count: int, last_values: object
> > -    ) -> str:
> > -        return name
> > -
> > -    def __str__(self) -> str:
> > -        return self.name
> > -
> > -
> > -REGEX_FOR_PCI_ADDRESS = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
> > -
> > -
> > -def check_dts_python_version() -> None:
> > -    if sys.version_info.major < 3 or (
> > -        sys.version_info.major == 3 and sys.version_info.minor < 10
> > -    ):
> > -        print(
> > -            RED(
> > -                (
> > -                    "WARNING: DTS execution node's python version is lower than"
> > -                    "python 3.10, is deprecated and will not work in future releases."
> > -                )
> > -            ),
> > -            file=sys.stderr,
> > -        )
> > -        print(RED("Please use Python >= 3.10 instead"), file=sys.stderr)
> > +REGEX_FOR_PCI_ADDRESS: str = "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}.[0-9]{1}/"
> >
> >
> >   def expand_range(range_str: str) -> list[int]:
> > @@ -67,7 +38,7 @@ def expand_range(range_str: str) -> list[int]:
> >       return expanded_range
> >
> >
> > -def get_packet_summaries(packets: list[Packet]):
> > +def get_packet_summaries(packets: list[Packet]) -> str:
> >       if len(packets) == 1:
> >           packet_summaries = packets[0].summary()
> >       else:
> > @@ -77,8 +48,15 @@ def get_packet_summaries(packets: list[Packet]):
> >       return f"Packet contents: \n{packet_summaries}"
> >
> >
> > -def RED(text: str) -> str:
> > -    return f"\u001B[31;1m{str(text)}\u001B[0m"
> > +class StrEnum(Enum):
> > +    @staticmethod
> > +    def _generate_next_value_(
> > +        name: str, start: int, count: int, last_values: object
> > +    ) -> str:
> > +        return name
>
> I don't understand this function? I don't see it used anywhere. And the
> parameters are unused?
>

This is an internal method of Enum that defines what happens when
auto() is called (which is used plenty).

> > +
> > +    def __str__(self) -> str:
> > +        return self.name
> >
> >
> >   class MesonArgs(object):
> > @@ -225,5 +203,5 @@ def _delete_tarball(self) -> None:
> >           if self._tarball_path and os.path.exists(self._tarball_path):
> >               os.remove(self._tarball_path)
> >
> > -    def __fspath__(self):
> > +    def __fspath__(self) -> str:
> >           return str(self._tarball_path)
> > diff --git a/dts/main.py b/dts/main.py
> > index 43311fa847..5d4714b0c3 100755
> > --- a/dts/main.py
> > +++ b/dts/main.py
> > @@ -10,10 +10,17 @@
> >
> >   import logging
> >
> > -from framework import dts
> > +from framework import settings
> >
> >
> >   def main() -> None:
> > +    """Set DTS settings, then run DTS.
> > +
> > +    The DTS settings are taken from the command line arguments and the environment variables.
> > +    """
> > +    settings.SETTINGS = settings.get_settings()
> > +    from framework import dts
>
> Why the import *inside* the main ?
>

This is actually explained in the docstring added in one of the later
patches, so let me copy paste it here:

The DTS settings are taken from the command line arguments and the
environment variables.
The settings object is stored in the module-level variable
settings.SETTINGS which the entire
framework uses. After importing the module (or the variable), any
changes to the variable are
not going to be reflected without a re-import. This means that the
SETTINGS variable must
be modified before the settings module is imported anywhere else in
the framework.

> > +
> >       dts.run_all()
> >
> >
>