ioam / topographica

A general-purpose neural simulator focusing on topographic maps.
topographica.org
BSD 3-Clause "New" or "Revised" License
53 stars 32 forks source link

Confusing documentation about using openmp #542

Open ceball opened 10 years ago

ceball commented 10 years ago

The documentation in inlinec talks about setting openmp to True and setting openmp_threads to the number of desired threads, but that doesn't match what the code does (my reading is that it ignores openmp, and takes any value of openmp_threads except 1 to mean 'turn on openmp and get number of threads from the OMP_NUM_THREADS environment variable'.).

Additionally, I couldn't figure out how to get topographica to tell me how many threads it's using. This is probably just because I couldn't figure out the right way to set logging to the right level before commandline.py's process_argv() is called.

jbednar commented 10 years ago

As listed at https://github.com/ioam/topographica/pull/526 (pull request #526), our intention was to remove all openmp-related parameters and special processing in Topographica code, leaving the user free to pass any options they want directly to OpenMP via OpenMP's environment variables. Looks like I removed all that processing from topo.misc.commandline, but did not see the further processing in topo.misc.inlinec. The openmp_ code in inlinec doesn't seem to make the least bit of sense to me, and so I'll remove it and simply enable OpenMP permanently, unless someone objects.

jlstevens commented 10 years ago

As long as I can use OMP_NUM_THREADS environment variable to determine the OpenMP behaviour and as long as the default behaviour of using all available threads is still in place, I don't mind what happens to inlinec. Aren't we supposed to be switching to Philipp's Cython implementation soon anyway?

philippjfr commented 10 years ago

That code simply sets up the OpenMP looping pragma and sets the compiler arguments. Once we switch to Cython (I'll email DICE support now), that just moves to the optimized module.

jbednar commented 10 years ago

Before it moves, can you please clean it up to make sense? What it says in the comments or docstrings doesn't match what it does, and what it does is odd. Specifically, openmp_threads is treated as a number (compared against 1), when as far as I can tell it should be a Boolean that simply enables or disables OpenMP. Can we change the documentation to say that explicitly, and use != False instead of ==1 to make it clear that it's only a Boolean (and not e.g. the number of threads to use)?

ceball commented 10 years ago

From #541 (I don't want to de-rail the mac problem):

Chris, the behavior for warning messages should be the same as ever, i.e. that they get printed to the terminal; you can see that by disabling weave explicitly:

./topographica -c import_weave=False examples/tiny.ty Note: Inline-optimized components are currently disabled; see topo.misc.inlinec

If you aren't getting that message, Topographica thinks weave is working, which it can tell reliably because it tries to compile a random program each time it runs, just to detect any problems of this type. It doesn't do similar checking to see if OpenMP is working, since that's not always installed, but we could certainly try to test for it and report its absence.

I think I was trying to get topographica to tell me how many threads it was using, and when I couldn't, I began to wonder about all the message printing. Specifically, I couldn't get this message from topo.misc.inlinec to print:

openmp_main.verbose("Using %d threads on a machine with %d detected CPUs" % (num_threads, total_cores))

(https://github.com/ioam/topographica/blob/master/topo/misc/commandline.py#L687)

How do I set the logging level in time? I tried this:

$ ./topographica -c 'import param.parameterized; param.parameterized.get_logger().level=param.parameterized.VERBOSE' examples/tiny.ty

(Maybe I should study logging's documentation...anyway, I realize you're going to be leaving it to the user and openmp to deal with number of threads between themselves, so this code doesn't matter any more.)

About using all the threads available by default - is that just to compete with people running matlab on the same machine? Or do we have functions whose performance scales well with the number of threads now? ;)

jbednar commented 10 years ago

Ah. Yes, there is a problem there. The simpler way to enable verbose printing is just:

./topographica -v examples/tiny.ty

but that doesn't work either, because the verbose message you want to see is logged just before command-line processing. That's actually one of our big headaches that caused us to remove our openmp option processing -- how to let the user do things while making sure that command-line options are processed in the right order. Here we have to deal with OpenMP before the command line options are processed (since some of those options will cause computationally intensive procedures to start), but until we've seen all of those options, we can't be sure people won't change the openmp options. That's why we punted.

In this case, the only way I can see to enable verbose message printing in time is to put it into your .topographicarc, which is processed before anything to do with OpenMP because we assume you won't put any CPU-intensive code in the rc file::

$ cat ~/.topographicarc import param.parameterized param.parameterized.get_logger().level=param.parameterized.VERBOSE $ ./topographica examples/tiny.ty Executing user startup file /afs/inf.ed.ac.uk/user/j/jbednar/.topographicarc VERBOSE:root:Time: 000000.00 OpenMP: Using 4 threads on a machine with 4 detected CPUs $

This works, but is fairly non-intuitive. The only way around it that I could see is to process all -v options first on the commandline, before enabling openmp, which is a bit of a pain. So I'm not sure there's a simple fix, but if there is, let me know.

As for using threads, current versions make excellent use of multiple cores, with substantial performance improvements from additional cores. It's not linear speedup, due to memory contention, but it's still very much worth doing.