Re: [dts] [PATCH v1] python: standard project structure

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

[-- Attachment #1: Type: text/plain, Size: 3099 bytes --]



From: Owen Hilyard <ohilyard@iol.unh.edu>
Sent: Tuesday, August 24, 2021 3:47 PM
To: Juraj Linkeš <juraj.linkes@pantheon.tech>
Cc: Tu, Lijuan <lijuan.tu@intel.com>; dts@dpdk.org
Subject: Re: [PATCH v1] python: standard project structure

(and consequently remove the redundant dts shell script).

I don't think we should be making a breaking change to the external interface right now. It may be worth it to discuss adding a deprecation warning to the script, but we should give time for anyone who is using DTS to using python directly. This includes the Community CI Lab, since this change would break our current approach to updating DTS, which is to have a "canary" system updated and running for a bit to find potential issues before everything else gets updated.

Good point, changing the API should definitely not be done in this patch. I'll revert this.

1. Should we do the explicit import paths?

It may be easier to force absolute import paths since I know some older automated tooling might now handle relative imports correctly. They also make refactoring much easier if we ever need to move files around again.

2. If so, do we want to enforce it (for new patches)? How (is there a tool that check that or just manually in review)?

I think we should just to keep everything consistent throughout the codebase. I don't know if there is a tool, but it should be easy to look at the top of files in a patch and check for relative imports.

This is what I was also thinking - there's value in using them and manual reviews should be sufficient.

3. Some paths might not have been moved to the explicit format, as I may have missed them.
4. I did not test the patch as I don't have access to an environment in which I could do so. Is there a way to test it locally (i.e. without any hw dependencies)?

The easiest way would probably be to put something like this in main to test:

from framework import *
from framework.flow import *
from framework.ixia_network import *
from tests import *
from nics import *

This should import every single file in the project, which will cause every file to evaluate its own imports. As long as nothing imports main, that should work.

Ok, I'll try this.

5. Do we want to sort imports?

I think that we should ask that imports are logically grouped, but not prescribe any particular method unless we start to have issues.

6. If so, how? Do we want to do it in this patch?

It might make sense to do a single pass now using isort (https://github.com/PyCQA/isort). We can discuss adding that as a tool later, but unless there's an easy way to add it as a commit hook, it might make sense to do it once and leave it be. DTS has a lot of other stuff to work on, and I think that this is something we can discuss once we've fixed some of the bigger issues.

I'll do the sort now then.
For now, we could add this to the "reviewed by DTS workgroup" requirements (along with the relative imports) and just check it manually during review. In the future, we could just add a check to a syntax checking job.

[-- Attachment #2: Type: text/html, Size: 9838 bytes --]