Python interface #1068

corbett5 · 2020-07-22T10:09:18Z

Related to
GEOS-DEV/LvArray#186
https://github.com/GEOSX/integratedTests/pull/78
GEOS-DEV/PAMELA#46
GEOS-DEV/PVTPackage#37

corbett5 · 2020-07-22T10:14:05Z

@jameshcorbett can you add methods to numpyConversion.hpp to create a python object from scalars? Then write some LvArray scalar tests and finally some tests for WrapperBase::createPythonObject in GEOSX. They don't need to be extensive.

corbett5 · 2020-07-22T22:27:57Z

@cssherman do you have an idea on how to modify EvenManager::Run to support an event like ReturnToPython which would return immediately from the event loop and allow subsequent calls to run to resume right where it left off? I see there's already an exit flag check. But that only allows you to break out of the loop after all the sub-events have occurred. Furthermore when you do exit you run the cleanup on each sub-event.

https://github.com/GEOSX/GEOSX/blob/a56fdcf07049c059464f76d3c618af0fbe17d61c/src/coreComponents/managers/EventManager.cpp#L180

cssherman · 2020-07-22T23:28:58Z

@cssherman do you have an idea on how to modify EvenManager::Run to support an event like ReturnToPython which would return immediately from the event loop and allow subsequent calls to run to resume right where it left off? I see there's already an exit flag check. But that only allows you to break out of the loop after all the sub-events have occurred. Furthermore when you do exit you run the cleanup on each sub-event.

https://github.com/GEOSX/GEOSX/blob/a56fdcf07049c059464f76d3c618af0fbe17d61c/src/coreComponents/managers/EventManager.cpp#L180

@corbett5 - I expect that shouldn't be too difficult.

Since the primary while loop is written with restarts in mind, calling EventManager::Run() will cause it to automatically resume where it stopped. I'd suggest adding an additional pythonInterrupt flag that the events can set to handle the breaking.

Also, this comes to mind: https://xkcd.com/292/

rrsettgast · 2020-07-22T23:32:35Z

make sure to create a man page:
https://xkcd.com/293/

corbett5 · 2020-07-23T00:06:07Z

@corbett5 - I expect that shouldn't be too difficult.

Since the primary while loop is written with restarts in mind, calling EventManager::Run() will cause it to automatically resume where it stopped. I'd suggest adding an additional pythonInterrupt flag that the events can set to handle the breaking.

Also, this comes to mind: https://xkcd.com/292/

Awesome, thanks!

corbett5 · 2020-07-23T08:40:45Z

@rrsettgast Where do we need to be able to return back into python to set boundary conditions? Currently it returns after ProblemManager::ProblemSetup and then anywhere you put an even in once the event loop begins.

rrsettgast · 2020-07-23T16:10:42Z

@rrsettgast Where do we need to be able to return back into python to set boundary conditions? Currently it returns after ProblemManager::ProblemSetup and then anywhere you put an even in once the event loop begins.

@corbett5 Can we use "baton passing" terminology? I would propose that we want to pass the baton around a couple of places prior to the event loop:

Hand the baton to python prior to the execution of ProblemManager::ApplyInitialConditions. From this point we could add entries to the FieldSpecificationManager. We can add both boundary conditions and initial conditions through the standard infrastructure provided by FieldSpecificationManager. Then hand the baton back to GEOSX and execute the contents of ProblemManager::ApplyInitialConditions.
Hand the baton to python after to the execution of ProblemManager::ApplyInitialConditions in order to manually initial conditions from within python outside the structure of FieldSpecificationManager. That way python can override initial conditions from the xml file, and it could easily manipulate the data it wants to set.
As far as setting (non-initial) boundary conditions in python from within the physics solver (without going through the FieldSpecificationManager), I would think that we want to avoid this. HOWEVER, we can always set BC's manually once the systems are constructed and filled. I would suggest that there be a mechanism to pass the baton (and the matrix/rhs) back to python prior to the execution of SolveSystem(). So that is kind of in the physics solver, but probably in SolverBase.

Of course, this is just my thinking. I am very open to improving the approach.

corbett5 · 2020-07-23T18:50:07Z

I like the baton. So before and after ApplyInitialConditions is easy but inside PhysicsSolverBase::Execute is tricky. We would have to make both SolverStep and Execute able to pick up where they left off, not to mention that they're both virtual methods that are overridden by some solvers. If you have any ideas on how to do that let me know.

corbett5 · 2020-07-29T07:43:39Z

Closes #792

rrsettgast · 2020-07-29T17:52:18Z

I like the baton. So before and after ApplyInitialConditions is easy but inside PhysicsSolverBase::Execute is tricky. We would have to make both SolverStep and Execute able to pick up where they left off, not to mention that they're both virtual methods that are overridden by some solvers. If you have any ideas on how to do that let me know.

I would say that we shouldn't have any of the baton passing in virtual functions and the solver passes should be placed in NonlinearImplicitStep....but I see that NonlinearImplicitStep is virtual. So the rule becomes "no baton passing in virtual functions that are commonly overridden". As it stands NonlinearImplicitStep will most likely be used intact, with the occasional override occurring while developing an approach that should ultimately made part of the original NonlinearImplicitStep.

corbett5 · 2020-07-29T18:32:05Z

I would say that we shouldn't have any of the baton passing in virtual functions and the solver passes should be placed in NonlinearImplicitStep....but I see that NonlinearImplicitStep is virtual. So the rule becomes "no baton passing in virtual functions that are commonly overridden". As it stands NonlinearImplicitStep will most likely be used intact, with the occasional override occurring while developing an approach that should ultimately made part of the original NonlinearImplicitStep.

And in LinearImplicitStep right? Also it appears that LagrangianContactSolver is the only one to override NonLinearImplicitStep and nothing overrides LinearImplicitStep.

rrsettgast · 2020-07-29T19:32:24Z

I would say that we shouldn't have any of the baton passing in virtual functions and the solver passes should be placed in NonlinearImplicitStep....but I see that NonlinearImplicitStep is virtual. So the rule becomes "no baton passing in virtual functions that are commonly overridden". As it stands NonlinearImplicitStep will most likely be used intact, with the occasional override occurring while developing an approach that should ultimately made part of the original NonlinearImplicitStep.

And in LinearImplicitStep right? Also it appears that LagrangianContactSolver is the only one to override NonLinearImplicitStep and nothing overrides LinearImplicitStep.

We may just remove LinearImplicitStep at some point. Is it actually still called from anywhere?

corbett5 · 2020-07-29T19:33:07Z

We may just remove LinearImplicitStep at some point. Is it actually still called from anywhere?

Looks like LaplaceFEM is the only place.

src/coreComponents/dataRepository/Group.cpp

rrsettgast · 2020-07-31T01:52:09Z

src/coreComponents/mpiCommunications/CommunicationTools.hpp

  CommunicationTools();
  ~CommunicationTools();

-  static void AssignGlobalIndices( ObjectManagerBase & object,


wait...no static functions?

Static functions are fine, but one of the methods had a static variable. I made the variable a class member and therefore all of the previous static methods that then used this now non-static function had to be non-static.

cssherman · 2020-08-03T16:56:27Z

src/coreComponents/python/example_bindings/python_bind.cpp

@@ -1,123 +0,0 @@
-/*


It's OK to get rid of these old binding examples

cssherman · 2020-08-03T16:58:52Z

src/coreComponents/python/modules/pygeos_package/.gitignore

@@ -1,2 +0,0 @@
-build


We should not get rid of this package. In #990 I'm planning to change the name to geosx_xml_tools, which is more descriptive of what it actually does

src/cmake/thirdparty/SetupGeosxThirdParty.cmake

cssherman · 2020-09-01T22:36:13Z

@corbett5 - since we'll be accessing a number of 1D arrays, it would be useful to define the __len__() method for LvArray::Array. (Note: for >1D arrays, numpy returns the length of the first axis.)

aperture = problem.getWrapper(aperture_key).value(True)

# This is much more convenient than (np.prod(np.shape(aperture)) > 0)
if len(aperture):
  print('do something')

jameshcorbett · 2020-09-02T01:07:10Z

@corbett5 - since we'll be accessing a number of 1D arrays, it would be useful to define the __len__() method for LvArray::Array. (Note: for >1D arrays, numpy returns the length of the first axis.)
aperture = problem.getWrapper(aperture_key).value(True)

# This is much more convenient than (np.prod(np.shape(aperture)) > 0)
if len(aperture):
  print('do something')

We can definitely do that. Actually, Ben and I have been talking about making it so that Arrays and SortedArrays behave exactly like Numpy arrays, so you could pass them to numpy.sum, call array.sin(), call len() like you mentioned, and so on. The only difference should be that they have some additional LvArray-specific methods for resizing, moving to the GPU, etc. There is a reasonably easy way to do it with some Python special methods---we would just make (pure python) wrapper classes around the python classes we already have defined in C++. What do you think?

jameshcorbett · 2020-09-02T01:09:20Z

By the way, I just finished up some documentation for all the Python LvArray classes. If you pull the latest branch of LvArray (it's not in GEOSX yet, I need to make some changes before I can incorporate it) and make the documentation, you'll see a new section "LvArray in Python" which outlines all the classes and their methods.

Hopefully tomorrow I can make something for the pygeosx module as well.

cssherman · 2020-09-02T16:08:29Z

additional LvArray-specific methods for resizing, moving to the GPU, etc. There is a reasonably easy way to do it with some Python special methods---we would just make (pure python) wrapper classes around the python classes we already have defined in C++. What do you think?

Makes perfect sense to me

cssherman · 2020-09-10T22:04:57Z

@corbett5 @jameshcorbett - I was curious to see what would happen if I tried running an xml file with a python hook using the primary geosx executable. Obviously, this usage shouldn't work. We should work on the error message though:

...
Adding Object FaceElementRegion named Fracture from ObjectManager::Catalog.
Time: 0s, dt:1s, Cycle: 0
***** WARNING
***** LOCATION: /usr/WS2/sherman/GEOSX/src/main/main.cpp:54
***** Controlling expression (should be false): state.getState() != State::COMPLETED
Simulation exited early.

corbett5 · 2020-09-11T01:16:10Z

@corbett5 @jameshcorbett - I was curious to see what would happen if I tried running an xml file with a python hook using the primary geosx executable. Obviously, this usage shouldn't work. We should work on the error message though:

Yeah that's not the best error message, thanks!

jameshcorbett · 2020-09-23T05:40:02Z

I just force-pushed a bunch of changes. Here's a summary:

There is a new pylvarray module which defines all the Python wrappers for LvArray classes. You can't instantiate instances of the classes, though, so it's really only useful for type-checking and help messages.
bin/python has been removed from the build directory. It has been replaced with lib/PYGEOSX/bin/python. PYGEOSX is a virtual environment that can be activated with source lib/PYGEOSX/bin/activate and deactivated with deactivate. When it's active, you'll be able to import pygeosx and pylvarray, since those modules are symlinked into the venv.
Both pygeosx and pylvarray have been standardized to PEP8 compliance and will be kept that way from now on. So @cssherman you will need to change, e.g., getGroup() to get_group() in your branch.
Both pylvarray and pygeosx are incorporated into the sphinx documentation. For right now, to build you'll first need to build pygeosx, then activate a Python3 virtual environment with sphinx installed (python3 -m venv MyVenv && source MyVenv/bin/activate && pip install sphinx), then make the documentation. Under "user guide" you should see "GEOSX in python" with a description of all the functions and classes (and a link to the pylvarray documentation at the top). I'll get rid of the need for the virtual environment soon, but right now it makes the documentation easier.
You can now add a callback to the physics solver, which will be invoked after the solver applies boundary conditions. Add it like pygeosx.get_group("Problem/Solvers/lagsolve").register(callback). The callback will be invoked (possibly many times) during calls to pygeosx.run(). The callback should take two arguments: the CRSMatrix and the 1d array m_localRhs.

Ben and I were talking about what should happen when the callback raises an exception. Usually you want to propagate the exception back to Python immediately, so that the user sees it right away. Unfortunately, doing so would require a lot of changes to the code base, because all of the functions between pygeosx.run() and SolverBase::NonlinearImplicitStep would need to support returning some kind of failure notification, as in "I'm returning early because an error needs to be propagated back to Python." That would be pretty ugly, so we thought the best way to do it would be by raising a custom C++ exception class that is caught right before returning to Python---that way we didn't have to modify any of the existing code at all. It makes for a nice solution, but Ben said that the GEOSX team hadn't agreed about if or how C++ exceptions should be used, so it deserves mention. If we don't want to introduce C++ exceptions to the code base, we could either (a) pretend the callback succeeded and raise the error later, possibly giving unexpected results; (b) forcibly exit the process; or (c) modify the code base so that we can return the error immediately without using the C++ exception mechanism.

Ben also suggested that I ask you @cssherman whether you think it would be useful to have callbacks in other locations in the code. It would be very easy to do---the hardest part would be determining how to make it clear for the user what all the different callbacks are for (maybe something like pygeosx.get_group("Problem/Solvers/lagsolve").register(callback, pygeosx.APPLY_BOUNDARY_CONDITIONS)).

jameshcorbett · 2020-09-30T18:49:38Z

With regards to the docstrings, that we might be mis-understanding each other and talking about different things. I'd propose waiting until a clear use-case pops up, where we need to pass information to the user.

I'm sure you're right. To change the subject, then, I think what @corbett5 wanted to know about the callbacks is whether you think we should add more of them. Right now there's only one kind: it's on the physics solver, and it's called after applying the boundary conditions. (So it may be called multiple times during one call to run.) If you think there are other places you would like be able to manipulate GEOSX state, just say so and I can add more breakpoints in GEOSX.

To clarify, yes an individual virtual environment is absolutely tied to a single python distribution. The point I was trying to make is that that the modules/scripts we're using in the existing environment are compatible with both versions of python. As such, we can derive the environment from the same python 3 that you use to compile pygeosx.

Oh OK, that makes sense, cool.

corbett5 · 2020-11-12T19:05:55Z

Closes #792

corbett5 · 2021-01-30T01:17:20Z

@rrsettgast @cssherman Okay this is up-to-date with develop. I built python and the associated libraries with spack and installed them in /usr/gapps/GEOSX/thirdPartyLibs/python and added them to the quartz-gcc@8.1.0 and quartz-clang@10.0.0 host configs.

I'm sure there's still work to be done, but I think it's in a good state to merge. That way we can get some people using it and some feedback.

cssherman · 2021-02-01T14:54:58Z

@corbett5 - cool! I'll do some work to update the separate branch with the python helper functions I built to work with the new interface.

corbett5 · 2021-02-01T19:03:47Z

@cssherman awesome! We're going to give a walkthrough at the meeting Thursday, it'd be nice if you could talk about the virtual environment and the stuff you have cooking on that branch a little bit.

cssherman · 2021-02-03T21:25:58Z

@corbett5 - to make things easier, could we enable the ssl package in the python distribution (a config option, required to run pip)? It would also be very useful to also install the virtualenv package so that users can build virtual python environments on shared systems.

corbett5 · 2021-02-04T08:02:00Z

@cssherman I'll do that tomorrow. Spack builds Python with SSL by default, and I pointed it to the LC installation but I guess I didn't point correctly as Python failed to build the ssl module which is not a hard error so I didn't notice.

corbett5 · 2021-02-08T23:59:39Z

Okay, in my opinion this is ready to merge.

@cssherman The two Python installations in quartz-clang@10 and quartz-gcc are built with ssl and I have pip installed virtualenv. If there are any packages you would like to globally install (xml?) feel free to do so, but please do it as geosadmn for both of the builds and chmod -R g+rx /usr/gapps/GEOSX/thirdPartyLibs/python and chgrp -R GEOS /usr/gapps/GEOSX/thirdPartyLibs/python once they've been installed.

corbett5 · 2021-02-09T00:15:49Z

@bmhan12 The cuda-debug build is failing although it looks to me like it's actually passing. Thoughts?
https://travis-ci.com/github/GEOSX/GEOSX/jobs/481399225

bmhan12 · 2021-02-09T00:36:31Z

@bmhan12 The cuda-debug build is failing although it looks to me like it's actually passing. Thoughts?
https://travis-ci.com/github/GEOSX/GEOSX/jobs/481399225

That's odd, I can't see why the exit code would change. Does it fail when you rerun the job?

klevzoff · 2021-02-09T00:40:44Z

@bmhan12 The cuda-debug build is failing although it looks to me like it's actually passing. Thoughts?
https://travis-ci.com/github/GEOSX/GEOSX/jobs/481399225

That's odd, I can't see why the exit code would change. Does it fail when you rerun the job?

High up the build log, there's this:

docker: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit.
See 'docker run --help'.
The command "GEOSX_TPL_DIR=$(docker run --rm ${DOCKER_REPOSITORY}:${GEOSX_TPL_TAG} /bin/bash -c 'echo ${GEOSX_TPL_DIR}')" exited with 125.

corbett5 · 2021-02-09T04:31:39Z

@klevzoff wow thanks! That's funny it still ran.

rrsettgast · 2021-02-10T23:41:12Z

src/coreComponents/dataRepository/Group.cpp

-Group::Group( string const & name,
-              Group * const parent ):
-  m_parent( parent ),
+Group::Group( std::string const & name,


remove std::

corbett5 added the type: feature New feature or request label Jul 22, 2020

corbett5 requested a review from jameshcorbett July 22, 2020 10:09

corbett5 force-pushed the feature/corbetts/python branch from e7bc82d to 04ac9cf Compare July 28, 2020 04:24

corbett5 linked an issue Jul 29, 2020 that may be closed by this pull request

Create a RAII wrapper around CommunicationTools's communication ID. #792

Closed

corbett5 force-pushed the feature/corbetts/python branch from 8dd3dda to 453c3f4 Compare July 30, 2020 01:40

rrsettgast reviewed Jul 31, 2020

View reviewed changes

cssherman reviewed Aug 3, 2020

View reviewed changes

cssherman reviewed Aug 5, 2020

View reviewed changes

src/cmake/thirdparty/SetupGeosxThirdParty.cmake Outdated Show resolved Hide resolved

jameshcorbett force-pushed the feature/corbetts/python branch from a87143e to ec163ec Compare September 23, 2020 04:22

corbett5 force-pushed the feature/corbetts/python branch from e46828c to ef37751 Compare January 30, 2021 01:12

corbett5 marked this pull request as ready for review January 30, 2021 01:13

corbett5 added flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests flag: requires updated submodule(s) labels Jan 30, 2021

corbett5 force-pushed the feature/corbetts/python branch from 004cf8e to 4be095c Compare February 4, 2021 04:04

corbett5 changed the title ~~WIP: Python interface~~ Python interface Feb 4, 2021

corbett5 force-pushed the feature/corbetts/python branch from e48bbec to 899aef6 Compare February 8, 2021 22:08

jameshcorbett approved these changes Feb 10, 2021

View reviewed changes

rrsettgast approved these changes Feb 11, 2021

View reviewed changes

rrsettgast and others added 5 commits February 11, 2021 11:01

Add current GEOSX compilers to spack.

cdfe177

Added python interface.

2c95fde

Removed static variables.

b096368

Python improvements.

4f7106a

Removed std::string.

22010fa

corbett5 force-pushed the feature/corbetts/python branch from 899aef6 to 22010fa Compare February 11, 2021 19:16

corbett5 merged commit 11dffd4 into develop Feb 12, 2021

corbett5 deleted the feature/corbetts/python branch February 12, 2021 00:58

Python interface #1068

Python interface #1068

Conversation

corbett5 commented Jul 22, 2020 • edited Loading

corbett5 commented Jul 22, 2020

corbett5 commented Jul 22, 2020

cssherman commented Jul 22, 2020

rrsettgast commented Jul 22, 2020

corbett5 commented Jul 23, 2020

corbett5 commented Jul 23, 2020

rrsettgast commented Jul 23, 2020

corbett5 commented Jul 23, 2020

corbett5 commented Jul 29, 2020

rrsettgast commented Jul 29, 2020

corbett5 commented Jul 29, 2020

rrsettgast commented Jul 29, 2020

corbett5 commented Jul 29, 2020

rrsettgast Jul 31, 2020

Choose a reason for hiding this comment

corbett5 Aug 4, 2020

Choose a reason for hiding this comment

cssherman Aug 3, 2020

Choose a reason for hiding this comment

cssherman Aug 3, 2020

Choose a reason for hiding this comment

cssherman commented Sep 1, 2020

jameshcorbett commented Sep 2, 2020

jameshcorbett commented Sep 2, 2020

cssherman commented Sep 2, 2020

cssherman commented Sep 10, 2020

corbett5 commented Sep 11, 2020

jameshcorbett commented Sep 23, 2020

jameshcorbett commented Sep 30, 2020 • edited Loading

corbett5 commented Nov 12, 2020

corbett5 commented Jan 30, 2021

cssherman commented Feb 1, 2021

corbett5 commented Feb 1, 2021

cssherman commented Feb 3, 2021

corbett5 commented Feb 4, 2021

corbett5 commented Feb 8, 2021

corbett5 commented Feb 9, 2021 • edited Loading

bmhan12 commented Feb 9, 2021

klevzoff commented Feb 9, 2021

corbett5 commented Feb 9, 2021

rrsettgast Feb 10, 2021

Choose a reason for hiding this comment

corbett5 commented Jul 22, 2020 •

edited

Loading

jameshcorbett commented Sep 30, 2020 •

edited

Loading

corbett5 commented Feb 9, 2021 •

edited

Loading