On Thu, Apr 11, 2024 at 1:47 PM Luca Vizzarro <luca.vizza...@arm.com> wrote: > > 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. >
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 = 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. > 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 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. +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 – as proposed, would also allow > users to use a plain (EalParams + string). So sounds like a good > approach overall.
