Review parallelism and flcbdfwrappers.so about proteus HOT 8 CLOSED

I think we just need to carve out time to fix this across all proteus modules as we discussed at some point during the sprint. Any proteus module should be safe to import.

from proteus.

Just a summary of the issue. Proteus currently possesses an extension module, flcbdfwrappers, that is responsible for a large number of interface routines to C and PETSc, of which I understand many are not currently being used. This extension module relies on being initialized with a proper communicator at some point. I think the Python module, Comm, is responsible for doing that, but there are issues with this approach.

I'm reviewing how the flcbdfwrappers code works, and it looks like it initializes PETSc in the class constructor function DaetkPetscSys_init. This is in turn called from Comm. This opaque object is troublesome, however, because it sets the static module variable: Daetk::Petsc::cc::MPI_Comm Py_PETSC_COMM_WORLD, which in turn is used by all the routines in the module.

This behavior (creating an object affects global data elsewhere in the module), is pretty unexpected, and I think is at the root of this issue.

from proteus.

Yeah, so in the old implementation Comm depended on flcbdfWrappers, which handled all the interaction with PETSc and MPI. It seem to me we just want to complete the handover to mpi4py and petsc4py so that flcbdfWrappers depends cleanly on Comm.

The DaetkPetscSys objects in C++ and their Python wrapper class flcbdfWrappers.DaetkPetscSys work like many other comm abstractions: 1) you can instantiate them multiple times safely, and 2) you can access generic communication functionality through them (e.g. comm.rank, comm.size, comm.barrier, etc.)

It seems like we just need to replace the comm object returned by Comm.init with a Python object that uses petsc4py/mpi4py to implement it's interface instead of DaetkPetscSys. Then Comm doesn't depend on flcbdfWrappers.

The next step is to make sure that on import flcbdfWrappers imports Comm, initializes it (which is safe to call multiple times), and sets the Py_PETSC_COMM_WORLD variable. I think we'll need to instantiate at least one DaetkPetscSys object because that's effectively the Comm object that daetk requires that you instantiate to use MPI.

from proteus.

So I think the a C++ issue in flcbdfwrappers may break our ability to initialize the MPI_COMM_WORLD/PETSC_COMM_WORLD variable safely in the flcbdfwrappers module code, but maybe Chris has an idea how to fix this. I've added more checks to the norms in #128

from proteus.

I think we're clear on how to remove the dependency of proteus.Comm on DaetkPetscSys by just implementing the 8 DaetkPetscSys_methods using petsc4py as members of some new class, say Comm_p4py, and return an object of that type from Comm.init(). That shouldn't break anything.

The next bit of having flcbdfWrappersModule import proteus.Comm in initflcbdfWrappers requires a little bit of Python C API, which is mostly described here (and the C API Reference):

http://stackoverflow.com/questions/3286448/calling-a-python-method-from-c-c-and-extracting-its-return-value

The code that needs to go into initflcbdfWrappers is to replace the body of DaetkPetscSys_init. It needs to set Py_PETSC_COMM_WORLD and instantiate a Daetk::Petsc::Sys (probably stored in a static variable like Py_PETSC_COMM_WORLD) . You may be able to use the petsc4py C API directly to get some of the information directly rather than going through proteus.Comm. All we're really doing besides setting the communicator used by everything in flcbdfWrappers.cpp is initializing the PETSc/MPI subsystem of Daetk, which is done via instantiation of a Daetk::Petsc::Sys. We want to make sure that instantiation references the correct argv and petsc database if they have been provided to Comm in a prior call to Comm.init().

from proteus.

One other thing we might want to consider is renaming Py_PETSC_COMM_WORLD to PROTEUS_COMM_WORLD. Seems like what we really want here is flcbdfWrappers using "the proteus communicator" as set up by the Comm module. A basic call to Comm.init() would default to setting it to MPI_COMM_WORLD=PETSC_COMM_WORLD but it would prepare us for the option of using a different communicator. Everything in flcbdfWrappers is specifically for the global proteus communicator, whatever that may be.

from proteus.

Just looking through this further, the DAETK wrappers intermingle a fair amount of module-level static/global data with the Sys object, so there's more unwinding to do there if we ever need to make that section of the code thread-safe or want to try to maintain multiple communicators.

from proteus.

Mostly addressed when we shifted control to mpi4py and refactored partitioning and flcbdfWrappers with Cython.

from proteus.