Re: [Fuego] [PATCH] Convert the print statement to the print() function

Hi Tim,

I am happy to talk with you again.

Thanks for your comments and please find my answers inline below.

>-----Original Message-----
>From: Fuego <[email protected]> On Behalf Of Bird, T=
im
>Sent: 30 September 2022 00:58
>To: nakkala sireesha(=1B$B#T#S#I#P=1B(B TMIEC ODG Porting) <sireesha.nakka=
la@toshiba-
>tsip.com>; [email protected]
>Cc: hayashi kazuhiro(=1B$BNS=1B(B =1B$BOB9(=1B(B =1B$B""#S#W#C"~#A#C#T=1B(=
B)
><[email protected]>; [email protected]; dinesh
>kumar(=1B$B#T#S#I#P=1B(B TMIEC ODG Porting) <[email protected]=
>
>Subject: Re: [Fuego] [PATCH] Convert the print statement to the print() fu=
nction
>
>Sireesha,
>
>I like the effort to make the scripts compatible with both the python2 and=
 python3
>interpreters, but I have some questions about portions of the patches.
>
>
>> -----Original Message-----
>> From: [email protected]
>> <[email protected]>
>>
>> From: sireesha <[email protected]>
>>
>> Update parentheses for all print statements in python script.
>> It is compatible with both python2 and python3 interpreters.
>>
>> Signed-off-by: sireesha <[email protected]>
>> ---
>>  tests/Benchmark.IOzone/parser.py   |  4 ++--
>>  tests/Benchmark.Stream/parser.py   |  4 ++--
>>  tests/Benchmark.bonnie/parser.py   |  8 ++++----
>>  tests/Benchmark.lmbench2/parser.py | 13 ++++++-------
>>  4 files changed, 14 insertions(+), 15 deletions(-)  mode change
>> 100755 =3D> 100644 tests/Benchmark.lmbench2/parser.py
>>
>> diff --git a/tests/Benchmark.IOzone/parser.py
>> b/tests/Benchmark.IOzone/parser.py
>> index b1631ae..8a7d5b2 100755
>> --- a/tests/Benchmark.IOzone/parser.py
>> +++ b/tests/Benchmark.IOzone/parser.py
>> @@ -1,11 +1,11 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>
>In my setup of Fuego, this will have no effect.  All invocations of the pa=
rser in
>Fuego execute parser.py as an argument of 'python'.  Specifically, see the
>run_python function in fuego-core/scripts/common.sh.  The actual invocatio=
n
>looks like this:
>
>PATH=3D$ORIG_PATH TOOLCHAIN=3D$TOOLCHAIN python "$@"
>(where the $@ arguments contain, among other things, 'parser.py')
>
>I believe Toshiba is using Fuego in a configuration where Fuego is run nat=
ively
>(outside the docker container).  Does /usr/bin/python in your environment =
default
>to python3?
>Or, did Toshiba change the run_python function to call python3 instead? or=
 to
>directly executed the parser.py script?

Actually, in first we were changing our environment (/usr/bin/python -> /us=
r/bin/python3) and run all fuego python scripts with default shebang.
But later we felt it is not good change, so we started updating the scripts=
 to use python3 always,
Also we modified fuego-core/scripts/common.sh as below

PATH=3D$ORIG_PATH TOOLCHAIN=3D$TOOLCHAIN python3 "$@"

We have not sent this patch to you because we are not sure how this patch a=
ffects the existing environment of yours or it expects to support both pyth=
on2 and 3?

May be we should discuss more about this and finalize the approach consider=
ing the backward compatibility?

I will send the patch in another mail that I am talking about, which will h=
ave python -> python3 changes for those tests we were using, maybe we can d=
ecide the approach after seeing the patch.

>
>I need to understand this so that I can decide what the best solution is f=
or
>supporting both docker-based and native-based Fuego installations, as well=
 as
>supporting legacy Fuego docker containers (which might not have a compatib=
le
>python3 interpreter).
>
>Also...
>If this change (to the #! header for the parser script) was needed, it wou=
ld be
>needed for all tests you are running.  Or, at least, I see unparenthesized=
 print
>statements in the following tests:
>IOzone, Stream, x11perf, gtkperf, tiobench, LTP_one_test, Interbench, iper=
f,
>ebizzy, bonnie, BF_power_test, aim7, blobsallad, fs_mark, nbench_byte, pte=
st,
>lmbench2
>
>It looks like the following test already have parenthesized print statemen=
ts:
>arch_time, LTP, autopkgtest, fuego_ftc_test, fuego_tguid_check,
>fuego_check_tables, fio, and dd
>
>
>I guess my question is what tests are you running (only those 4, or those =
4 and
>others)? and Do you plan to modify all the tests that have unparenthesized=
 print
>statements (ie, with additional patches)?
As of now we fixed print parenthesis problems in those tests which we were =
using
(dhrystone, hackbench, iperf3, lmbench2, fio, bonnie, iozone, stream)

and for the remaining tests we don=1B$B!G=1B(Bt have setup to test them aft=
er changing.
So, we are not planning to change in other tests.

>
>If so, I'll hold off doing it myself.  But if you don't plan to look at th=
e others, I'll do
>a sweep through the existing tests and adjust all print statements to be
>compatible with both python2 and python3.
>
>>
>>  import os, sys
>>  import common as plib
>>
>>  measurements =3D {}
>>  cur_file =3D open(plib.TEST_LOG,'r')
>> -print "Reading current values from " + plib.TEST_LOG +"\n"
>> +print("Reading current values from " + plib.TEST_LOG +"\n")
>
>This print (which a lot of parsers have) is not really that important.  It=
's more of a
>debug statement, and could probably be replaced with a call to dprint() (w=
hich is
>defined in common.py, which is always imported as plib (!) - there are so =
many
>oddities in this legacy code!), or removed entirely.
>
>In any event, I guess I should resist the urge to change all these to dpri=
nts().
>Doing so would change the test's default output, which might be a backward=
-
>compatibility-breaking change.
>
>Having said all that, this change to parenthesize the print statement is f=
ine.
>
>>
>>  lines =3D cur_file.readlines()
>>  cur_file.close()
>> diff --git a/tests/Benchmark.Stream/parser.py
>> b/tests/Benchmark.Stream/parser.py
>> index a564d74..69edd27 100755
>> --- a/tests/Benchmark.Stream/parser.py
>> +++ b/tests/Benchmark.Stream/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>Same issue here as above.
>
>>
>>  import os, re, sys
>>  import common as plib
>> @@ -7,7 +7,7 @@ ref_section_pat =3D "^\[[\w]+.[gle]{2}\]"
>>  # groups           1        2
>>  cur_search_str =3D "^(\w*):\ *([\d]{1,5}.[\d]{1,4}).*"
>>
>> -print "Reading current values from " + plib.TEST_LOG
>> +print("Reading current values from " + plib.TEST_LOG)
>>  cur_file =3D open(plib.TEST_LOG,'r')
>>
>>  lines =3D cur_file.readlines()
>> diff --git a/tests/Benchmark.bonnie/parser.py
>> b/tests/Benchmark.bonnie/parser.py
>> index b68bc3e..5f35af4 100755
>> --- a/tests/Benchmark.bonnie/parser.py
>> +++ b/tests/Benchmark.bonnie/parser.py
>> @@ -5,16 +5,16 @@ import common as plib
>>
>>  measurements =3D {}
>>
>> -print "Reading current values from " + plib.TEST_LOG + "\n"
>> +print("Reading current values from " + plib.TEST_LOG + "\n")
>>  with open(plib.TEST_LOG,'r') as cur_file:
>>      raw_values =3D cur_file.readlines()
>>      results =3D raw_values[-1].rstrip("\n").split(",")
>>
>>  if len(results) < 26:
>> -    print "\nFuego error reason: No results found\n"
>> +    print("\nFuego error reason: No results found\n")
>>      sys.exit(2)
>>
>> -print "Bonnie++ raw results: " + str(results)
>> +print("Bonnie++ raw results: " + str(results))
>>
>>
>>  measurements["Sequential_Output.PerChr"] =3D [] @@ -81,6 +81,6 @@ if
>> not '+' in results[26]:
>>
>>  # Add a hint when the spec seems to require a bigger SIZE  if '+' in
>> results[4]:
>> -    print "\nWARNING: Some test result data is missing. You might need =
bigger
>test data size.\nTry the test using the 'more-data' spec
>> for better results.\n"
>> +    print("\nWARNING: Some test result data is missing. You might
>> + need bigger test data size.\nTry the test using the 'more-data' spec
>> for better results.\n")
>>
>>  sys.exit(plib.process(measurements))
>> diff --git a/tests/Benchmark.lmbench2/parser.py
>> b/tests/Benchmark.lmbench2/parser.py
>> old mode 100755
>> new mode 100644
>> index f06f132..5ccfe97
>> --- a/tests/Benchmark.lmbench2/parser.py
>> +++ b/tests/Benchmark.lmbench2/parser.py
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/python
>> +#!/usr/bin/python3
>>
>>  import os, re, sys
>>  import common as plib
>> @@ -11,7 +11,7 @@ cur_search_pat =3D
>> re.compile("^\S+\s+Linux\s[0-9a-z.-]+\s+([\s\d.KMGT-]*)",re.MUL
>>
>>  cur_dict =3D {}
>>  cur_file =3D open(plib.TEST_LOG,'r')
>> -print "Reading current values from " + plib.TEST_LOG +"\n"
>> +print("Reading current values from " + plib.TEST_LOG +"\n")
>>
>>  lines =3D cur_file.readlines()
>>
>> @@ -19,18 +19,17 @@ lines =3D cur_file.readlines()  t_index =3D 0  subli=
st
>> =3D []
>>
>> -print lines
>> +print(lines)
>>
>>  for line in lines:
>>      result =3D cur_search_pat.findall(line)
>>      if result and len(result[0]) > 0:
>> -        print 'result: ', result
>> +        print('result: ', result)
>>
>>          # Ugly hack to work around invalid processing of empty cells
>>          result[0] =3D result[0].replace('       ', '     0 ')
>>          test_res =3D result[0].rstrip('\n').split(' ')
>> -
>> -        print "test_res =3D %s" % (test_res)
>> +        print("test_res =3D %s" % (test_res))
>>
>>          if 0 < t_index < 9:
>>              for value in test_res:
>> @@ -104,6 +103,6 @@ cur_dict["Memory_Latencies.Main_mem"] =3D
>> sublist[48]  #cur_dict["Memory_Latencies.Guesses"] =3D sublist[49]
>> cur_dict["Memory_Latencies.Rand_mem"] =3D sublist[50]
>>
>> -print "cur_dict =3D %s" % (cur_dict)
>> +print("cur_dict =3D %s" % (cur_dict))
>This is also one of those debug statements that should probably be a dprin=
t().
>
>I'm more inclined to change this one, because this is just showing interna=
l data of
>the parser, which really should not be displayed in the non-debug case.
>
>>
>>  sys.exit(plib.process_data(ref_section_pat, cur_dict, 'xl', ' '))
>> --
>> 2.20.1
>>
>
>OK.  Overall, all the print statement conversions are fine.  However, I do=
n't want
>to change the #!/usr/bin/python lines, as I don't think that has any actua=
l effect.
>If it does need to be changed, it will need to be changed globally in all =
parser.py
>files.  AND I'll have to go back and test to make sure that this will work=
 with
>previous docker containers (from Fuego 1.2 and above) to make sure they ha=
ve a
>compatible python3 interpreter installed.
>
Sorry for these lines (#!/usr/bin/python3), actually it is not intended to =
send these lines for the upstream,=20
Until it is decided to replace completely with python3.

A separate patch I am talking about in the above, supposed to have these li=
nes and add this change when it is decided to use.

>Thanks for raising this issue.  Please let me know the answers to my quest=
ions,
>and we'll get this change in (and possibly other related changes).
>
>We may have to drag Fuego kicking and screaming into full python3
>compatibility. :-)
>
> -- Tim
>
>
>_______________________________________________
>Fuego mailing list
>[email protected]
>https://lists.linuxfoundation.org/mailman/listinfo/fuego