<div dir="ltr">Comments on the main patch inline, haven't looked at the tests yet:<div><br></div><div>--</div><div><br></div><div><div>> From 08612457b3c39c29eba5aa8ada79aeeb1e1d21cd Mon Sep 17 00:00:00 2001</div><div>
> From: Chris Matthews <<a href="mailto:cmatthews5@apple.com">cmatthews5@apple.com</a>></div><div>> Date: Mon, 21 Jul 2014 14:53:23 -0700</div><div>> Subject: [PATCH] Add rerun flag: when passed, rerun benchmarks which the</div>
<div>>  server says changed</div><div>> </div><div>> ---</div><div>>  lnt/tests/nt.py | 319 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-</div><div>>  1 file changed, 315 insertions(+), 4 deletions(-)</div>
<div>> </div><div>> diff --git a/lnt/tests/nt.py b/lnt/tests/nt.py</div><div>> index 7e292bc..4e34fb6 100644</div><div>> --- a/lnt/tests/nt.py</div><div>> +++ b/lnt/tests/nt.py</div><div>> @@ -7,18 +7,27 @@ import subprocess</div>
<div>>  import sys</div><div>>  import time</div><div>>  import traceback</div><div>> -</div><div>>  from datetime import datetime</div><div>> +from optparse import OptionParser, OptionGroup</div><div>> +</div>
<div>>  </div><div>>  import lnt.testing</div><div>>  import lnt.testing.util.compilers</div><div>>  import lnt.util.ImportData as ImportData</div><div>>  </div><div>> -from lnt.testing.util.commands import note, warning, error, fatal, resolve_command_path</div>
<div>> +from lnt.testing.util.commands import note, warning, fatal</div><div>>  from lnt.testing.util.commands import capture, mkdir_p, which</div><div>> +from lnt.testing.util.commands import resolve_command_path</div>
<div>> +</div><div>>  from lnt.testing.util.rcs import get_source_version</div><div>> +</div><div>>  from lnt.testing.util.misc import timestamp</div><div>>  </div><div>> +from lnt.server.reporting.analysis import UNCHANGED_PASS, UNCHANGED_FAIL</div>
<div>> +from lnt.server.reporting.analysis import REGRESSED, IMPROVED</div><div>> +from lnt.util import ImportData</div><div>> +import builtintest</div><div>> +</div><div>>  ###</div><div>>  </div><div>>  class TestModule(object):</div>
<div>> @@ -1033,5 +1042,61 @@ def run_test(nick_prefix, iteration, config):</div><div>>  </div><div>>  ###</div><div>>  </div><div>> -import builtintest</div><div>> -from optparse import OptionParser, OptionGroup</div>
<div>> +def _construct_report_path(basedir, only_test, test_style, file_type="csv"):</div><div>> +    """Get the full path to report files in the sandbox.</div><div>> +    """</div>
<div>> +    report_path = os.path.join(basedir)</div><div>> +    if only_test is not None:</div><div>> +        report_path =  os.path.join(report_path, only_test)</div><div>> +    report_path = os.path.join(report_path, ('report.%s.' % test_style) + file_type)</div>
<div>> +    return report_path</div><div>> +</div><div>> +</div><div>> +def rerun_test(config, name, num_times):</div><div>> +    """Take the test at name, and rerun it num_times with the previous settings</div>
<div>> +    stored in config.</div><div>> +</div><div>> +    """</div><div>> +    # Extend the old log file.</div><div>> +    logfile = open(config.test_log_path(None), 'a')</div><div>
> +</div><div>> +    relative_test_path = os.path.dirname(name)</div><div>> +    test_name = os.path.basename(name)</div><div>> +</div><div>> +    single_test_report = _construct_report_path(config.report_dir,</div>
<div>> +                                               config.only_test,</div><div>> +                                               config.test_style)</div><div>> +</div><div>> +    assert os.path.exists(single_test_report), "Previous report not there?" + \</div>
<div>> +        single_test_report</div><div>> +</div><div>> +    test_full_path = os.path.join(</div><div>> +        config.report_dir, relative_test_path)</div><div>> +</div><div>> +    assert os.path.exists(test_full_path), "Previous test directory not there?" + \</div>
<div>> +        test_full_path</div><div>> +</div><div>> +    results = []</div><div>> +    for i in xrange(0, num_times):</div><div>> +        _prepare_testsuite_for_rerun(test_name, test_full_path, config)</div>
<div>> +        result_path = _execute_test_again(config,</div><div>> +                                          test_name,</div><div>> +                                          test_full_path,</div><div>> +                                          logfile)</div>
<div>> +        results.extend(load_nt_report_file(result_path, config))</div><div>> +</div><div>> +    # Check we got an exec and status from each run.</div><div>> +    assert len(results) >= num_times, "Did not get all the runs?"</div>
<div>> +</div><div>> +    logfile.close()</div><div>> +    return results</div><div>> +</div><div>> +</div><div>> +def _prepare_testsuite_for_rerun(test_name, test_full_path, config):</div><div>> +    """Rerun  step 1: wipe out old files. To prepare the test suite,</div>
<div>> +    we must delete the correct files to convince make to rerun</div><div>> +    just this test. For executoin time runs, these are the</div><div>> +    previous execution time runs, then any reports that would hav</div>
<div>> +    been created by them.</div><div>> +</div><div>> +    """</div><div><br></div><div>Sic (execution, have)</div><div><br></div><div>> +    output = os.path.join(test_full_path, "Output/")</div>
<div>> +    # The execution timing file.</div><div>> +</div><div>> +    try:</div><div>> +        os.remove(output + test_name + ".out-" + config.test_style)</div><div>> +    except OSError:</div>
<div>> +        # If that does not work, try the mangled name.</div><div>> +        os.remove(output + test_name.replace("_", ".") + ".out-" + config.test_style)</div><div><br></div><div>
I don't like the ad hoc handling of mangling here, and this won't properly work</div><div>with tests that had a mix of actual '_' and '.' characters. I think we should</div><div>find a principled way to solve this, one way would be to have the initial run</div>
<div>leave behind a mapping of original names to mangled names that could be inverted</div><div>on demand.</div><div><br></div><div>> +</div><div>> +    # Remove the report files.</div><div>> +    os.remove(_construct_report_path(config.report_dir,</div>
<div>> +                                    config.only_test,</div><div>> +                                    config.test_style))</div><div>> +</div><div>> +    os.remove(_construct_report_path(config.report_dir,</div>
<div>> +                                    config.only_test,</div><div>> +                                    config.test_style,</div><div>> +                                    "txt"))</div><div>> +</div>
<div>> +    os.remove(_construct_report_path(config.report_dir,</div><div>> +                                    config.only_test,</div><div>> +                                    config.test_style,</div><div>> +                                    "raw.out"))</div>
<div><br></div><div>I'm tempted to make this code more resilient and simpler by just removing the</div><div>entire contents of the Output directory. Do you see any good reason not to do</div><div>that? If we don't do that, I'd like to remove *.$(TEST).* from in the test dir.</div>
<div><br></div><div>> +</div><div>> +</div><div>> +def _execute_test_again(config, test_name, test_path, logfile):</div><div>> +    """Rerun step 2: rerun the execution benchmark of interest.  Now that the</div>
<div>> +    test suite is ready, rerun the make command to gather new</div><div>> +    data.  Modify the old make arguments to just be the execution</div><div>> +    target and all the reports that are dependent on that target.</div>
<div>> +</div><div>> +    """</div><div>> +    # Grab old make invocation.</div><div>> +    mk_vars, _ = config.compute_run_make_variables()</div><div>> +    to_exec = list(mk_vars)</div><div>
> +    to_exec = ['make','-k'] + to_exec</div><div>> +    to_exec.extend('%s=%s' % (k,v) for k,v in mk_vars.items())</div><div>> +    if config.only_test is not None:</div><div>> +        to_exec.extend(['-C', config.only_test])</div>
<div>> +</div><div>> +    to_exec.append("Output/" + test_name + ".out-" + config.test_style)</div><div>> +    to_exec.append("Output/" + test_name + ".diff-" + config.test_style)</div>
<div>> +    to_exec.append("Output/" + test_name + ".exec-" + config.test_style)</div><div>> +    to_exec.append("Output/" + test_name + ".exec.report." + config.test_style)</div>
<div>> +    to_exec.append("Output/" + test_name + "." + config.test_style + ".report.txt")</div><div>> +    to_exec.append("report." + config.test_style + ".csv")</div>
<div>> +    to_exec.append("report")</div><div><br></div><div>You don't need to provide all of these targets. The "master" target for driving</div><div>each test is $(TEST-NAME).$(TEST).report.txt, and we should only build that</div>
<div>target. If you build "report" or "report.$(TEST).csv", you may end up rerunning</div><div>many other tests (in directories that have multiple tests, or that have</div><div>subdirectories).</div><div>
<br></div><div>> +</div><div>> +    result_loc = _construct_report_path(config.report_dir,</div><div>> +                                       config.only_test,</div><div>> +                                       config.test_style)</div>
<div>> + </div><div><br></div><div>Instead of building the report.$(TEST).csv, I'd like to just build the one</div><div>individual report file, then manually call GenerateReport.pl on the one input</div><div>file we need so we can get an output we can pass to load_nt_report_file()</div>
<div><br></div><div>> +    assert not os.path.exists(result_loc)</div><div>> +    execute_command(logfile, config.build_dir(None), to_exec, test_path)</div><div>> +    assert os.path.exists(result_loc), "Missing {}".format(result_loc)</div>
<div>> +    return result_loc</div><div><br></div><div>I think a more sensible API is for this method to return the report data, not</div><div>the path to the report. Also, I would sink the call to</div><div>prepare_testsuite_for_rerun into this method.</div>
<div><br></div><div>> +</div><div>> +</div><div>> +# When set to true, all benchmarks will be rerun.</div><div>> +DEBUG_RERUN = False</div><div><br></div><div>What is this here for?</div><div><br></div><div>> +NUMBER_OF_RERUNS = 4</div>
<div>> +</div><div>> +SERVER_FAIL = u'FAIL'</div><div>> +SERVER_PASS = u'PASS'</div><div>> +</div><div>> +# Local test reults names have these suffexes</div><div>> +# Test will have the perf suffex if it passed</div>
<div>> +# if it failed it will have a status suffex.</div><div><br></div><div>Sic (suffix)</div><div><br></div><div>> +LOCAL_COMPILE_PERF = "compile"</div><div>> +LOCAL_COMPILE_STATUS = "compile.status"</div>
<div>> +LOCAL_EXEC_PERF = "exec"</div><div>> +LOCAL_EXEC_STATUS = "exec.status"</div><div>> +</div><div>> +# Server results have both status and performance in each entry</div><div>> +SERVER_COMPILE_RESULT = "compile_time"</div>
<div>> +SERVER_EXEC_RESULT = "execution_time"</div><div>> +</div><div>> +</div><div>> +class PastRunData:</div><div><br></div><div>This should inherit from object to be modern.</div><div><br></div><div>
> +    """To decide if we need to rerun, we must know</div><div>> +    what happened on each test in the first runs.</div><div>> +    Because the server retruns data in a different format than</div>
<div>> +    the local results, this class contains a per-test per-run</div><div>> +    aggregate of the two reports."""</div><div><br></div><div>Sic (reruns)</div><div><br></div><div>> +    def __init__(self, name):</div>
<div>> +        <a href="http://self.name">self.name</a> = name</div><div>> +        self.compile_status = None</div><div>> +        self.compile_time = None</div><div>> +        self.execution_status = None</div>
<div>> +        self.execution_time = None</div><div>> +        self.valid = False</div><div>> +</div><div>> +    def _check(self):</div><div>> +        """Make sure this run data is complete."""</div>
<div>> +        assert <a href="http://self.name">self.name</a> is not None</div><div>> +        msg = "Malformed test: %s" % (repr(self))</div><div>> +        assert self.compile_status is not None, msg</div>
<div>> +        assert self.execution_status is not None, msg</div><div>> +        assert self.compile_time is not None, msg</div><div>> +        assert self.execution_time is not None, msg</div><div>> +        self.valid = True</div>
<div>> +</div><div>> +    def _is_rerunable(self):</div><div>> +        """Decide if we should rerun this test. Rerun all execute tests when in</div><div>> +        debug, re-run if test compiled correctly, execuited correctly,</div>
<div>> +        but performance changed. Don't rerun if compile failed, or execute</div><div>> +        failed, sucessful execute did not change in performance (as decided by</div><div>> +        the server).</div>
<div>> +        """</div><div><br></div><div>Sic (executed). This doc comment just duplicates the comments in the code, I</div><div>would just make this describe the API (should we rerun this test) not the logic.</div>
<div><br></div><div>> +        assert self.valid</div><div>> +        # Don't rerun if compile failed.</div><div>> +        if self.compile_status == SERVER_FAIL:</div><div>> +            return False</div>
<div>> +</div><div>> +        # Rerun everything in debug.</div><div>> +        if DEBUG_RERUN:</div><div>> +            return True</div><div>> +</div><div>> +        # Don't rerun on correctness failure or test pass.</div>
<div>> +        if self.execution_status == UNCHANGED_FAIL or \</div><div>> +           self.execution_status == UNCHANGED_PASS or \</div><div>> +           self.execution_status == SERVER_FAIL:</div><div>> +            return False</div>
<div>> +</div><div>> +        # Do rerun on regression or improvement.</div><div>> +        if self.execution_status == REGRESSED or \</div><div>> +           self.execution_status == IMPROVED:</div><div>> +            return True</div>
<div>> +</div><div>> +        assert False, "Malformed run data: " \</div><div>> +            "you should not get here. " + str(self)</div><div>> +</div><div>> +    def __repr__(self):</div>
<div>> +        template = "<{}: CS {}, CT {}, ES {}, ET {}>"</div><div>> +        return template.format(<a href="http://self.name">self.name</a>,</div><div>> +                               repr(self.compile_status),</div>
<div>> +                               repr(self.compile_time),</div><div>> +                               repr(self.execution_status),</div><div>> +                               repr(self.execution_time))</div>
<div>> +</div><div>> +</div><div>> +def _process_reruns(config, server_reply, local_results):</div><div>> +    """Rerun each benchmark which the server reported "changed", N more</div>
<div>> +    times. By default N=4 to make 5 samples total. 5 samples is the</div><div>> +    smallest thing that has an okay median.  For now, only rerun the</div><div>> +    exec time tests, and don't rerun test which failed or whoes</div>
<div>> +    compile failed.</div><div>> +</div><div>> +    """</div><div><br></div><div>This comment again duplicates ones elsewhere, better to delegate.</div><div><br></div><div>> +    # To know which benchmarks to run, we need to check the local results</div>
<div>> +    # to see which did not fail, then check the server results to see</div><div>> +    # which regressed or improved. The server will return extra results</div><div>> +    # for things that we did not run this time, it may also omit performance</div>
<div>> +    # results.</div><div><br></div><div>More duplication of logic explanation.</div><div><br></div><div>> +    server_results = server_reply['test_results'][0]['results']</div><div>> +</div>
<div>> +    # Holds the combined local and server results.</div><div>> +    cololated_results = dict()</div><div><br></div><div>Sic (collated)</div><div><br></div><div>> +</div><div>> +    for b in local_results.tests:</div>
<div>> +        # format: suite.test/path/and/name.type<.type></div><div>> +        fields = b.name.split('.')</div><div>> +        test_name = fields[1]</div><div>> +        test_type = '.'.join(fields[2:])\</div>
<div>> +</div><div>> +        updating_entry = cololated_results.get(test_name,</div><div>> +                                               PastRunData(test_name))</div><div>> +        if test_type == LOCAL_COMPILE_PERF:</div>
<div>> +            updating_entry.compile_time = b.data</div><div>> +        elif test_type == LOCAL_COMPILE_STATUS:</div><div>> +            updating_entry.compile_status = SERVER_FAIL</div><div>> +        elif test_type == LOCAL_EXEC_PERF:</div>
<div>> +            updating_entry.execution_time = b.data</div><div>> +        elif test_type == LOCAL_EXEC_STATUS:</div><div>> +                updating_entry.execution_status = SERVER_FAIL</div><div>> +        else:</div>
<div>> +            assert False, "Unexpected local test type."</div><div>> +</div><div>> +        cololated_results[test_name] = updating_entry</div><div>> +</div><div>> +    # Now add on top the server results to any entry we already have.</div>
<div>> +    for full_name, results_status, perf_status in server_results:</div><div>> +        test_name, test_type = full_name.split(".")</div><div>> +</div><div>> +        new_entry = cololated_results.get(test_name,  None)</div>
<div>> +        # Some tests will come from the server, which we did not run locally.</div><div>> +        # Drop them.</div><div>> +        if new_entry is None:</div><div>> +            continue</div><div>> +        # Set these, if they were not set with fails above.</div>
<div>> +        if SERVER_COMPILE_RESULT in test_type:</div><div>> +            if new_entry.compile_status is None:</div><div>> +                new_entry.compile_status = results_status</div><div>> +        elif SERVER_EXEC_RESULT in test_type:</div>
<div>> +            if new_entry.execution_status is None:</div><div>> +                # If the server has not seen the test before, it will return</div><div>> +                # None for the performance results analysis. In this case we</div>
<div>> +                # will assume no rerun is needed, so assign unchanged.</div><div>> +                if perf_status is None:</div><div>> +                    derived_perf_status = UNCHANGED_PASS</div><div>
> +                else:</div><div>> +                    derived_perf_status = perf_status</div><div>> +                new_entry.execution_status = derived_perf_status</div><div>> +        else:</div><div>> +            assert False, "Unexpected server result type."</div>
<div>> +        cololated_results[test_name] = new_entry</div><div>> +</div><div>> +    # Double check that all values are there for all tests.</div><div>> +    for test in cololated_results.values():</div><div>
> +        test._check()</div><div>> +</div><div>> +    rerunable_benches = filter(lambda x: x._is_rerunable(),</div><div>> +                               cololated_results.values())</div><div>> +    rerunable_benches.sort(key=lambda x: <a href="http://x.name">x.name</a>)</div>
<div>> +    # Now lets actually do the reruns.</div><div>> +    rerun_results = []</div><div>> +    SUMMARY = "Rerunning {} of {} benchmarks."</div><div>> +    note(SUMMARY.format(len(rerunable_benches),</div>
<div>> +                        len(cololated_results.values())))</div><div>> +</div><div>> +    for i, bench in enumerate(rerunable_benches):</div><div>> +        note("Rerunning: {} [{}/{}]".format(<a href="http://bench.name">bench.name</a>,</div>
<div>> +                                            i + 1,</div><div>> +                                            len(rerunable_benches)))</div><div>> +        fresh_samples = rerun_test(config,</div><div>> +                                   <a href="http://bench.name">bench.name</a>,</div>
<div>> +                                   NUMBER_OF_RERUNS)</div><div>> +        rerun_results.extend(fresh_samples)</div><div>> +</div><div>> +    return rerun_results</div><div>> +</div><div><br></div><div>
This method seems more complicated than it needs to be. Can't we just scan the</div><div>server results for the list of tests we need to rerun, then filter out any that</div><div>we didn't run locally, then run those? It isn't clear to me we need to do the</div>
<div>collation and build up all the PastRunData objects.</div><div> </div><div>>  usage_info = """</div><div>>  Script for running the tests in LLVM's test-suite repository.</div><div>> @@ -1233,6 +1529,9 @@ class NTTest(builtintest.BuiltinTest):</div>
<div>>          group.add_option("", "--build-threads", dest="build_threads",</div><div>>                           help="Number of compilation threads",</div><div>>                           type=int, default=0, metavar="N")</div>
<div>> +        group.add_option("", "--rerun", dest="rerun",</div><div>> +                 help="Rerun tests that have regressed.",</div><div>> +                 action="store_true", default=False)</div>
<div>>  </div><div>>          group.add_option("", "--remote", dest="remote",</div><div>>                           help=("Execute remotely, see "</div><div>> @@ -1495,3 +1794,13 @@ class NTTest(builtintest.BuiltinTest):</div>
<div>>  </div><div>>          else:</div><div>>              test_results = run_test(nick, None, config)</div><div>> +            if opts.rerun:</div><div>> +                self.log("Performing any needed reruns.")</div>
<div>> +                server_report = self.submit_helper(config, commit=False)</div><div>> +                new_samples = _process_reruns(config, server_report, test_results)</div><div>> +                test_results.update_report(new_samples)</div>
<div>> +</div><div>> +                # persist report with new samples.</div><div>> +                lnt_report_path = config.report_path(None)</div><div>> +                os.unlink(lnt_report_path)</div><div>
> +                lnt_report_file = open(lnt_report_path, 'w')</div><div><br></div><div>Why bother to unlink here? </div><div><br></div><div>> +                print >>lnt_report_file, test_results.render()</div>
<div>> +                lnt_report_file.close()</div><div>>  </div><div>>              if config.output is not None:</div><div>>                  self.print_report(test_results, config.output)</div><div>> -- </div>
<div>> 1.9.3 (Apple Git-50)+GitX</div></div><div><br></div><div>--</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 21, 2014 at 3:32 PM, Chris Matthews <span dir="ltr"><<a href="mailto:chris.matthews@apple.com" target="_blank">chris.matthews@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Daniel,<br>
<br>
Here is an update to the rerun patch discussed last year. Attached is a patch for the NT test suite as well as a new rerun test in the LNT test suite.<br>
<br>
The patch does not perform well (reruns take quite a while). I’d like to address that in a separate patch.<br>
<br>
</blockquote></div><br></div>