From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CCD3843335; Wed, 15 Nov 2023 08:47:07 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9488B40261; Wed, 15 Nov 2023 08:47:07 +0100 (CET) Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mails.dpdk.org (Postfix) with ESMTP id 92C7E400EF for ; Wed, 15 Nov 2023 08:47:05 +0100 (CET) Received: by mail-ej1-f44.google.com with SMTP id a640c23a62f3a-9dbb3e0ff65so915631266b.1 for ; Tue, 14 Nov 2023 23:47:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700034425; x=1700639225; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=UKP74pf5lzjih0RpmIMrQg+yeOR7kbyQzo4nnANCGJY=; b=Xn6S+q2H/8kYMOI5jmu1hQcxBpBtPY/g1Z8C00XrDkFcbgV800WkDSlmshPvedvpyk PF6opUg3F3Hy5zPVe66IbYU2xZqrxQJMEtWKP/0DAQ6tkfbr5RFXb3AQDcimuh+iQgB/ HYWbQ9xZqHeUWe/QmKmKdjpJkbHh2DIZdlc5d8qocZ9MNEtKCkmXyZZdw3krcFB95oLl YdKEiEYm+TIvALjWQheR19T9MqjH6Lc0jpMr5h0807ffn1yn/DeWRl01KQMj3onfdxUr V2J8KWxIFwJir5w+kK/SnvfsXTHO8Cmv/U6iD6H7GvFLYucYjWzrSSFFAdNawbipI5Ub lQ5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700034425; x=1700639225; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=UKP74pf5lzjih0RpmIMrQg+yeOR7kbyQzo4nnANCGJY=; b=CB6w+ydJ1AN43WQpN4EGTVQmv65e5e7PJ5cnokJD2pp1Zfq42ZQbztBH337Xm9yx73 Val66q5GhvuNQTu8E4kpCBkEW/l2jcQIh19/LaLuinOeOmkA6yFPOywmQb+upd7Hni+V b9SlDeHLvqBv+qRyKvehWXixAsbAzOBZds5V9m3ftzC3sk5hFTj9+MgsmidYpalBjNoW BzL1Xw0J89U4eysSyIrLGgo4OjmbXnv2+tIW77vO/vDpahSZMbdvdTfFbSYN1xh9i6Nq IW53wa8gJcflCXwfcg1wWJxm/FV321AygbL8Pv81Gb+aPBvZtDopQNfsCSpVf4CCh7Rt r+mQ== X-Gm-Message-State: AOJu0YwX8mORKVx47qOgRNk9qcXe7zY37k7GTZqn5/fWXX8g5+5osHBS cwb0/3Rebl8rlCMG5pWWFDujx/RnXMKsqmwEWCiS+ig2oJ4a+YVzUHkCwA== X-Google-Smtp-Source: AGHT+IEHcMtpG64S6ORsej4lv/lNX6PdO8ySP7c1liMVCrGbLui0T6XCAuH4Up2h3ZTwtI8cZWGBkP7sl9BHZ1bU6+k= X-Received: by 2002:a17:906:c450:b0:9be:562:a44a with SMTP id ck16-20020a170906c45000b009be0562a44amr9511218ejb.23.1700034424933; Tue, 14 Nov 2023 23:47:04 -0800 (PST) MIME-Version: 1.0 References: <20230831100407.59865-1-juraj.linkes@pantheon.tech> <20231106171601.160749-1-juraj.linkes@pantheon.tech> <20231106171601.160749-2-juraj.linkes@pantheon.tech> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 15 Nov 2023 08:46:54 +0100 Message-ID: Subject: Re: [PATCH v5 01/23] dts: code adjustments for doc generation To: Yoan Picchi Cc: Thomas Monjalon , Honnappa Nagarahalli , Bruce Richardson , Jeremy Spewock , Patrick Robb , Paul Szczepanek , dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Nov 8, 2023 at 2:35=E2=80=AFPM Yoan Picchi wrote: > > On 11/6/23 17:15, Juraj Linke=C5=A1 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__ =3D=3D '__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=C5=A1 > > --- > > 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 =3D> }/interactive_shell.py | 0 > > .../{remote =3D> }/python_shell.py | 0 > > .../remote_session/remote/__init__.py | 27 ------ > > .../{remote =3D> }/remote_session.py | 0 > > .../{remote =3D> }/ssh_session.py | 12 +-- > > .../{remote =3D> }/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 =3D> }/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 =3D> }/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 +- > > .../{ =3D> traffic_generator}/scapy.py | 23 ++--- > > .../traffic_generator.py | 16 +++- > > .../testbed_model/{hw =3D> }/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 =3D> }/interactive_remote= _session.py (100%) > > rename dts/framework/remote_session/{remote =3D> }/interactive_shell.= py (100%) > > rename dts/framework/remote_session/{remote =3D> }/python_shell.py (1= 00%) > > delete mode 100644 dts/framework/remote_session/remote/__init__.py > > rename dts/framework/remote_session/{remote =3D> }/remote_session.py = (100%) > > rename dts/framework/remote_session/{remote =3D> }/ssh_session.py (91= %) > > rename dts/framework/remote_session/{remote =3D> }/testpmd_shell.py (= 100%) > > rename dts/framework/testbed_model/{hw =3D> }/cpu.py (95%) > > delete mode 100644 dts/framework/testbed_model/hw/__init__.py > > rename dts/framework/{remote_session =3D> testbed_model}/linux_sessio= n.py (97%) > > rename dts/framework/{remote_session =3D> testbed_model}/os_session.p= y (95%) > > rename dts/framework/testbed_model/{hw =3D> }/port.py (100%) > > rename dts/framework/{remote_session =3D> testbed_model}/posix_sessio= n.py (98%) > > create mode 100644 dts/framework/testbed_model/traffic_generator/__in= it__.py > > rename dts/framework/testbed_model/{ =3D> traffic_generator}/capturin= g_traffic_generator.py (96%) > > rename dts/framework/testbed_model/{ =3D> traffic_generator}/scapy.py= (95%) > > rename dts/framework/testbed_model/{ =3D> traffic_generator}/traffic_= generator.py (80%) > > rename dts/framework/testbed_model/{hw =3D> }/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=3DTrafficGeneratorType.SCA= PY > > ) > > + case _: > > + raise ConfigurationError( > > + f'Unknown traffic generator type "{d["type"]}".' > > + ) > > > > > > @dataclass(slots=3DTrue, frozen=3DTrue) > > --- a/dts/framework/settings.py > > +++ b/dts/framework/settings.py > > @@ -162,23 +176,22 @@ def _get_parser() -> argparse.ArgumentParser: > > return parser > > > > > > -def _get_settings() -> _Settings: > > +def get_settings() -> Settings: > > parsed_args =3D _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__ =3D=3D "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=3Dparsed_args.config_file, > > output_dir=3Dparsed_args.output_dir, > > timeout=3Dparsed_args.timeout, > > - verbose=3D(parsed_args.verbose =3D=3D "Y"), > > - skip_setup=3D(parsed_args.skip_setup =3D=3D "Y"), > > + verbose=3Dparsed_args.verbose, > > + skip_setup=3Dparsed_args.skip_setup, > > dpdk_tarball_path=3DPath( > > - 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.outpu= t_dir)) > > + if not os.path.exists(parsed_args.tarball) > > + else Path(parsed_args.tarball) > > + ), > > compile_timeout=3Dparsed_args.compile_timeout, > > - test_cases=3Dparsed_args.test_cases.split(",") if parsed_args.= test_cases else [], > > + test_cases=3D( > > + parsed_args.test_cases.split(",") if parsed_args.test_case= s else [] > > + ), > > re_run=3Dparsed_args.re_run, > > ) > > - > > - > > -SETTINGS: _Settings =3D _get_settings() > > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbe= d_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 =3D [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 =3D 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 supp= ly different > > amounts of memory for hugepages and numa-based hugepage alloc= ation may need > > @@ -249,3 +253,13 @@ def skip_setup(func: Callable[..., Any]) -> Callab= le[..., 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, NodeInf= o > > 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 =3D TypeVar("InteractiveShellType", bound=3DInte= ractiveShell) > > > > @@ -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 > > 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 =3D "/[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 =3D=3D 3 and sys.version_info.minor < 1= 0 > > - ): > > - print( > > - RED( > > - ( > > - "WARNING: DTS execution node's python version is l= ower than" > > - "python 3.10, is deprecated and will not work in f= uture releases." > > - ) > > - ), > > - file=3Dsys.stderr, > > - ) > > - print(RED("Please use Python >=3D 3.10 instead"), file=3Dsys.s= tderr) > > +REGEX_FOR_PCI_ADDRESS: str =3D "/[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-f= A-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) =3D=3D 1: > > packet_summaries =3D 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 =3D 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() > > > > >