From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id ABFEB43E44;
	Thu, 11 Apr 2024 14:13:47 +0200 (CEST)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 3B8724029C;
	Thu, 11 Apr 2024 14:13:47 +0200 (CEST)
Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com
 [209.85.208.41]) by mails.dpdk.org (Postfix) with ESMTP id 0882340268
 for <dev@dpdk.org>; Thu, 11 Apr 2024 14:13:46 +0200 (CEST)
Received: by mail-ed1-f41.google.com with SMTP id
 4fb4d7f45d1cf-56e2b3e114fso8272100a12.2
 for <dev@dpdk.org>; Thu, 11 Apr 2024 05:13:45 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=pantheon.tech; s=google; t=1712837625; x=1713442425; 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=Hy94m6fXQw2unZTLRZUZcAvuWQ78Ai8Xan4eXedUnXk=;
 b=LUimgGuG8Ry8Y8Faai6bc2VRk9XbuhEiObaQB3fG12DfUc0QG6sNmkF0fwyILZfk0Z
 kFoswvHBvuGPjKD87JcocFG2MhFDSVRIbTlWEI7VDnnOye8eZe+RnyFOA6WxWgykED2e
 gcc+EgeZ5G8w6rzcteAnBb1bXoZfPXyPX7qMqLrPWrelxB+SMbuyYwvjRAjpKk6JRnhn
 GcgpOeI21t4gBFwHHj0ijmVkEeeX2r171DXxF1/Zs7g7E/VuF+Hyo5ZSyEOYcggCjjry
 MHH5BoEGm03TvDrNm/8zogzk4EBPjSOa4k1/SL+gui6WsSZaZn0oW31r3L6F6FNMWcnL
 ZMiA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20230601; t=1712837625; x=1713442425;
 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=Hy94m6fXQw2unZTLRZUZcAvuWQ78Ai8Xan4eXedUnXk=;
 b=YrkKzcXNcOoIwDG0aVR2xhB/KyJJikgjQdNqdngvHdyxsFP9RSn0FYwuxDltTM1jlY
 oLyLoQp9qioIRtzGxxzFWP+iyyXCEpVEbPOeiCo+JxzAwBVAJpO6b3twg+qJxBY+KDMg
 kdJdxfQlKCpob7XuFwkIIJLrhyV6IEsXyqoG00oDzqKTNPJm/QRSF/nuU6F8IBRnbx8l
 nPEN6+XZgCXLyAv5lpMkGLyT4HxHHhFT5bLDM5oY6c64AfP/5mK9a3JHTm+UYgI0GLIZ
 igCaqJwNyxrMDH5cbRm7NKrBE9WyuMC7iijghqg2qiy0x8m9MDVgXQIjz8nFo6Addmoq
 ODXw==
X-Forwarded-Encrypted: i=1;
 AJvYcCV4Tvk/AphNJoMmbcehaqAvoRiwdQXDbDs3iYQf/p0YrLsKLJXN6idihs/+nsWZzER8a/RPdAPmy5VB0AI=
X-Gm-Message-State: AOJu0YyufCxTVgX3fQBGz3G1sjmP20C2fj3cerQ1nU5p73hdzaiQbh9T
 9aEXCzf7coCRKskjNP324rv4fCweJnlSagbemR8vC5ceMXDdANMdp6Itjw5/u3gdOa29Q6kZCq9
 7mjNWqKNvQrbLiTVWsNTZqNpf5glZuHY2HUKTnA==
X-Google-Smtp-Source: AGHT+IF2XtjHBlb9/n3mMC/a1fEaqyeJvNCEGzohbgHc81emhkTfViSA1jB9FqDcDcnagwYt0jzSCPJFXaPa9leFvR8=
X-Received: by 2002:a17:907:1b25:b0:a52:15c3:b64e with SMTP id
 mp37-20020a1709071b2500b00a5215c3b64emr3009547ejc.10.1712837625491; Thu, 11
 Apr 2024 05:13:45 -0700 (PDT)
MIME-Version: 1.0
References: <20240326190422.577028-1-luca.vizzarro@arm.com>
 <20240326190422.577028-7-luca.vizzarro@arm.com>
 <CAAA20UTosheBk=XDUDkiVxXfwxYK1-McP7D6AAWuRoAmSPPwiA@mail.gmail.com>
 <CAOb5WZauxF8F=4O7WFDnXB4gPFUJ0ApnXnhAWgqGFsSBQu-HVA@mail.gmail.com>
 <1db1b2b8-fac0-41ad-9ba2-911365385a9b@arm.com>
 <CAOb5WZY5iH_9n_RTL+9AwsOj8o=yEbXLFm12NjYP-VA2BU2LtQ@mail.gmail.com>
 <5d35012d-0ffd-4c23-ad0c-8315453b8c9e@arm.com>
In-Reply-To: <5d35012d-0ffd-4c23-ad0c-8315453b8c9e@arm.com>
From: =?UTF-8?Q?Juraj_Linke=C5=A1?= <juraj.linkes@pantheon.tech>
Date: Thu, 11 Apr 2024 14:13:34 +0200
Message-ID: <CAOb5WZbjdDBg5rL-_JQKxt-J0BfUjEEhp9LtZTtEq3M+mzEhFw@mail.gmail.com>
Subject: Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
To: Luca Vizzarro <Luca.Vizzarro@arm.com>
Cc: Jeremy Spewock <jspewock@iol.unh.edu>, dev@dpdk.org, 
 Jack Bond-Preston <jack.bond-preston@arm.com>, 
 Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

On Thu, Apr 11, 2024 at 1:47=E2=80=AFPM Luca Vizzarro <Luca.Vizzarro@arm.co=
m> wrote:
>
> On 11/04/2024 11:30, Juraj Linke=C5=A1 wrote:
> > I've been thinking about these interactive shell constructors for some
> > time and I think the factory pattern is not well suitable for this.
> > Factories work well with classes with the same API (i.e.
> > implementations of abstract classes that don't add anything extra),
> > but are much less useful when dealing with classes with different
> > behaviors, such as the interactive shells. We see this here, different
> > apps are going to require different args and that alone kinda breaks
> > the factory pattern. I think we'll need to either ditch these
> > factories and instead just have methods that return the proper shell
> > (and the methods would only exist in classes where they belong, e.g.
> > testpmd only makes sense on an SUT). Or we could overload each factory
> > (the support has only been added in 3.11 with @typing.overload, but is
> > also available in typing_extensions, so we would be able to use it
> > with the extra dependency) where different signatures would return
> > different objects. In both cases the caller won't have to import the
> > class and the method signature is going to be clearer.
> >
> > We have this pattern with sut/tg nodes. I decided to move away from
> > the node factory because it didn't add much and in fact the code was
> > only clunkier. The interactive shell is not quite the same, as the
> > shells are not standalone in the same way the nodes are (the shells
> > are tied to nodes). Let me know what you think about all this - both
> > Luca and Jeremy.
>
> When writing this series, I went down the path of creating a
> `create_testpmd_shell` method at some point as a solution to these
> problems. Realising after that it may be too big of a change, and
> possibly best left to a discussion exactly like this one.
>

The changes we discuss below don't seem that big. What do you think,
do we just add another patch to the series?

> Generics used at this level may be a bit too much, especially for
> Python, as support is not *that* great. I am of the opinion that having
> a dedicated wrapper is easier for the developer and the user. Generics
> are not needed to this level anyways, as we have a limited selection of
> shells that are actually going to be used.
>
> We can also swap the wrapping process to simplify things, instead of:
>
>    shell =3D self.sut_node.create_interactive_shell(TestPmdShell, ..)
>
> do:
>
>    shell =3D TestPmdShell(self.sut_node, ..)
>
> Let the Shell class ingest the node, and not the other way round.
>

I thought about this a bit as well, it's a good approach. The current
design is top-down, as you say, in that "I have a node and I do things
with the node, including starting testpmd on the node". But it could
also be "I have a node, but I also have other non-node resources at my
disposal and it's up to me how I utilize those". If we can make the
imports work then this is likely the best option.

> The current approach appears to me to be top-down instead of bottom-up.
> We take the most abstracted part and we work our way down. But all we
> want is concreteness to the end user (developer).
>
> > Let me illustrate this on the TestPmdShell __init__() method I had in m=
ind:
> >
> > def __init__(self, interactive_session: SSHClient,
> >          logger: DTSLogger,
> >          get_privileged_command: Callable[[str], str] | None,
> >          app_args: EalTestPmdParams | None =3D None,
> >          timeout: float =3D SETTINGS.timeout,
> >      ) -> None:
> >      super().__init__(interactive_session, logger, get_privileged_comma=
nd)
> >      self.state =3D TestPmdState()
> >
> > Where EalTestPmdParams would be something that enforces that
> > app_args.app_params is of the TestPmdParameters type.
> >
> > But thinking more about this, we're probably better off switching the
> > params composition. Instead of TestPmdParameters being part of
> > EalParameters, we do it the other way around. This way the type of
> > app_args could just be TestPmdParameters and the types should work.
> > Or we pass the args separately, but that would likely require ditching
> > the factories and replacing them with methods (or overloading them).
> >
> > And hopefully the imports won't be impossible to solve. :-)
>
> It is what I feared, and I think it may become even more convoluted. As
> you said, ditching the factories will simplify things and make it more
> straightforward. So, we wouldn't find ourselves in problems like these.
>
> I don't have a strong preference in approach between:
> * overloading node methods
> * dedicated node methods
> * let the shells ingest nodes instead
>
> But if I were to give priority, I'd take it from last to first. Letting
> shells ingest nodes will decouple the situation adding an extra step of
> simplification.

+1 for simplification.

> I may not see the full picture though. The two are
> reasonable but, having a dedicated node method will stop the requirement
> to import the shell we need, and it's pretty much equivalent... but
> overloading also is very new to Python, so I may prefer to stick to more
> established.
>

Let's try shells ingesting nodes if the imports work out then. If not,
we can fall back to dedicated node methods.

> Letting TestPmdParams take EalParams, instead of the other way around,
> would naturally follow the bottom-up approach too. Allowing Params to
> arbitrarily append string arguments =E2=80=93 as proposed, would also all=
ow
> users to use a plain (EalParams + string). So sounds like a good
> approach overall.