Re: [PATCH] make fio scripts python3-ready

[Bill O'Donnell <billodo@redhat.com>]

On Fri, May 04, 2018 at 05:29:42PM +0100, Sitsofe Wheeler wrote:
> Hi,
> 
> On 4 May 2018 at 15:34, Bill O'Donnell <billodo@redhat.com> wrote:
> > Many distributions are moving to python3 by default.  Here's
> > an attempt to make the python scripts in fio python3-ready.
> >
> > Conversion was facilitated with automated tools. A few areas
> > were hand fixed: remove superfluous parentheses introduced by
> > 2to3 converter in print function calls, shebang modifications
> > to use environment variable for python version, and byte-string
> > decode correction in steadystate_tests.py following 2to3
> > conversion.
> >
> > The modified scripts pass rudimentary testing when run under
> > python2.7 as well as python3.
> >
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  doc/conf.py                     |  3 +++
> >  tools/fiologparser.py           |  6 +++++-
> >  tools/hist/fiologparser_hist.py | 17 +++++++++++------
> >  unit_tests/steadystate_tests.py | 21 ++++++++++++---------
> >  4 files changed, 31 insertions(+), 16 deletions(-)
> >
> > diff --git a/doc/conf.py b/doc/conf.py
> > index d4dd9d20..087a9a11 100644
> > --- a/doc/conf.py
> > +++ b/doc/conf.py
> > @@ -22,6 +22,9 @@
> >
> >  # -- General configuration ------------------------------------------------
> >
> > +from __future__ import absolute_import
> > +from __future__ import print_function
> > +
> >  # If your documentation needs a minimal Sphinx version, state it here.
> >  #
> >  # needs_sphinx = '1.0'
> > diff --git a/tools/fiologparser.py b/tools/fiologparser.py
> > index 8549859f..14ddc417 100755
> > --- a/tools/fiologparser.py
> > +++ b/tools/fiologparser.py
> > @@ -1,4 +1,4 @@
> > -#!/usr/bin/python2.7
> > +#!/usr/bin/env python
> 
> Apparently there's a distro or operating system that discourages or
> doesn't like every type of python #! line you can come up with (see
> https://github.com/axboe/fio/commit/60023ade47e7817db1c18d9b7e511839de5c2c99
> ). For now let's leave it at #!/usr/bin/python2.7 but add a comment
> beneath saying that the script actually works with python2 or python3 (also
> see the discussion in https://www.spinics.net/lists/fio/msg06746.html
> , https://www.spinics.net/lists/fio/msg06748.html and
> https://www.spinics.net/lists/fio/msg06977.html ). Distros can then
> use a uniform tool at packaging time to convert all the scripts to
> whatever their preferred syntax is.

Agreed.

> 
> >  #
> >  # fiologparser.py
> >  #
> > @@ -13,8 +13,12 @@
> >  #
> >  # to see per-interval average completion latency.
> >
> > +from __future__ import absolute_import
> > +from __future__ import print_function
> >  import argparse
> >  import math
> > +from six.moves import range
> > +from functools import reduce
> 
> I wonder if some of these imports are actually redundant and could be skipped...
>

Indeed, these amount to overdone future-proofing by the automated converter.

> >
> >  def parse_args():
> >      parser = argparse.ArgumentParser()
> > diff --git a/tools/hist/fiologparser_hist.py b/tools/hist/fiologparser_hist.py
> > index 8910d5fa..958c9141 100755
> > --- a/tools/hist/fiologparser_hist.py
> > +++ b/tools/hist/fiologparser_hist.py
> 
> Isn't this one already python 3 compatible? (see the thread in
> https://www.spinics.net/lists/fio/msg06709.html for where Kris[[
> converted it).

Ah...Yes. Kris' changes along with bb36ebad32a8 make my changes unnecessary.

I'll fix a version 2 of the patch.

> 
> > @@ -1,4 +1,4 @@
> > -#!/usr/bin/python2.7
> > +#!/usr/bin/env python
> >  """
> >      Utility for converting *_clat_hist* files generated by fio into latency statistics.
> >
> > @@ -13,11 +13,16 @@
> >
> >      @author Karl Cronburg <karl.cronburg@gmail.com>
> >  """
> > +from __future__ import absolute_import
> > +from __future__ import print_function
> >  import os
> >  import sys
> >  import pandas
> >  import re
> >  import numpy as np
> > +from six.moves import map
> > +from six.moves import range
> > +from six.moves import zip
> >
> >  runascmd = False
> >
> > @@ -124,7 +129,7 @@ def gen_output_columns(ctx):
> >          columns = ["end-time", "dir", "samples", "min", "avg", "median"]
> >      else:
> >          columns = ["end-time", "samples", "min", "avg", "median"]
> > -    columns.extend(list(map(lambda x: x+'%', strpercs)))
> > +    columns.extend(list([x+'%' for x in strpercs]))
> >      columns.append("max")
> >
> >  def fmt_float_list(ctx, num=1):
> > @@ -254,7 +259,7 @@ def print_all_stats(ctx, end, mn, ss_cnt, vs, ws, mx, dir=dir):
> >          # max and min are decimal values if no divisor
> >          fmt = fmt + "%d, " + fmt_float_list(ctx, len(percs)+1) + ", %d"
> >
> > -    print (fmt % tuple(row))
> > +    print(fmt % tuple(row))
> >
> >  def update_extreme(val, fncn, new_val):
> >      """ Calculate min / max in the presence of None values """
> > @@ -338,8 +343,8 @@ def guess_max_from_bins(ctx, hist_cols):
> >          bins = [ctx.group_nr * (1 << 6)]
> >      else:
> >          bins = [1216,1280,1344,1408,1472,1536,1600,1664]
> > -    coarses = range(max_coarse + 1)
> > -    fncn = lambda z: list(map(lambda x: z/2**x if z % 2**x == 0 else -10, coarses))
> > +    coarses = list(range(max_coarse + 1))
> > +    fncn = lambda z: list([z/2**x if z % 2**x == 0 else -10 for x in coarses])
> >
> >      arr = np.transpose(list(map(fncn, bins)))
> >      idx = np.where(arr == hist_cols)
> > @@ -473,7 +478,7 @@ def main(ctx):
> >          try:
> >              from configparser import SafeConfigParser, NoOptionError
> >          except ImportError:
> > -            from ConfigParser import SafeConfigParser, NoOptionError
> > +            from six.moves.configparser import SafeConfigParser, NoOptionError
> >
> >          cp = SafeConfigParser(allow_no_value=True)
> >          with open(ctx.job_file, 'r') as fp:
> > diff --git a/unit_tests/steadystate_tests.py b/unit_tests/steadystate_tests.py
> > index 5a74f956..8a5189ce 100755
> > --- a/unit_tests/steadystate_tests.py
> > +++ b/unit_tests/steadystate_tests.py
> > @@ -1,4 +1,4 @@
> > -#!/usr/bin/python2.7
> > +#!/usr/bin/env python
> >  #
> >  # steadystate_tests.py
> >  #
> > @@ -18,6 +18,8 @@
> >  # if ss attained: min runtime = ss_dur + ss_ramp
> >  # if not attained: runtime = timeout
> >
> > +from __future__ import absolute_import
> > +from __future__ import print_function
> >  import os
> >  import sys
> >  import json
> > @@ -26,11 +28,12 @@ import pprint
> >  import argparse
> >  import subprocess
> >  from scipy import stats
> > +from six.moves import range
> >
> >  def parse_args():
> >      parser = argparse.ArgumentParser()
> >      parser.add_argument('fio',
> > -                        help='path to fio executable');
> > +                        help='path to fio executable')
> >      parser.add_argument('--read',
> >                          help='target for read testing')
> >      parser.add_argument('--write',
> > @@ -45,7 +48,7 @@ def check(data, iops, slope, pct, limit, dur, criterion):
> >      data = data[measurement]
> >      mean = sum(data) / len(data)
> >      if slope:
> > -        x = range(len(data))
> > +        x = list(range(len(data)))
> >          m, intercept, r_value, p_value, std_err = stats.linregress(x,data)
> >          m = abs(m)
> >          if pct:
> > @@ -89,11 +92,11 @@ if __name__ == '__main__':
> >                    'output': "set steady state BW threshold to 12" },
> >                ]
> >      for test in parsing:
> > -        output = subprocess.check_output([args.fio] + test['args']);
> > -        if test['output'] in output:
> > -            print "PASSED '{0}' found with arguments {1}".format(test['output'], test['args'])
> > +        output = subprocess.check_output([args.fio] + test['args'])
> > +        if test['output'] in output.decode():
> > +            print("PASSED '{0}' found with arguments {1}".format(test['output'], test['args']))
> >          else:
> > -            print "FAILED '{0}' NOT found with arguments {1}".format(test['output'], test['args'])
> > +            print("FAILED '{0}' NOT found with arguments {1}".format(test['output'], test['args']))
> >
> >  #
> >  # test some read workloads
> > @@ -117,7 +120,7 @@ if __name__ == '__main__':
> >              args.read = '/dev/zero'
> >              extra = [ "--size=134217728" ]  # 128 MiB
> >          else:
> > -            print "ERROR: file for read testing must be specified on non-posix systems"
> > +            print("ERROR: file for read testing must be specified on non-posix systems")
> >              sys.exit(1)
> >      else:
> >          extra = []
> > @@ -216,7 +219,7 @@ if __name__ == '__main__':
> >                  else:
> >                      result = 'FAILED '
> >                  line = result + line + ' no ss, expected runtime {0} ~= actual runtime {1}'.format(expected, actual)
> > -            print line
> > +            print(line)
> >              if 'steadystate' in jsonjob:
> >                  pp.pprint(jsonjob['steadystate'])
> >          jobnum += 1
> > --
> > 2.14.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe fio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Sitsofe | http://sucs.org/~sits/