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 59EA543E43; Thu, 11 Apr 2024 13:47:08 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F02F24029C; Thu, 11 Apr 2024 13:47:07 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id B864F40268 for ; Thu, 11 Apr 2024 13:47:06 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7E58E113E; Thu, 11 Apr 2024 04:47:35 -0700 (PDT) Received: from [192.168.50.86] (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2CCB93F64C; Thu, 11 Apr 2024 04:47:05 -0700 (PDT) Message-ID: <5d35012d-0ffd-4c23-ad0c-8315453b8c9e@arm.com> Date: Thu, 11 Apr 2024 12:47:04 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 6/6] dts: add statefulness to TestPmdShell Content-Language: en-GB To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Jeremy Spewock , dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-7-luca.vizzarro@arm.com> <1db1b2b8-fac0-41ad-9ba2-911365385a9b@arm.com> From: Luca Vizzarro In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 11/04/2024 11:30, Juraj Linkeš 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. 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 = self.sut_node.create_interactive_shell(TestPmdShell, ..) do: shell = TestPmdShell(self.sut_node, ..) Let the Shell class ingest the node, and not the other way round. 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 mind: > > def __init__(self, interactive_session: SSHClient, > logger: DTSLogger, > get_privileged_command: Callable[[str], str] | None, > app_args: EalTestPmdParams | None = None, > timeout: float = SETTINGS.timeout, > ) -> None: > super().__init__(interactive_session, logger, get_privileged_command) > self.state = 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. 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. 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 – as proposed, would also allow users to use a plain (EalParams + string). So sounds like a good approach overall.