[Fuego] [PATCH] common: add new criteria operator bt

Tim.Bird at sony.com Tim.Bird at sony.com
Thu Sep 27 18:34:35 UTC 2018


> -----Original Message-----
> From: Qiu Tingting
> 
> Add new criteria operator bt to support that criteria check result is pass when
> ouput value  is between min value and max value.
> e.g. of criteria file
>   "reference":{
>       "value":"7.5-10.5",

'-' is not a good range delimiter here, because it could be used as part
of a number value (for a negative number).
Possibly ',' would work, but this patch is quite involved and I didn't have
time to test it.

See more explanation below.

>       "operator":"bt"
>   }
> 
> Signed-off-by: Qiu Tingting <qiutt at cn.fujitsu.com>
> ---
>  engine/scripts/parser/common.py             | 20 +++++---
>  engine/scripts/parser/prepare_chart_data.py | 79
> +++++++++++++++++++++--------
>  2 files changed, 69 insertions(+), 30 deletions(-)
> 
> diff --git a/engine/scripts/parser/common.py
> b/engine/scripts/parser/common.py
> index 9f3a8bb..5bea7f6 100644
> --- a/engine/scripts/parser/common.py
> +++ b/engine/scripts/parser/common.py
> @@ -158,17 +158,19 @@ def check_measure(tguid, criteria_data, measure):
>          return 'SKIP'
> 
>      if reference['operator'] == 'lt':
> -        result = measure['measure'] < reference["value"]
> +        result = measure['measure'] < float(reference["value"])
>      elif reference['operator'] == 'le':
> -        result = measure['measure'] <= reference["value"]
> +        result = measure['measure'] <= float(reference["value"])
>      elif reference['operator'] == 'gt':
> -        result = measure['measure'] > reference["value"]
> +        result = measure['measure'] > float(reference["value"])
>      elif reference['operator'] == 'ge':
> -        result = measure['measure'] >= reference["value"]
> +        result = measure['measure'] >= float(reference["value"])
>      elif reference['operator'] == 'eq':
> -        result = measure['measure'] == reference["value"]
> +        result = measure['measure'] == float(reference["value"])
>      elif reference['operator'] == 'ne':
> -        result = measure['measure'] != reference["value"]
> +        result = measure['measure'] != float(reference["value"])
> +    elif reference['operator'] == 'bt':
> +        result = measure['measure'] >= float(reference["value"].split('-')[0])
> and measure['measure'] <= float(reference["value"].split('-')[1])
splitting on '-' I problematical, because it will cause problems with negative numbers.

For example, this code will fail with the range "-7--3"

I changed the range delimiter to ',' so the above could be expressed as:
"-7,-3"

>      status = 'PASS' if result else 'FAIL'
> 
>      dprint("  result=%s" % result)
> @@ -283,7 +285,7 @@ def convert_reference_log_to_criteria(filename):
>              old_measure_id, operation = crit_str.split("|")
>              ts_name, tc_name, measure_name = split_old_id(old_measure_id)
>              tguid = "%s.%s.%s" % (ts_name, tc_name, measure_name)
> -            value = float(lines[i+1])
> +            value = lines[i+1]
> 
>              reference = {"value":value, "operator": operation.strip()}
>              crit = {"tguid":tguid, "reference": reference }
> @@ -568,15 +570,17 @@ def process(results={}):
>      update_results_json(test_logdir, TESTDIR, REF_JSON)
> 
>      ref_map = {}
> +    op_map = {}
>      for crit in criteria_data["criteria"]:
>          # not all criteria are measure reference thresholds
>          try:
>              ref_map[crit["tguid"]] = crit["reference"]["value"]
> +            op_map[crit["tguid"]] = crit["reference"]["operator"]
>          except:
>              # ignore other data
>              pass
> 
> -    data_lines = store_flat_results(test_logdir, run_data, ref_map)
> +    data_lines = store_flat_results(test_logdir, run_data, ref_map, op_map)
>      make_chart_data(test_logdir, TESTDIR, CHART_CONFIG_JSON, data_lines)
> 
>      status = run_data.get('status', 'FAIL')
> diff --git a/engine/scripts/parser/prepare_chart_data.py
> b/engine/scripts/parser/prepare_chart_data.py
> index 4bf23cd..87af7fa 100644
> --- a/engine/scripts/parser/prepare_chart_data.py
> +++ b/engine/scripts/parser/prepare_chart_data.py
> @@ -66,9 +66,10 @@ class flat_entry:
>          self.tguid = \
>          self.test_set = \
>          self.ref = \
> +        self.op = \
>          self.result = "ERROR-undefined"
>          parts = line.split()
> -        if len(parts) != 9:
> +        if len(parts) < 9:
>              print("Error reading line '%s' in prepare_chart_data.py" % line)
>              print("Possible data corruption in flat_plot_data.txt")
> 
> @@ -81,6 +82,7 @@ class flat_entry:
>              self.kernel = parts[5]
>              self.tguid = parts[6]
>              self.ref = parts[7]
> +            self.op = parts[9]
>              self.result = parts[8]
> 
>              # break apart tguid
> @@ -103,10 +105,10 @@ class flat_entry:
>              pass
> 
>      def __str__(self):
> -        return "%s %s %s %s %s %s %s %s\n" % \
> +        return "%s %s %s %s %s %s %s %s %s\n" % \
>                  (self.board, self.testname, self.spec, self.build_number,
>                    self.timestamp, self.kernel, self.tguid, self.ref,
> -                  self.result)
> +                  self.result, self.op)
> 
>  def get_chart_config(TESTDIR, chart_config_filename):
>      try:
> @@ -146,10 +148,12 @@ def reread_run_json_files(TESTDIR):
>      #  and possibly FUEGO_CRITERIA_JSON_PATH)
>      criteria_data = load_criteria()
>      ref_map = {}
> +    op_map = {}
>      for crit in criteria_data["criteria"]:
>          # not all criteria are measure reference thresholds
>          try:
>              ref_map[crit["tguid"]] = crit["reference"]["value"]
> +            op_map[crit["tguid"]] = crit["reference"]["operator"]
>          except:
>              # ignore other data
>              pass
> @@ -162,7 +166,7 @@ def reread_run_json_files(TESTDIR):
>              # read run_data
>              with open(run_filename) as run_file:
>                  run_data = json.load(run_file,
> object_pairs_hook=collections.OrderedDict)
> -            store_flat_results(test_logdir, run_data, ref_map)
> +            store_flat_results(test_logdir, run_data, ref_map, op_map)
> 
>  # Store tguid results in a flat list, space-separated
>  # this is stored in a file called flat_plot_data.txt in the test directory
> @@ -173,7 +177,7 @@ def reread_run_json_files(TESTDIR):
>  #
>  # returns the list of lines in the updated flat results file
> 
> -def store_flat_results(test_logdir, run_data, ref_map):
> +def store_flat_results(test_logdir, run_data, ref_map, op_map):
>      # test log dir is up one directory
> 
>      plot_data_filename=test_logdir+"/flat_plot_data.txt"
> @@ -212,7 +216,7 @@ def store_flat_results(test_logdir, run_data,
> ref_map):
>          return
> 
>      dprint("ref_map=%s" % ref_map)
> -
> +    dprint("op_map=%s" % op_map)
>      for test_set in test_sets:
>          ts_name = test_set["name"]
>          test_cases = test_set["test_cases"]
> @@ -245,11 +249,13 @@ def store_flat_results(test_logdir, run_data,
> ref_map):
>                      except:
>                          try:
>                              m_ref = ref_map[tguid]
> +                            m_op = op_map[tguid]
>                          except:
>                              m_ref = 0
> -                    line = "%s %s %s %s %s %s %s %s %s\n" % \
> +                            m_op = "none"
> +                    line = "%s %s %s %s %s %s %s %s %s %s\n" % \

OK - this is a lot of extra work and overhead (including the op for every
criteria reference, for every measure of every benchmark), to support
just a single operator ('bt') which might not be used that much.

It appears to me that this is only used one place in the make_measure_plot
code (the 'if entry.op == '"bt"').  Could you detect this automatically if
the ref value had a ',' in it?  This would save having to extend the flat_plot_data
by an extra field, which is desirable.

>                          (board, test_name, test_spec, build_number, timestamp,
> -                            kernel, tguid, m_ref, m_result)
> +                            kernel, tguid, m_ref, m_result, m_op)
>                      plot_file.write(line)
>                      data_lines.append(line)
> 
> @@ -319,20 +325,49 @@ def make_measure_plots(test_name,
> chart_config, entries):
> 
>              ref_series_key = series_key + "-ref"
> 
> -            dprint("ref_series_key=%s" % ref_series_key)
> -
> -            if ref_series_key not in series_map:
> -                dprint("making a new series for '%s'" % ref_series_key)
> -                series_map[ref_series_key] = { "label": ref_series_key,
> -                    "data": [],
> -                    "points": {"symbol": "cross"} }
> -            # add a plot point for this ref entry
> -            try:
> -                value = float(entry.ref)
> -            except:
> -                value = 0
> -            point = [entry.build_number, value]
> -            series_map[ref_series_key]["data"].append(point)
> +            if entry.op == "bt":
Maybe detect this case with 'if ',' in entry.ref:' instead of adding
a whole field to flat_plot_data.txt

> +                ref_min_series_key = ref_series_key + "-min"
> +                dprint("ref_min_series_key=%s" % ref_min_series_key)
> +                if ref_min_series_key not in series_map:
> +                    # add a plot point for this ref min entry
> +                    dprint("making a new series for '%s'" % ref_min_series_key)
> +                    series_map[ref_min_series_key] = { "label": ref_min_series_key,
> +                        "data": [],
> +                        "points": {"symbol": "cross"} }
> +                try:
> +                    min_value = float(entry.ref.split('-')[0])
This would require changing also.

> +                except:
> +                    min_value = 0
> +                point = [entry.build_number, min_value]
> +                series_map[ref_min_series_key]["data"].append(point)
> +                try:
> +                    max_value = float(entry.ref.split('-')[1])
This would require changing also.

> +                except:
> +                    max_value = None
> +                ref_max_series_key = ref_series_key + "-max"
> +                dprint("ref_max_series_key=%s" % ref_max_series_key)
> +                if ref_max_series_key not in series_map:
> +                    # add a plot point for this ref max entry
> +                    dprint("making a new series for '%s'" % ref_max_series_key)
> +                    series_map[ref_max_series_key] = { "label":
> ref_max_series_key,
> +                        "data": [],
> +                        "points": {"symbol": "cross"} }
> +                point = [entry.build_number, max_value]
> +                series_map[ref_max_series_key]["data"].append(point)
> +            else:
> +                dprint("ref_series_key=%s" % ref_series_key)
> +                if ref_series_key not in series_map:
> +                    # add a plot point for this ref entry
> +                    dprint("making a new series for '%s'" % ref_series_key)
> +                    series_map[ref_series_key] = { "label": ref_series_key,
> +                        "data": [],
> +                        "points": {"symbol": "cross"} }
> +                try:
> +                    value = float(entry.ref)
> +                except:
> +                    value = 0
> +                point = [entry.build_number, value]
> +                series_map[ref_series_key]["data"].append(point)
> 
>          flot_data = series_map.values()
>          flot_data.sort(key=itemgetter('label'))
> --
> 2.7.4

OK - this is horrible of me, but I'm short on time.  I applied the patch, and
changed the delimiter to ','.  But I haven't had time to test it.

Can you please consider the change I recommend above, as well as
test this code, which I just pushed to master, to see if anything broke.

I also did a refactoring patch on top of this, for check_measure, to 
make it a bit simpler.

Please take a look.

Thanks,
 -- Tim



More information about the Fuego mailing list