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 EE03E4326D; Thu, 2 Nov 2023 11:17:46 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B9D6B40282; Thu, 2 Nov 2023 11:17:46 +0100 (CET) Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com [209.85.208.181]) by mails.dpdk.org (Postfix) with ESMTP id BF9E840262 for ; Thu, 2 Nov 2023 11:17:44 +0100 (CET) Received: by mail-lj1-f181.google.com with SMTP id 38308e7fff4ca-2c50fbc218bso9303331fa.3 for ; Thu, 02 Nov 2023 03:17:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1698920264; x=1699525064; 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=u6l1AGaZRnmPxqCGlU9++QWw4oPSrPic5cAc3m373rI=; b=IHc9/pjfehaL4TMjIHRJrDdyQRWG2UoSDd0sWeG48Eiz3iByDV5owCGQvx1ttVjt4f F5C20EM9wvd6DXpR1I+M5yoPbQjuROx8DWR9+lK1oZd5BBF7weV96gn+jtXadDkhkzIW 7eyPG0FDbb7JwLngx5dmqUfweCIhGH33lJe5WNj01kSqiBz6oBucEIcGvNWcXTppOEjL e2qG6/CGJAMYCdcRKzzuPn0nkvDzlGwHs363bYTcLZ+UpsTDy+Y/ifWkxquYGBeqES9l RDfATHbqQBbn8TRkPmaNgAN2FBa7oNxa8o97v5CgpWTKrJ8bdhFmEymHag++/zfccNoj 2x9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698920264; x=1699525064; 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=u6l1AGaZRnmPxqCGlU9++QWw4oPSrPic5cAc3m373rI=; b=K1XkBE4gIkcNJmaFKUJh+HXCjty1UwwliDqJgKASzgitr3LxiN0f7HD78NCVHw/M8F lkrXqgJkcBJt6M9BUDgMvnXtG23wQNnmSZKjwVgro2bSw15XtnnO7trHebt6UJbbq2jU 8Cv+7uBpSMBVNAPvFHtnNxq7oXqPcByfqpOnSZvQe+wLR7HpMWT9FKly6Ld2p9ZF2w9o Gh5OfrVN7GrflWLlV3DInR3nZlvdu4H/9aOS1E7hEHVzChmbHjLrvzjKu3sr9Wk4+jUJ 9LiJKMPhWVd1FnmXdLgaBpdJ6cLLdKpn5VePxHLBLdeElXXKmWqusWKKsW5dzBqay14T PK0g== X-Gm-Message-State: AOJu0Yw6fktAFj3FDQciajpqLZDMkNasM/lZgwfsxsolSqpdKlFK0/vV XhO9HdAUNoZxe1M8MMnfHPMXxIwNyfBMEVxcYEWtkw== X-Google-Smtp-Source: AGHT+IFlGqNxdPAfW5jMa5YcX8Ab+NE4LV3ZYfRLxS7NNFaJta2Yo0vbRS3gUIfVwNjlwpNwYbVkpT9rOLfmKqiOPPY= X-Received: by 2002:a2e:b254:0:b0:2bf:fa62:5ceb with SMTP id n20-20020a2eb254000000b002bffa625cebmr12660186ljm.11.1698920264035; Thu, 02 Nov 2023 03:17:44 -0700 (PDT) MIME-Version: 1.0 References: <20230511091408.236638-1-juraj.linkes@pantheon.tech> <20230831100407.59865-1-juraj.linkes@pantheon.tech> <20230831100407.59865-5-juraj.linkes@pantheon.tech> <2f79eb4a-eda0-43a5-a7f6-1c7af84a47e6@foss.arm.com> In-Reply-To: <2f79eb4a-eda0-43a5-a7f6-1c7af84a47e6@foss.arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Thu, 2 Nov 2023 11:17:33 +0100 Message-ID: Subject: Re: [RFC PATCH v4 4/4] dts: format docstrigs to google format To: Yoan Picchi Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, lijuan.tu@intel.com, bruce.richardson@intel.com, jspewock@iol.unh.edu, probb@iol.unh.edu, 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 Tue, Oct 31, 2023 at 1:10=E2=80=AFPM Yoan Picchi wrote: > > On 8/31/23 11:04, Juraj Linke=C5=A1 wrote: > > WIP: only one module is reformatted to serve as a demonstration. > > > > The google format is documented here [0]. > > > > [0]: https://google.github.io/styleguide/pyguide.html > > > > Signed-off-by: Juraj Linke=C5=A1 > > Acked-by: Jeremy Spweock > > --- > > dts/framework/testbed_model/node.py | 171 +++++++++++++++++++--------= - > > 1 file changed, 118 insertions(+), 53 deletions(-) > > > > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbe= d_model/node.py > > index 23efa79c50..619743ebe7 100644 > > --- a/dts/framework/testbed_model/node.py > > +++ b/dts/framework/testbed_model/node.py > > @@ -3,8 +3,13 @@ > > # Copyright(c) 2022-2023 PANTHEON.tech s.r.o. > > # Copyright(c) 2022-2023 University of New Hampshire > > > > -""" > > -A node is a generic host that DTS connects to and manages. > > +"""Common functionality for node management. > > + > > +There's a base class, Node, that's supposed to be extended by other cl= asses > > This comment and all the following ones are all something of a nitpick. I think they're pretty helpful. > This sounds too passive. Why not having something simpler like: > The virtual class Node is meant to be extended by other classes > with functionality specific to that node type. > Okay, makes sense. > > +with functionality specific to that node type. > > +The only part that can be used standalone is the Node.skip_setup stati= c method, > > +which is a decorator used to skip method execution if skip_setup is pa= ssed > > +by the user on the cmdline or in an env variable. > > I'd extend env to the full word as this is meant to go in the documentati= on. > I'll try to do this everywhere in docs. > > """ > > > > from abc import ABC > > @@ -35,10 +40,26 @@ > > > > > > class Node(ABC): > > - """ > > - Basic class for node management. This class implements methods tha= t > > - manage a node, such as information gathering (of CPU/PCI/NIC) and > > - environment setup. > > + """The base class for node management. > > + > > + It shouldn't be instantiated, but rather extended. > > + It implements common methods to manage any node: > > + > > + * connection to the node > > + * information gathering of CPU > > + * hugepages setup > > + > > + Arguments: > > + node_config: The config from the input configuration file. > > + > > + Attributes: > > + main_session: The primary OS-agnostic remote session used > > + to communicate with the node. > > + config: The configuration used to create the node. > > + name: The name of the node. > > + lcores: The list of logical cores that DTS can use on the node= . > > + It's derived from logical cores present on the node and us= er configuration. > > + ports: The ports of this node specified in user configuration. > > """ > > > > main_session: OSSession > > @@ -77,9 +98,14 @@ def _init_ports(self) -> None: > > self.configure_port_state(port) > > > > def set_up_execution(self, execution_config: ExecutionConfigurati= on) -> None: > > - """ > > - Perform the execution setup that will be done for each executi= on > > - this node is part of. > > + """Execution setup steps. > > + > > + Configure hugepages and call self._set_up_execution where > > + the rest of the configuration steps (if any) are implemented. > > + > > + Args: > > + execution_config: The execution configuration according to= which > > + the setup steps will be taken. > > """ > > self._setup_hugepages() > > self._set_up_execution(execution_config) > > @@ -88,58 +114,78 @@ def set_up_execution(self, execution_config: Execu= tionConfiguration) -> None: > > self.virtual_devices.append(VirtualDevice(vdev)) > > > > def _set_up_execution(self, execution_config: ExecutionConfigurat= ion) -> None: > > - """ > > - This method exists to be optionally overwritten by derived cla= sses and > > - is not decorated so that the derived class doesn't have to use= the decorator. > > + """Optional additional execution setup steps for derived class= es. > > + > > + Derived classes should overwrite this > > + if they want to add additional execution setup steps. > > I'd probably use need or require instead of want (it's the dev that > wants, not the class) > Good point, that's a better wording. > > """ > > > > def tear_down_execution(self) -> None: > > - """ > > - Perform the execution teardown that will be done after each ex= ecution > > - this node is part of concludes. > > + """Execution teardown steps. > > + > > + There are currently no common execution teardown steps > > + common to all DTS node types. > > """ > > self.virtual_devices =3D [] > > self._tear_down_execution() > > > > def _tear_down_execution(self) -> None: > > - """ > > - This method exists to be optionally overwritten by derived cla= sses and > > - is not decorated so that the derived class doesn't have to use= the decorator. > > + """Optional additional execution teardown steps for derived cl= asses. > > + > > + Derived classes should overwrite this > > + if they want to add additional execution teardown steps. > > """ > > > > def set_up_build_target( > > self, build_target_config: BuildTargetConfiguration > > ) -> None: > > - """ > > - Perform the build target setup that will be done for each buil= d target > > - tested on this node. > > + """Build target setup steps. > > + > > + There are currently no common build target setup steps > > + common to all DTS node types. > > + > > + Args: > > + build_target_config: The build target configuration accord= ing to which > > + the setup steps will be taken. > > """ > > self._set_up_build_target(build_target_config) > > > > def _set_up_build_target( > > self, build_target_config: BuildTargetConfiguration > > ) -> None: > > - """ > > - This method exists to be optionally overwritten by derived cla= sses and > > - is not decorated so that the derived class doesn't have to use= the decorator. > > + """Optional additional build target setup steps for derived cl= asses. > > + > > + Derived classes should optionally overwrite this > > Is there a reason for the "optionally" here when it's absent in all the > other functions? > I've actually caught this as well when addressing the previous comment. I unified the wording (without optionally, it's reduntant). > > + if they want to add additional build target setup steps. > > """ > > > > def tear_down_build_target(self) -> None: > > - """ > > - Perform the build target teardown that will be done after each= build target > > - tested on this node. > > + """Build target teardown steps. > > + > > + There are currently no common build target teardown steps > > + common to all DTS node types. > > """ > > self._tear_down_build_target() > > > > def _tear_down_build_target(self) -> None: > > - """ > > - This method exists to be optionally overwritten by derived cla= sses and > > - is not decorated so that the derived class doesn't have to use= the decorator. > > + """Optional additional build target teardown steps for derived= classes. > > + > > + Derived classes should overwrite this > > + if they want to add additional build target teardown steps. > > """ > > > > def create_session(self, name: str) -> OSSession: > > - """ > > - Create and return a new OSSession tailored to the remote OS. > > + """Create and return a new OS-agnostic remote session. > > + > > + The returned session won't be used by the node creating it. > > + The session must be used by the caller. > > + Will be cleaned up automatically. > > I had a double take reading this before I understood that the subject > was the previously mentioned session. I'd recommend to either add "it" > or "the session". Also, it will be cleaned automatically... when? When I > create a new session? when the node is deleted? > The session will be cleaned up as part of the node's cleanup. What about this new wording: The returned session won't be used by the node creating it. The session must be used by the caller. The session will be maintained for the entire lifecycle of the node object, at the end of which the session will be cleaned up automatically. I've added a note: Note: Any number of these supplementary sessions may be created. > > + > > + Args: > > + name: The name of the session. > > + > > + Returns: > > + A new OS-agnostic remote session. > > """ > > session_name =3D f"{self.name} {name}" > > connection =3D create_session( > > @@ -186,14 +232,24 @@ def filter_lcores( > > filter_specifier: LogicalCoreCount | LogicalCoreList, > > ascending: bool =3D True, > > ) -> list[LogicalCore]: > > - """ > > - Filter the LogicalCores found on the Node according to > > - a LogicalCoreCount or a LogicalCoreList. > > + """Filter the node's logical cores that DTS can use. > > > > - If ascending is True, use cores with the lowest numerical id f= irst > > - and continue in ascending order. If False, start with the high= est > > - id and continue in descending order. This ordering affects whi= ch > > - sockets to consider first as well. > > + Logical cores that DTS can use are ones that are present on th= e node, > > + but filtered according to user config. > > + The filter_specifier will filter cores from those logical core= s. > > + > > + Args: > > + filter_specifier: Two different filters can be used, one t= hat specifies > > + the number of logical cores per core, cores per socket= and > > + the number of sockets, > > + the other that specifies a logical core list. > > + ascending: If True, use cores with the lowest numerical id= first > > + and continue in ascending order. If False, start with = the highest > > + id and continue in descending order. This ordering aff= ects which > > + sockets to consider first as well. > > + > > + Returns: > > + A list of logical cores. > > """ > > self._logger.debug(f"Filtering {filter_specifier} from {self.= lcores}.") > > return lcore_filter( > > @@ -203,17 +259,14 @@ def filter_lcores( > > ).filter() > > > > def _get_remote_cpus(self) -> None: > > - """ > > - Scan CPUs in the remote OS and store a list of LogicalCores. > > - """ > > + """Scan CPUs in the remote OS and store a list of LogicalCores= .""" > > self._logger.info("Getting CPU information.") > > self.lcores =3D self.main_session.get_remote_cpus(self.config= .use_first_core) > > > > def _setup_hugepages(self): > > - """ > > - Setup hugepages on the Node. Different architectures can suppl= y different > > - amounts of memory for hugepages and numa-based hugepage alloca= tion may need > > - to be considered. > > + """Setup hugepages on the Node. > > + > > + Configure the hugepages only if they're specified in user conf= iguration. > > """ > > if self.config.hugepages: > > self.main_session.setup_hugepages( > > @@ -221,8 +274,11 @@ def _setup_hugepages(self): > > ) > > > > def configure_port_state(self, port: Port, enable: bool =3D True)= -> None: > > - """ > > - Enable/disable port. > > + """Enable/disable port. > > + > > + Args: > > + port: The port to enable/disable. > > + enable: True to enable, false to disable. > > """ > > self.main_session.configure_port_state(port, enable) > > > > @@ -232,15 +288,19 @@ def configure_port_ip_address( > > port: Port, > > delete: bool =3D False, > > ) -> None: > > - """ > > - Configure the IP address of a port on this node. > > + """Add an IP address to a port on this node. > > + > > + Args: > > + address: The IP address with mask in CIDR format. > > + Can be either IPv4 or IPv6. > > + port: The port to which to add the address. > > + delete: If True, will delete the address from the port > > + instead of adding it. > > """ > > self.main_session.configure_port_ip_address(address, port, de= lete) > > > > def close(self) -> None: > > - """ > > - Close all connections and free other resources. > > - """ > > + """Close all connections and free other resources.""" > > if self.main_session: > > self.main_session.close() > > for session in self._other_sessions: > > @@ -249,6 +309,11 @@ def close(self) -> None: > > > > @staticmethod > > def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]: > > + """A decorator that skips the decorated function. > > + > > + When used, the decorator executes an empty lambda function > > + instead of the decorated function. > > + """ > > if SETTINGS.skip_setup: > > return lambda *args: None > > else: >