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 A9F4843F47; Mon, 29 Apr 2024 14:06:20 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2C00F402A3; Mon, 29 Apr 2024 14:06:20 +0200 (CEST) Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by mails.dpdk.org (Postfix) with ESMTP id 98CB54025C for ; Mon, 29 Apr 2024 14:06:17 +0200 (CEST) Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-a58872c07d8so955730666b.0 for ; Mon, 29 Apr 2024 05:06:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1714392377; x=1714997177; 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=DXBwBnX8Xz9pglbaVcE1EUYzDZNm29ed2KonaJadwYc=; b=Qsdi8AbnH2xPoeIJtT29aOu1bNfMIIK+ZKSW2Pb+apjl6hvYD+pd1x9zRwXiINkAvn 8VKIvpPymjpXV97efuX3t/kOpPVusQbSAzQwQQGRItwUvRPUpuNA20yjL8QSQo9JWKpo /pFH6Bcs2Gys8AH7Lobw2cSu5MgS1EhD8ee04REw0Pc/yHKCB6kTb+CCuNo3aBEYbkZd mhfXGgJlwg/7rUE7T0xpa+ikRLPazfcUJLhwoXKJS2fN8gverOITNbs4ABSQhBBYjT95 S2E2TYQzr2zobelRO5RquPEbiGq3nhDTSL/0wBFcKmtebEY/0vMhxMOWRVWsD7eNvmQK xoUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714392377; x=1714997177; 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=DXBwBnX8Xz9pglbaVcE1EUYzDZNm29ed2KonaJadwYc=; b=bv1gWLS/qJ7KRTPfVmjRWO49SF5Dd3WBENe3jwj3eISdCaGsYmg647RbbBzGhaMbm1 a89yJTQmzFuQCz/y0q4YmVUInxkzHhzvsyoIJx6qL586xDTsr4W+w23HC9ttbfx9zzMr KyyaT2VWimZ1XfGDDULoacqrqKoF7FnkRqAxpCbZ5e9GaTLoAeiwxMZMc7BN0clo2eU7 FL9Ojf8uneURWTZCh5UP9BLj0DW1MfneoFEFcaew2WW92FmPwxJOESM0U+W/4YIW2unY l+LWlHFOM2QmHrhqfr7Pj20jnxnyfAervbAF/uT6/E6M2oiLrNtAZCKYNFuHg8FGquP5 tZbg== X-Forwarded-Encrypted: i=1; AJvYcCV6A1iryuIeTXoYULvqzz1cArN3giE4FyQErmM6XBlGdt70FuK5PW3TJ23JiBoAKsDn1joiWSX2JeWo0TM= X-Gm-Message-State: AOJu0Yy3p2jfV3Qe2OOzoCw8MzMw2mitZxMk3VIx4g6OJ130E/1BEJZ+ 8yZhs8qc8Wh6ksBX4qqulCUD9EKrNe4jqIuqM6ElGJvQdfsV7J0tuS9S/3CzQZXXMuGmZMPZefo OQuPkIZYrDDxtUySAfVYsSP5WgTQ0G51jysFwUg== X-Google-Smtp-Source: AGHT+IH3MrzwRq6F1LweeCfV7vLVCAIADDWyKMWrBZ7zcnBwHonFpMSMIDZ/rR7omldlpgd3DlMS8qcu1YRz6ZXEQUg= X-Received: by 2002:a17:906:b80d:b0:a58:eac2:a54f with SMTP id dv13-20020a170906b80d00b00a58eac2a54fmr5605026ejb.18.1714392377044; Mon, 29 Apr 2024 05:06:17 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-7-luca.vizzarro@arm.com> <1db1b2b8-fac0-41ad-9ba2-911365385a9b@arm.com> <5d35012d-0ffd-4c23-ad0c-8315453b8c9e@arm.com> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 29 Apr 2024 14:06:06 +0200 Message-ID: Subject: Re: [PATCH 6/6] dts: add statefulness to TestPmdShell To: Jeremy Spewock Cc: Luca Vizzarro , dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli 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 Fri, Apr 26, 2024 at 8:06=E2=80=AFPM Jeremy Spewock wrote: > > Apologies for being so late on the discussion, but just a few of my > thoughts: > > * I think using something like overloading even though it is new to > python is completely fine because this new python version is a > dependency of the DTS runner. The DTS runner can have bleeding-edge > requirements because we manage that through a container to make things > easier. > > On Thu, Apr 11, 2024 at 8:13=E2=80=AFAM Juraj Linke=C5=A1 wrote: > > > > On Thu, Apr 11, 2024 at 1:47=E2=80=AFPM Luca Vizzarro wrote: > > > > > > On 11/04/2024 11:30, Juraj Linke=C5=A1 wrote: > > > > I've been thinking about these interactive shell constructors for s= ome > > > > 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, differ= ent > > > > apps are going to require different args and that alone kinda break= s > > > > the factory pattern. I think we'll need to either ditch these > > > > factories and instead just have methods that return the proper shel= l > > > > (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 fact= ory > > > > (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 th= e > > > > 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 wa= s > > > > 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 - bot= h > > > > 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 havi= ng > > > a dedicated wrapper is easier for the developer and the user. Generic= s > > > 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. > > It might be me slightly stuck in the old ways of doing things, but I > might slightly favor the overloading methods approach. This is really > because, at least in my mind, the SUT node is somewhat of a central > API for the developer to use during testing, so having a method on > that API for creating a shell for you to use on the node makes sense > to me. It creates more of a "one stop shop" kind of idea where > developers have to do less reading about how to do things and can just > look at the methods of the SUT node to get what they would need. > This was the case before we introduced the testpmd shell, which is a standalone object (as in the dev uses the sut node and the test pmd object to do what they need). One advantage of using sut methods to instantiate testpmd shells is that devs won't need to import the TestPmd shell, but I don't know which of these is going to be better. > That being said, I think in any other framework the passing of the > node into the shell would easily make more sense and I'm not opposed > to going that route either. In general, I agree that not using a > factory with a generic will make things much easier in the future. > > > > > > The current approach appears to me to be top-down instead of bottom-u= p. > > > 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 mind: > > > > > > > > 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_c= ommand) > > > > 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 t= he > > > > 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 ditch= ing > > > > 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 mor= e > > > straightforward. So, we wouldn't find ourselves in problems like thes= e. > > > > > > 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. Letti= ng > > > 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 requirem= ent > > > 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 m= ore > > > 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= allow > > > users to use a plain (EalParams + string). So sounds like a good > > > approach overall. > > This I like a lot. We don't want to force EalParams to have > TestpmdParams nested inside of them because other DPDK apps might need > them too and this fixes the issue of always having to assert what type > of inner params you have.