Re: [PATCH] dts: improve documentation

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Fri, Jan 12, 2024 at 6:16 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Hi Juraj,
>
> Thank you for your review!
>
> On 12/01/2024 13:24, Juraj Linkeš wrote:
> > I have two extra suggestions apart from the comments below:
> > There's a typo inside the "How To Write a Test Suite" section:
> > In that case, nothing will happen when they're is executed.
> >
> > And Mypy is missing from the list of linters in the "DTS Developer
> > Tools" section, could you please add it?
>
> Ack.
>
> > Just out of curiosity, is this generated from the schema? It's a
> > pretty neat format, but maintaining it could be a nightmare without a
> > script that would always produce the same format.
>
> I originally found only one tool that would generate rst. The generated
> output was quite messy though. Only after hacking its few configuration
> options and the schema, it produced something decent. But as it is only
> available on pip and it would have to become a DPDK docs requirement to
> generate them, it wouldn't be acceptable.
>

This wouldn't need to be a hard requirement. It could just be a tool
that submitters could use as a starting point (which they would
optionally install and use).

> Unless someone comes up with a good tool that could match our needs,
> unfortunately manual work is the only solution for the time being...and
> I won't deny it took me a bit of time to format it. The only major
> problem is all the extra whitespaces and the alignment of the columns
> needed to make sphinx happy. Once most of the work is done though – as
> it is in this case, changing it from there shouldn't be too bad.
>

Alright, I hope people won't be too frustrated with it. :-) But it's
likely the section is not going to be changed that much so it's not a
big deal.

> > The section names look to be taken from the schema and all of the
> > terminology is taken from the schema. Would it make sense to use YAML
> > terminology here, since people will try to use this information in
> > YAML files? Or maybe explain what we mean by definitions, properties,
> > objects, arrays or maybe some other things which YAML doesn't specify.
>
> I understand your point of view. I had a quick look at the YAML glossary
> and I think it may be a lot more confusing than using generic software
> engineering terminology. Also we may not want to be constrained to YAML.
>
> The YAML glossary refers to what we would call dictionary in Python and
> objects in JSON as mappings. And arrays and lists as sequences. Maybe
> changing array to list could be a good idea. Objects instead is
> something I don't personally like either. I took it from the JSON schema
> to rst generator tool, as I am not sure what fits best here.
>

I like mappings [0] and sequences [1], as that is the standard Python
terminology.

[0] https://docs.python.org/3/library/stdtypes.html#mapping-types-dict
[1] https://docs.python.org/3/library/stdtypes.html#sequence-types-list-tuple-range

> I welcome suggestions to change specific terms. On the other hand, I am
> not so sure about explaining them as it is out of scope. Moreover, the
> configuration template should cover all of the scenarios, so one can
> also infer usage from there.
>

One more thing to like about mappings and sequences, we don't (or
shouldn't) have to explain those. I think those would work fine on
both fronts.

> > We should update "amount" (uncountable) to something that's countable,
> > such as count or number.
>
> Ack.
>
> > Of note here is that some traffic generators (to which the port config
> > also applies) won't be using os_driver_for_dpdk (such as Scapy), but
> > rather os_driver, so the use is broader.
>
> Ack. Thanks for the explanation, makes sense.
>
> > The last newline is missing.
>
> Ack.
>
> > Let's keep the note that the skip_smoke_tests flag is optional
>
> Ack.
>
> > The traffic generator may need this core configuration. However, since
> > it's not required for all traffic generators (such as Scapy), we could
> > just move to the traffic_generator section. That would require some
> > code modifications though, but even the removal of lcores and
> > use_first_core should be addressed in the configuration classes as
> > well. Have you tried running DTS with these changes?
>
> Please correct me if I'm wrong. At the current state of DTS it seems
> that these two properties are only used with DPDK. If this is correct,
> it may be misleading to the user to add them for the traffic generator
> node, hinting that they may make a difference when they don't.
>

That's right, it's only used in DPDK.

> It would definitely makes sense to have dedicated properties for DPDK
> and for the traffic generators, instead of common ones.

The dedicated DPDK/TG properties look like the way to go, it just makes sense.

> But this is
> possibly more of a concern when and if support for other traffic
> generators will be added in the future.
>

We'll need to add support for at least one traffic generator that's
suitable for performance testing (Scapy is too slow) and then we'll
need to extra config (such as cores for T-Rex).

> The removal of `lcores` and `use_first_core` does not affect the
> execution as both have default values they fallback to, as seen in
> `dts/framework/config/__init__.py#NodeConfiguration.from_dict`.
>
> Yes, I tried running DTS and wasn't met by anything unusual.

Ah, the defaults made it work. Let's remove the tg node config from
the yaml file then, as you suggested.

>
> Best,
> Luca