[Fuego] [PATCH 3/3] ftc: add dynamic-vars to ftc run-test

Fri Oct 5 01:43:04 UTC 2018

Hi Tim,

Sorry for the late reply!

> -----Original Message-----
> From: Tim.Bird at sony.com <Tim.Bird at sony.com>
> Sent: Saturday, September 8, 2018 3:56 AM
> To: daniel.sangorrin at toshiba.co.jp; fuego at lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH 3/3] ftc: add dynamic-vars to ftc run-test
> 
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > This patch adds the ability to override parameters of a
> > base spec (if not specified, the default spec is used)
> > from ftc run-test. The values are logged into the run's
> > log folder as spec.json. It is useful to modify the
> > parameters given to a test in a quick manner.
> >
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin at toshiba.co.jp>
> > ---
> >  engine/scripts/ftc | 54
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> ++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > index 2840f5c..875ed59 100755
> > --- a/engine/scripts/ftc
> > +++ b/engine/scripts/ftc
> > @@ -242,6 +242,7 @@ You can obtain a list of run_ids for runs on the local
> > system with
> >      """Usage: ftc run-test -b <board> -t <test> [-s <spec>] [-p <phases>]
> >      [-k <kill timeout>] [--rebuild <true|false>] [--reboot <true|false>]
> >      [--precleanup <true|false>] [--postcleanup <true|false>]
> > +    [--dynamic-vars python_dict]
> >  Run the indicated test on the specified board.  Use the
> >  indicated spec, if one is provided.
> >
> > @@ -251,6 +252,8 @@ rebuild: if true rebuild the test source even if it was
> > already built.
> >  reboot: if true reboot the board before the test.
> >  precleanup: if true clean the board's test folder before the test.
> >  postcleanup: if true clean the board's test folder after the test.
> > +dynamic-vars: allows overriding the variables in a spec from the command
> > line.
> > +  Example: ftc run-test -b bbb -t Benchmark.Dhrystone --dynamic-vars
> > "{'LOOPS':'400000000'}"
> I'm not a huge fan of the python/json syntax for simple name/value items.
> However, I see the potential benefit of being able to specify arbitrary
> data structures here.
> 
> Would you object to an addition to also support a simplified name=value syntax?
> The idea would be something like this:
> --dynamic-vars "LOOPS=400000000,OTHER_THING=foo -bar"
> Which would be converted to:
> "{'LOOPS':'400000000','OTHER_THING':'foo -bar'}"
> 
> The code could use the first character to determine which syntax was being used:
> '{' for ast, and alpha-numeric for name=value synax.

The above should work with the patch I just sent.

> Out of curiosity - do you expect the most common use case is for people to
> type these on the command line, or have them coded into scripts, or have
> them generated by some other tool?  (e.g. a variation-creating tool, or
> a bisector tool?)

There are several use cases:
  - People trying to find the test parameters that best fit their boards. For example, the number of loops appropriated for their board. Once they found them, they can keep them in per-board files.
  - Developers that want to debug a failure and need to see what happends when they change the parameters.
  - Periodic CI scripts that run Fuego from the command line
  - Fuzzy tools that try to find corner cases where the test fails

I am interested in the latest use case. I want to provide a fuzzy tool for Fuego tests.

> >  A list of phase characters may be provided, to execute only those phases
> >    p = pre_test, c=pre_check, b=build, d=deploy, r=run,
> > @@ -3219,6 +3222,18 @@ def do_run_test(conf, options):
> >      else:
> >          error_out("Run-test command requires a test name")
> >
> > +    if '--dynamic-vars' in options:
> > +        try:
> > +            import ast
> > +            dyn_vars_str = options[options.index('--dynamic-vars') + 1]
> > +            dyn_vars = ast.literal_eval(dyn_vars_str)
> > +        except IndexError:
> > +            error_out('Dynamic vars not provided after --dynamic-vars.')
> > +        options.remove(dyn_vars_str)
> > +        options.remove('--dynamic-vars')
> > +    else:
> > +        dyn_vars = None
> > +
> >      if '-s' in options:
> >          try:
> >              spec_name = options[options.index('-s') + 1]
> > @@ -3226,7 +3241,8 @@ def do_run_test(conf, options):
> >              error_out('Testspec not provided after -s.')
> >          spec_list = get_specs(conf, test_name)
> >          if spec_name not in spec_list:
> > -            error_out('Unknown spec %s' % spec_name)
> > +            if not dyn_vars:
> > +                error_out('Unknown spec %s' % spec_name)
> >          options.remove(spec_name)
> >          options.remove('-s')
> >          test_dict["spec"] = spec_name
> > @@ -3405,6 +3421,12 @@ def do_run_test(conf, options):
> >          build_number = find_next_build_number_by_log_scan(logs_dir)
> >
> >      build_data.build_number = build_number
> > +    build_data.test_logdir = conf.FUEGO_RW + '/logs/' + \
> > +        build_data.test_name    + '/' + \
> > +        build_data.board_name   + '.' + \
> > +        build_data.spec_name    + '.' + \
> > +        build_data.build_number + '.' + \
> > +        build_data.build_number
> >
> >      os.environ["EXECUTOR_NUMBER"] = "0"
> >      os.environ["BUILD_ID"] = build_data.build_number
> > @@ -3437,6 +3459,36 @@ def do_run_test(conf, options):
> >      os.environ["Target_PreCleanup"] = build_data.precleanup_flag
> >      os.environ["Target_PostCleanup"] = build_data.postcleanup_flag
> >
> > +    # prepare a per-run spec.json file
> > +    specpath = '%s/engine/tests/%s/spec.json' % (conf.FUEGO_CORE,
> > test_name)
> > +    with open(specpath) as f:
> > +        try:
> > +            test_spec_data = json.load(f)
> > +        except:
> > +            error_out("Error parsing spec file %s" % specpath)
> > +
> > +    for key in test_spec_data['specs'].keys():
> > +        if key != test.spec:
> > +            del test_spec_data['specs'][key]
> > +
> > +    print "dynvars: " + str(dyn_vars)
> > +    if dyn_vars:
> > +        if test.spec not in test_spec_data['specs']:
> > +            test_spec_data['specs'][test.spec] = dyn_vars
> > +        else:
> > +            for key in dyn_vars.keys():
> > +                test_spec_data['specs'][test.spec][key] = dyn_vars[key]
> > +        # track what variables where modified
> > +        test_spec_data['specs'][test.spec]['dyn_vars'] = dyn_vars.keys()
> > +
> > +    print "test spec data" + str(test_spec_data)
> > +
> > +    # FIXTHIS: use a more pythonic way
> > +    os.system("mkdir -p " + build_data.test_logdir)
> > +
> > +    with open(build_data.test_logdir + '/spec.json', 'w+') as spec_file:
> > +        json.dump(test_spec_data, spec_file)
> > +
> >      # cd to buildzone directory
> >      saved_cur_dir = os.getcwd()
> >      os.chdir(build_data.workspace)
> > --
> > 2.7.4
> 
> Ok - looks good.  Applied.
> 
> Just to clarify this feature:
>  - if dynamic variables are specified, ftc creates logdir/spec.json with the dynamic
> variables overriding any
>   values in the specified 'spec', that came from testdir/spec.json
>     - also, the spec data is filtered to just the spec that will be used for the run
>     - the overlay generator will use logdir/spec.json
>     - the list of dynamic vars is noted in the generated spec.json
>  - if no dynamic variables are specified, ftc does nothing, and the overlay generator
> copies
>    the spec data from testdir/spec.json into logdir/spec.json
>    - the spec data is filtered to just the spec that will be used for the run

Yes, that's correct.

> This seems like a duplication of code.  Is this to support backwards compatibility
> with jobs
> that still call main.sh, when we do the switchover to calling 'ftc run-test'?

Yes that is also correct. I have send a patch that removes the backwards compatibility now that we switched over to calling ftc run-test, but you can turn it into a "Warning, this is deprecated" sort of thing. Probably, that is the wiser choice.

> If so, thanks.  Good job!
>  -- Tim

Thanks to you!
Daniel