<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="SK" link="blue" vlink="purple"> <div class="WordSection1"> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p> <div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm"> <p class="MsoNormal"><b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Owen Hilyard <ohilyard@iol.unh.edu> <br> <b>Sent:</b> Tuesday, August 24, 2021 3:47 PM<br> <b>To:</b> Juraj Linkeš <juraj.linkes@pantheon.tech><br> <b>Cc:</b> Tu, Lijuan <lijuan.tu@intel.com>; dts@dpdk.org<br> <b>Subject:</b> Re: [PATCH v1] python: standard project structure<o:p></o:p></span></p> </div> </div> <p class="MsoNormal"><o:p> </o:p></p> <div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm"> <p class="MsoNormal">(and consequently remove the redundant dts shell script).<o:p></o:p></p> </blockquote> <div> <p class="MsoNormal"><o:p> </o:p></p> </div> <div> <p class="MsoNormal">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.<o:p></o:p></p> </div> <div> <p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Good point, changing the API should definitely not be done in this patch. I'll revert this.<o:p></o:p></span></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p> </div> <div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm"> <p class="MsoNormal">1. Should we do the explicit import paths?<o:p></o:p></p> </blockquote> <div> <p class="MsoNormal"><o:p> </o:p></p> </div> <div> <p class="MsoNormal">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.<o:p></o:p></p> </div> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm"> <p class="MsoNormal">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)?<o:p></o:p></p> </blockquote> <div> <p class="MsoNormal"><o:p> </o:p></p> </div> <div> <p class="MsoNormal">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. <o:p></o:p></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">This is what I was also thinking - there's value in using them and manual reviews should be sufficient.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm"> <p class="MsoNormal">3. Some paths might not have been moved to the explicit format, as I may have missed them.<br> 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)?<o:p></o:p></p> </blockquote> <div> <p class="MsoNormal"><o:p> </o:p></p> </div> <div> <p class="MsoNormal">The easiest way would probably be to put something like this in main to test:<br> <br> <span style="font-family:"Courier New"">from framework import *</span><o:p></o:p></p> </div> <div> <p class="MsoNormal"><span style="font-family:"Courier New"">from framework.flow import *</span><o:p></o:p></p> </div> <div> <p class="MsoNormal"><span style="font-family:"Courier New"">from framework.ixia_network import *</span><o:p></o:p></p> </div> <div> <p class="MsoNormal"><span style="font-family:"Courier New"">from tests import *</span><o:p></o:p></p> </div> <div> <p class="MsoNormal"><span style="font-family:"Courier New"">from nics import *</span><o:p></o:p></p> </div> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> <div> <p class="MsoNormal">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. <o:p></o:p></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Ok, I'll try this.<o:p></o:p></span></p> </div> <div> <p class="MsoNormal"><o:p> </o:p></p> </div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm"> <p class="MsoNormal">5. Do we want to sort imports?<o:p></o:p></p> </blockquote> <div> <p class="MsoNormal"><o:p> </o:p></p> </div> <div> <p class="MsoNormal">I think that we should ask that imports are logically grouped, but not prescribe any particular method unless we start to have issues. <o:p></o:p></p> </div> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> <blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm"> <p class="MsoNormal">6. If so, how? Do we want to do it in this patch?<o:p></o:p></p> </blockquote> </div> <div> <p class="MsoNormal"> <o:p></o:p></p> </div> <div> <p class="MsoNormal">It might make sense to do a single pass now using isort (<a href="https://github.com/PyCQA/isort">https://github.com/PyCQA/isort</a>). 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. <o:p></o:p></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">I'll do the sort now then.<o:p></o:p></span></p> <p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">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.<o:p></o:p></span></p> </div> </div> </div> </div> </body> </html>