Roll dosep.py parallel test runner into dotest.py command line

[tfiala]

Summary

This is a change discussed in http://reviews.llvm.org/D12416.

This change does the following:

• Removes the need to call dosep.py directly. Indeed, calling dosep.py directly now errors out (intentionally).
• Moves the arguments that dosep.py used to take into a Parallel execution options group of the dotest.py options.
• dotest.py now takes an optional --no-multiprocess option to shut off the multiprocess test runner. If not specified (i.e. the default), the test runner runs it in the same manner as dosep.py would have run it.
• The unnamed option (trailing argument) that dosep.py took was a test-dir-relative subdir to limit multiprocess test traversal within. This has been moved to the dotest.py --test-subdir option since dotest.py already handled unnamed options for something else.
• cmake and configure-based builds have been updated to use dotest.py or dotest.py --no-multiprocess in place of calling dosep.py.

I intend to do further cleanup here as follow up work, but I wanted to get the outside experience modified to "multiprocess by default, one interface" to the outside user.

[tfiala]

Removes the need to call dosep.py directly. Indeed, calling dosep.py now errors out (intentionally).

Calling it *directly* errors out. dotest.py still uses it under the covers at the moment, which is why I haven't removed it.

[zturner]

Are there any plans to actually just kill dosep.py at some point? I know you mentioned you want to do further cleanup, so I'm wondering if this is just an incremental step towards a dosep'less world.

I'll review this in more detail tomorrow, but please hold off on comitting until I get a chance to test everything on Windows and make sure everything still works. (I assume other people will want to do the same for their platforms)

[tfiala]

Yep, all good. I was planning on waiting for feedback first.

I've checked on Linux but don't have a Windows box available at the moment.

[labath]

I very much like the direction this is going in. We will need to update our buildbots when this lands (we need to run dosep manually to setup remote testing), but I have tested this manually and didn't see any problems.

test/dosep.py

503

You should print a \n after this message.

[tfiala]

In D12587#239054, @labath wrote:

I very much like the direction this is going in. We will need to update our buildbots when this lands (we need to run dosep manually to setup remote testing), but I have tested this manually and didn't see any problems.

Good catch, indeed!

[tfiala]

test/dosep.py

503

Yes, good catch.

[tfiala]

Added newline to dosep.py error sent via sys.stderr.write() when called directly.

[tfiala]

Also, I should mention this was tested on OS X in addition to Linux.

Let me know when you get a chance to run it on Windows, Zachary.

Are there any plans to actually just kill dosep.py at some point?

Hmm not seeing my response to this from my phone last night, so at the cost of potentially repeating:
Absolutely. I think there is a fair amount of cleanup that could happen to the test running code. I'd expect later to see the dosep.py functionality turn into an LLDBTestRunner class or something similar, with a concrete impl for the single-serialized running and a separate one for multithreaded running. There are many areas that could be cleaned up (both in terms of code organization and things like pep8/pylint --- a few of which I took care of in this change but the majority of which I left unchanged).

My goal for this particular change was to be minimally different at the implementation level to simplify the transition --- get the user interface right (no dosep.py from the user's point of view). In using a minimal implementation change, I'm also hopeful to avoid things like breaking the Windows build.

[tfiala]

[clayborg]

Looks good.

[zturner]

About to take a look at this

[tfiala]

In D12587#239410, @zturner wrote:

About to take a look at this

Great, thanks.

[zturner]

test/CMakeLists.txt

65

Is this necessary? dosep already takes a -t argument which specifies the number of threads, and if the number of threads is 1 it disables the thread pool. Is it possible to delete this option and just pass -t=1 instead?

[tfiala]

test/CMakeLists.txt

65

Right now there's an implementation difference between the two:

--no-multiprocess literally switches from using the dosep parallel test runner logic to using the old dotest.py test runner logic. Specifying -t/--threads THREADCOUNT where THREADCOUNT is 1 will use the multiprocess test runner with a single process queue. So, if somebody wanted to have the extra test subdir filtering of the dosep logic (the test-subdir arg) but for some reason wanted to debug or otherwise run it serially, they'd be out of luck if we adopted a -t=1 == --no-multiprocess stance.

What I'd recommend is we keep the two options for now since there is behavior we would otherwise lose at the moment. But, when we do the refactor to test runners, we get rid of all the bits that are special to the multi test runner (e.g. the test-subdir and the --output-on-success flags), at which point your idea that "--no-multiprocess is unnecessary" should be true, and we can remove it then.

Sound reasonable?

[zturner]

Ok, seems fine. No other issues on my side

[tfiala]

In D12587#239494, @zturner wrote:

Ok, seems fine. No other issues on my side

Great, thanks. I'm going to get this in.

[tfiala]

Submitted here:

$ svn commit
Sending        test/CMakeLists.txt
Sending        test/Makefile
Sending        test/dosep.py
Sending        test/dotest.py
Sending        test/dotest_args.py
Sending        www/test.html
Transmitting file data ......
Committed revision 246794.

[chying]

Sorry, coming late to this thread.
This change works ok on linux and darwin systems.

But it has some problem on Windows, the main code has to be protected with the following statement on Windows,
if name == 'main':
Otherwise it creates subprocesses recursively.

[zturner]

I didn't see this when I ran ninja check-lldb, but Adrian is seeing the
same thing on his machine for some reason.

[zturner]

I actually am seeing this now, I'm not sure why I didn't see it earlier,
the only thing I can think of is that the patch didn't apply successfully
even though I thoguht it did.

When I stop at this line:

if num_threads > 1:
    pool = multiprocessing.Pool(
        num_threads,
        initializer=setup_global_variables,
        initargs=(output_lock, test_counter, total_tests, test_name_len,
                  dotest_options))

in a debugger and inspect the value of dotest_options,
dotest_options.no_multiprocessing is set to False. Is this correct? It
seems like for the child dotest instances, it shouldn't be trying to
recursively do this multiprocessing?

[tfiala]

In D12587#239625, @chying wrote:

Sorry, coming late to this thread.
This change works ok on linux and darwin systems.

Hi @chying,

But it has some problem on Windows, the main code has to be protected with the following statement on Windows,
if name == 'main':
Otherwise it creates subprocesses recursively.

Can you tell me more about this? How are you calling it? Which one (dotest.py or dosep.py) is going recursively out of control? On what kind of machine? Did you used to run it with dosep,py or dotest.py? If you used to use dotest.py, you'll start seeing a bunch of dotest.py test runners (one per core, which you can control with the -t or --threads option). If you're on a machine that has 48 cores, you'll get 48 dotest runner processes. Are you sure that it is not running?

If it is not running and is truly recursing out of control, can you capture the command line args for several of the processes? All of the inferior dotest.py worker bees should have been kicked off with the --inferior flag added. If *not*, then something sounds like it is going wrong.

Do you have a patch that fixes this? (I'm not sure exactly which part you're wanting to protect).

One more question - I recall sometimes having .pyc files not get cleared out properly, and get used even when they're stale on some platforms in the past. Is it possible you have stale .pyc files that possibly don't have the updated code (i.e. .py file is fresh, but .pyc file is getting used accidentally)? Easiest way to rule that out is to blow away the .pyc files in the directory. Having stale files in this case could go really badly.

Thanks!

[tfiala]

In D12587#239639, @zturner wrote:

I actually am seeing this now, I'm not sure why I didn't see it earlier,
the only thing I can think of is that the patch didn't apply successfully
even though I thoguht it did.

When I stop at this line:

if num_threads > 1:
    pool = multiprocessing.Pool(
        num_threads,
        initializer=setup_global_variables,
        initargs=(output_lock, test_counter, total_tests, test_name_len,
                  dotest_options))

in a debugger and inspect the value of dotest_options,
dotest_options.no_multiprocessing is set to False. Is this correct? It
seems like for the child dotest instances, it shouldn't be trying to
recursively do this multiprocessing?

That flag is only meaningful to the initial invocation. It would be False if the user didn't specify it on the command line in the highest-level process that is driving the parallel test runner. That looks fine.

The children know their inferiors by an --inferior flag, and don't try to recurse if either the no_multiprocess or the --inferior flags are set. The parallel test runner arranges to always pass in the --inferior flag.

[zturner]

I'm testing a fix for this locally. Hold tight

[zturner]

http://reviews.llvm.org/D12612

[chying]

Sorry, coming late to this thread.
This change works ok on linux and darwin systems.

But it has some problem on Windows, the main code has to be protected with the following statement on Windows,
if name == 'main':
Otherwise it creates subprocesses recursively.

In D12587#239644, @tfiala wrote:

In D12587#239625, @chying wrote:

Sorry, coming late to this thread.
This change works ok on linux and darwin systems.

Hi @chying,

But it has some problem on Windows, the main code has to be protected with the following statement on Windows,
if name == 'main':
Otherwise it creates subprocesses recursively.

Can you tell me more about this? How are you calling it? Which one (dotest.py or dosep.py) is going recursively out of control? On what kind of machine? Did you used to run it with dosep,py or dotest.py? If you used to use dotest.py, you'll start seeing a bunch of dotest.py test runners (one per core, which you can control with the -t or --threads option). If you're on a machine that has 48 cores, you'll get 48 dotest runner processes. Are you sure that it is not running?

I call dotest directly, like "python.exe dotest.py --executable ..." on a Windows 7 machine.
I set LLDB_TEST_THREADS=8, and see 8 test runners from task manager. That has no problem.
But it seems each thread keeps calling dotest->dosep->dotest-... recursively.

If it is not running and is truly recursing out of control, can you capture the command line args for several of the processes? All of the inferior dotest.py worker bees should have been kicked off with the --inferior flag added. If *not*, then something sounds like it is going wrong.

Do you have a patch that fixes this? (I'm not sure exactly which part you're wanting to protect).

I don't have a patch yet, because the functions in dotest.py is pretty scattered.
I'm not sure which part to guard yet. I'm still looking at it.

One more question - I recall sometimes having .pyc files not get cleared out properly, and get used even when they're stale on some platforms in the past. Is it possible you have stale .pyc files that possibly don't have the updated code (i.e. .py file is fresh, but .pyc file is getting used accidentally)? Easiest way to rule that out is to blow away the .pyc files in the directory. Having stale files in this case could go really badly.

Yes, I had a clean run.

[zturner]

Hi Ying, check the patch I just posted, I think it fixes it.

[tfiala]

In D12587#239681, @chying wrote:

Yes, I had a clean run.

Thanks for all the details, @chying! It looks like Zachary found the culprit. I'm looking forward to hear if his patch fixes it for you.

test/CMakeLists.txt

 
 add_python_test_target(check-lldb-single
   ${LLDB_SOURCE_DIR}/test/dotest.py
-  "${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}"
+  "--no-multiprocess;${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}"
   "Testing LLDB with args: ${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}"
   )
 
-set(LLDB_DOSEP_ARGS -o;\"-q;${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}\")
+set(LLDB_DOTEST_ARGS -q;${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS})
 
 # If tests crash cause LLDB to crash, or things are otherwise unstable, or if machine-parsable
 # output is desired (i.e. in continuous integration contexts) check-lldb-single is a better target.
 add_python_test_target(check-lldb
-  ${LLDB_SOURCE_DIR}/test/dosep.py
-  "${LLDB_DOSEP_ARGS}"
-  "Testing LLDB (with a separate subprocess per test)"
+  ${LLDB_SOURCE_DIR}/test/dotest.py
+  "${LLDB_DOTEST_ARGS}"
+  "Testing LLDB (parallel execution, with a separate subprocess per test)"
   )

test/Makefile

 #----------------------------------------------------------------------
 check-local::
   rm -rf lldb-test-traces
-  python $(PROJ_SRC_DIR)/dosep.py -o "--executable $(ToolDir)/lldb -q -s lldb-test-traces -u CXXFLAGS -u CFLAGS -C $(subst ccache,,$(CC))"
+  python $(PROJ_SRC_DIR)/dotest.py --executable $(ToolDir)/lldb -q -s lldb-test-traces -u CXXFLAGS -u CFLAGS -C $(subst ccache,,$(CC))

test/dosep.py

 import platform
 import re
 import dotest_args
-import shlex
 import subprocess
 import sys
 

                                 result, re.MULTILINE)
         unexpected_success_count = re.search("^RESULT:.*([0-9]+) unexpected successes",
                                              result, re.MULTILINE)
-        this_fail_count = 0
-        this_error_count = 0
         if pass_count is not None:
             passes = passes + int(pass_count.group(1))
         if fail_count is not None:

         script_file = os.path.join(test_root, "dotest.py")
         command = ([sys.executable, script_file] +
                    dotest_argv +
-                   ["-p", name, root])
+                   ["--inferior", "-p", name, root])
 
         timeout_name = os.path.basename(os.path.splitext(name)[0]).upper()
 

               if status != ePassed]
     unexpected_passes = [name for name, _, _, _, unexpected_successes in results
                          if unexpected_successes > 0]
-    
     pass_count = sum([result[2] for result in results])
     fail_count = sum([result[3] for result in results])
 

     else:
         m = re.search('remote-(\w+)', platform_name)
         target = m.group(1)
-        remote = True
 
     expected_timeout = set()
 

     return result
 
 
-def main():
+def main(print_details_on_success, num_threads, test_subdir):
+    """Run dotest.py in inferior mode in parallel.
+
+    @param print_details_on_success the parsed value of the output-on-success
+    command line argument.  When True, details of a successful dotest inferior
+    are printed even when everything succeeds.  The normal behavior is to
+    not print any details when all the inferior tests pass.
+
+    @param num_threads the parsed value of the num-threads command line
+    argument.
+
+    @param test_subdir optionally specifies a subdir to limit testing
+    within.  May be None if the entire test tree is to be used.  This subdir
+    is assumed to be relative to the lldb/test root of the test hierarchy.
+    """
+
+    dotest_argv = sys.argv[1:]
+
+    global output_on_success
+    output_on_success = print_details_on_success
+
     # We can't use sys.path[0] to determine the script directory
     # because it doesn't work under a debugger
     test_directory = os.path.dirname(os.path.realpath(__file__))

        E.g., export LLDB_TEST_TIMEOUT=0
        or    export LLDB_TESTCONCURRENTEVENTS_TIMEOUT=0
 """)
-    parser.add_option(
-        '-o', '--options',
-        type='string', action='store',
-        dest='dotest_options',
-        help="""The options passed to 'dotest.py' if specified.""")
-
-    parser.add_option(
-        '-s', '--output-on-success',
-        action='store_true',
-        dest='output_on_success',
-        default=False,
-        help="""Print full output of 'dotest.py' even when it succeeds.""")
-
-    parser.add_option(
-        '-t', '--threads',
-        type='int',
-        dest='num_threads',
-        help="""The number of threads to use when running tests separately.""")
-
-    opts, args = parser.parse_args()
-    dotest_option_string = opts.dotest_options
-
-    is_posix = (os.name == "posix")
-    dotest_argv = (shlex.split(dotest_option_string, posix=is_posix)
-                   if dotest_option_string
-                   else [])
-
     parser = dotest_args.create_parser()
     global dotest_options
-    global output_on_success
-    output_on_success = opts.output_on_success
     dotest_options = dotest_args.parse_args(parser, dotest_argv)
 
     if not dotest_options.s:

     session_dir = os.path.join(os.getcwd(), dotest_options.s)
 
     # The root directory was specified on the command line
-    if len(args) == 0:
-        test_subdir = test_directory
+    if test_subdir and len(test_subdir) > 0:
+        test_subdir = os.path.join(test_directory, test_subdir)
     else:
-        test_subdir = os.path.join(test_directory, args[0])
+        test_subdir = test_directory
 
     # clean core files in test tree from previous runs (Linux)
     cores = find('core.*', test_subdir)
     for core in cores:
         os.unlink(core)
 
-    if opts.num_threads:
-        num_threads = opts.num_threads
-    else:
+    if not num_threads:
         num_threads_str = os.environ.get("LLDB_TEST_THREADS")
         if num_threads_str:
             num_threads = int(num_threads_str)

     sys.exit(exit_code)
 
 if __name__ == '__main__':
-    main()
+    sys.stderr.write(
+        "error: dosep.py no longer supports being called directly. "
+        "Please call dotest.py directly.  The dosep.py-specific arguments "
+        "have been added under the Parallel processing arguments.\n")
+    sys.exit(128)

test/dotest.py

 import signal
 import subprocess
 import sys
-import textwrap
-import time
 import inspect
 import unittest2
 import lldbtest_config

 lldb_platform_url = None
 lldb_platform_working_dir = None
 
+# Parallel execution settings
+is_inferior_test_runner = False
+multiprocess_test_subdir = None
+num_threads = None
+output_on_success = False
+no_multiprocess_test_runner = False
+
 def usage(parser):
     parser.print_help()
     if verbose > 0:

     global lldb_platform_url
     global lldb_platform_working_dir
     global setCrashInfoHook
+    global is_inferior_test_runner
+    global multiprocess_test_subdir
+    global num_threads
+    global output_on_success
+    global no_multiprocess_test_runner
 
     do_help = False
 

 
     parser = dotest_args.create_parser()
     args = dotest_args.parse_args(parser, sys.argv[1:])
-    
     if args.unset_env_varnames:
         for env_var in args.unset_env_varnames:
             if env_var in os.environ:

 
     if args.d:
         sys.stdout.write("Suspending the process %d to wait for debugger to attach...\n" % os.getpid())
-        sys.stdout.flush()    
+        sys.stdout.flush()
         os.kill(os.getpid(), signal.SIGSTOP)
 
     if args.e:

     if dont_do_lldbmi_test and just_do_lldbmi_test:
         usage(parser)
 
+    if args.no_multiprocess:
+        no_multiprocess_test_runner = True
+
+    if args.inferior:
+        is_inferior_test_runner = True
+
+    if args.output_on_success:
+        output_on_success = True
+
+    if args.num_threads:
+        num_threads = args.num_threads
+
+    if args.test_subdir:
+        multiprocess_test_subdir = args.test_subdir
+
     if args.lldb_platform_name:
         lldb_platform_name = args.lldb_platform_name
     if args.lldb_platform_url:

     if exitCode:
         sys.exit(exitCode)
 
+
+def isMultiprocessTestRunner():
+    # We're not multiprocess when we're either explicitly
+    # the inferior (as specified by the multiprocess test
+    # runner) OR we've been told to skip using the multiprocess
+    # test runner
+    return not (is_inferior_test_runner or no_multiprocess_test_runner)
+
 # On MacOS X, check to make sure that domain for com.apple.DebugSymbols defaults
 # does not exist before proceeding to running the test suite.
 if sys.platform.startswith("darwin"):

 # then, we walk the directory trees and collect the tests into our test suite.
 #
 parseOptionsAndInitTestdirs()
+
+# If we are running as the multiprocess test runner, kick off the
+# multiprocess test runner here.
+if isMultiprocessTestRunner():
+    import dosep
+    dosep.main(output_on_success, num_threads, multiprocess_test_subdir)
+    raise "should never get here"
+
 setupSysPath()
 setupCrashInfoHook()
 

test/dotest_args.py

     group.set_defaults(disable_crash_dialog=True)
     group.set_defaults(hide_inferior_console=True)
 
+    group = parser.add_argument_group('Parallel execution options')
+    group.add_argument(
+        '--inferior',
+        action='store_true',
+        help=('specify this invocation is a multiprocess inferior, '
+              'used internally'))
+    group.add_argument(
+        '--no-multiprocess',
+        action='store_true',
+        help='skip running the multiprocess test runner')
+    group.add_argument(
+        '--output-on-success',
+        action='store_true',
+        help=('print full output of the dotest.py inferior, '
+              'even when all tests succeed'))
+    group.add_argument(
+        '--threads',
+        type=int,
+        dest='num_threads',
+        help=('The number of threads/processes to use when running tests '
+              'separately, defaults to the number of CPU cores available'))
+    parser.add_argument(
+        '--test-subdir',
+        action='store',
+        help='Specify a test subdirectory to use relative to the test root dir'
+    )
+
     # Remove the reference to our helper function
     del X
 

www/test.html

             </code>
 
             <p>
-              Besides <code>dotest.py</code>, there is also <code>dosep.py</code>, which runs
-              multiple instances of <code>dotest.py</code> in parallel, thereby greatly
-              decreasing the time it takes to run the full testsuite. The number of concurrent
-              tests is controlled by the <code>LLDB_TEST_THREADS</code> environment variable or
-              the <code>--threads</code> command line parameter. The default value is the number
-              of CPUs on your system. To pass additional options to <code>dotest.py</code>,
-              specify those options as an <code>-o</code> argument to <code>dosep.py</code>. For
-              example, the command
+              The dotest.py script runs tests in parallel by default.
+              To disable the parallel test running feature, use the
+              <code>--no-multiprocess</code> flag. The number of
+              concurrent tests is controlled by
+              the <code>LLDB_TEST_THREADS</code> environment variable
+              or the <code>--threads</code> command line parameter.
+              The default value is the number of CPU cores on your
+              system.
             </p>
-            <code>python dosep.py -o "--executable bin/lldb -C bin/clang"</code>
             <p>
-              will specify the lldb and clang executables to test for each dotest invocation.
-              <code>ninja check-lldb</code> is wrapper around <code>dosep.py</code>.
+              The parallel test running feature will handle an
+              additional <code>--test-subdir SUBDIR</code> arg.  When
+              specified, SUBDIR is relative to the root test directory
+              and will limit all parallel test running to that
+              sudirectory's tree of tests.
+            </p>
+            <p>
+              The parallel test runner will run all tests within a
+              given directory serially, but will run multiple
+              directories concurrently.  Thus, as a test writer, we
+              provide serialized test run semantics within a
+              directory.  Note child directories are considered
+              entirely separate, so two child directories could be
+              running in parallel with a parent directory.
             </p>
 
             <h3>Running the test-suite remotely</h3>