Re: [PATCH 1/3] simplebench: add img_bench_templater.py

[Vladimir Sementsov-Ogievskiy]

19.08.2021 19:37, Hanna Reitz wrote:
> On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:
> > Add simple grammar-parsing template benchmark.
>
> This doesn’t really say much, and FWIW, for like ten minutes I thought this 
> would do something completely different than it did (while I was trying to 
> parse the help text).
>
> (I thought this was about formatting an existing test’s output, and that 
> “template” were kind of the wrong word, but then it turned out it’s exactly the 
> right word, only that this is not about using a test’s output as a template, 
> but actually using a template of a test (i.e. a test template, not a template 
> test) to generate test instances to run...  Which of course is much cooler.)
>
> Functionality-wise, as far as I understand (of course I have no knowledge of 
> lark), this looks good to me.  And it’s really quite cool.
>
> I just found the documentation confusing, so I have some suggestions for it 
> below.
>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   scripts/simplebench/img_bench_templater.py | 85 ++++++++++++++++++++++
> >   scripts/simplebench/table_templater.py     | 62 ++++++++++++++++
> >   2 files changed, 147 insertions(+)
> >   create mode 100755 scripts/simplebench/img_bench_templater.py
> >   create mode 100644 scripts/simplebench/table_templater.py
> >
> > diff --git a/scripts/simplebench/img_bench_templater.py 
> > b/scripts/simplebench/img_bench_templater.py
> > new file mode 100755
> > index 0000000000..d18a243d35
> > --- /dev/null
> > +++ b/scripts/simplebench/img_bench_templater.py
> > @@ -0,0 +1,85 @@
> > +#!/usr/bin/env python3
> > +#
> > +# Run img-bench template tests
> > +#
> > +# Copyright (c) 2021 Virtuozzo International GmbH.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +
> > +import sys
> > +import subprocess
> > +import re
> > +import json
> > +
> > +import simplebench
> > +from results_to_text import results_to_text
> > +from table_templater import Templater
> > +
> > +
> > +def bench_func(env, case):
> > +    test = templater.gen(env['data'], case['data'])
> > +
> > +    p = subprocess.run(test, shell=True, stdout=subprocess.PIPE,
> > +                       stderr=subprocess.STDOUT, universal_newlines=True)
> > +
> > +    if p.returncode == 0:
> > +        try:
> > +            m = re.search(r'Run completed in (\d+.\d+) seconds.', p.stdout)
> > +            return {'seconds': float(m.group(1))}
> > +        except Exception:
> > +            return {'error': f'failed to parse qemu-img output: {p.stdout}'}
> > +    else:
> > +        return {'error': f'qemu-img failed: {p.returncode}: {p.stdout}'}
> > +
> > +
> > +if __name__ == '__main__':
> > +    if len(sys.argv) > 1:
> > +        print("""
> > +Usage: no arguments. Just pass template test to stdin. Template test is
>
> FWIW, I completely misunderstood this.
>
> At first, this sounded really ambiguous to me; then I thought that clearly this 
> must mean that one should pipe the test’s output to this script, i.e.
>
> $ path/to/test.sh | scripts/simplebench/img_bench_templater.py
>
> But now after reading more, I finally understand that this is not what is 
> meant, but actually literally passing some template of a test script to this 
> script, i.e.
>
> $ scripts/simplebench/img_bench_templater.py < path/to/test-template.sh
>
> So, two things; first, I believe it should be a “test template”, not a 
> “template test”, because this is about templates for a test, not about a test 
> that has something to do with templates.
>
> Second, perhaps we should start with what this does.
>
> Perhaps:
>
> “This script generates performance tests from a test template (example below), 
> runs them, and displays the results in a table. The template is read from 
> stdin.  It must be written in bash and end with a `qemu-img bench` invocation 
> (whose result is parsed to get the test instance’s result).”?

Yes, that's correct, thanks for wording

> > +a bash script, last command should be qemu-img bench (it's output is parsed
> > +to get a result). For templating use the following synax:
>
> “Use the following syntax in the template to create the various different test 
> instances:”?
>
> > +
> > +  column templating: {var1|var2|...} - test will use different values in
> > +  different columns. You may use several {} constructions in the test, in this
> > +  case product of all choice-sets will be used.
> > +
> > +  row templating: [var1|var2|...] - similar thing to define rows (test-cases)
> > +
> > +Test tempalate example:
>
> *template
>
> > +
> > +Assume you want to compare two qemu-img binaries, called qemu-img-old and
> > +qemu-img-new in your build directory in two test-cases with 4K writes and 64K
> > +writes. Test may look like this:
>
> I’d prefer s/Test/The template/.
>
> > +
> > +qemu_img=/path/to/qemu/build/qemu-img-{old|new}
> > +$qemu_img create -f qcow2 /ssd/x.qcow2 1G
> > +$qemu_img bench -c 100 -d 8 [-s 4K|-s 64K] -w -t none -n /ssd/x.qcow2
> > +
> > +If pass it to stdin of img_bench_templater.py, the resulting comparison table
>
> s/If pass it/When passing this/
>
> > +will contain two columns (for two binaries) and two rows (for two test-cases).
> > +""")
> > +        sys.exit()
> > +
> > +    templater = Templater(sys.stdin.read())
> > +
> > +    envs = [{'id': ' / '.join(x), 'data': x} for x in templater.columns]
> > +    cases = [{'id': ' / '.join(x), 'data': x} for x in templater.rows]
> > +
> > +    result = simplebench.bench(bench_func, envs, cases, count=5,
> > +                               initial_run=False)
> > +    print(results_to_text(result))
> > +    with open('results.json', 'w') as f:
> > +        json.dump(result, f, indent=4)
>
> Is this worth documenting?
>
> > diff --git a/scripts/simplebench/table_templater.py 
> > b/scripts/simplebench/table_templater.py
> > new file mode 100644
> > index 0000000000..950f3b3024
> > --- /dev/null
> > +++ b/scripts/simplebench/table_templater.py
> > @@ -0,0 +1,62 @@
> > +# Parser for test templates
> > +#
> > +# Copyright (c) 2021 Virtuozzo International GmbH.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +import itertools
> > +from lark import Lark
> > +
> > +grammar = """
> > +start: ( text | column_switch | row_switch )+
> > +
> > +column_switch: "{" text ["|" text]+ "}"
> > +row_switch: "[" text ["|" text]+ "]"
> > +text: /[^|{}\[\]]+/
>
> So I have no idea how this really works, of course, but does this mean that the 
> `text` pattern cannot contain pipe symbols?  I.e. that you cannot use pipes in 
> the test template?

Hmm. I didn't try. I hope lark is smart enough to keep pipes that are out of {} 
[] as is.. But of course, you can't hope that pipe inside {} or [] will work as 
bash-pipe.

Same thing with other special symbols ("{}" and "[]"). I don't want to care 
about this too much now. This simple grammar works good for test template in patch 03. If we need 
something more, we can add a kind of special symbols escaping later.

> > +"""
> > +
> > +parser = Lark(grammar)
> > +
> > +class Templater:
> > +    def __init__(self, template):
> > +        self.tree = parser.parse(template)
> > +
> > +        c_switches = []
> > +        r_switches = []
> > +        for x in self.tree.children:
> > +            if x.data == 'column_switch':
> > +                c_switches.append([el.children[0].value for el in x.children])
> > +            elif x.data == 'row_switch':
> > +                r_switches.append([el.children[0].value for el in x.children])
> > +
> > +        self.columns = list(itertools.product(*c_switches))
> > +        self.rows = list(itertools.product(*r_switches))
> > +
> > +    def gen(self, column, row):
> > +        i = 0
> > +        j = 0
> > +        result = []
> > +
> > +        for x in self.tree.children:
> > +            if x.data == 'text':
> > +                result.append(x.children[0].value)
> > +            elif x.data == 'column_switch':
> > +                result.append(column[i])
> > +                i += 1
> > +            elif x.data == 'row_switch':
> > +                result.append(row[j])
> > +                j += 1
> > +
> > +        return ''.join(result)


--
Best regards,
Vladimir