Python gas-water simulator #6075
Conversation
This is a duplicate of PyBlackOilSimulator and should be refactored to avoid too much code duplication
Updated docstrings with args parameter.
@svenn-t Interesting. It is not immediately clear to me what is the difficulty with making a base simulator class. Can you elaborate on that? |
|
I made an attempt here: 45be9b8 which compiled but gave "undefined symbol" errors when I imported any of the "BlackOilSimulator" or "GasWaterSimulator" in Python. It might have something to do with how I made the base simulator but I also have next to no experience with pybind and it could be some issues with how I exported the classes. |
Explicitly instantiate the base template classes in the .cpp file.
@svenn-t I think we need to instantiate the templated base class explicitly for the two template values we are using, see this commit: hakonhagland@b551864. This fixed the undefined symbol error for me. |
You are correct, this fixed the "undefined symbol" errors for me as well. I did get some other issues when running examples using the python classes that I need to look into before updating this PR. But thanks for fixing the immediate errors! |
@svenn-t Yes, I am getting this error when I run the unit tests: |
Seems like this happens because the derived class |
Ok, so the issue is having the |
Move the PyMain pointer from the derived class to the base class.
@svenn-t The following seems to work for me: hakonhagland@36ccf4a |
Indeed, this worked for me as well. Thanks a lot for the fixes! Unfortunately, I get a lot of "violates the C++ One Definition Rule" errors when I compile, and running cases with the GasWaterSimulator crashes. Since there are so many of the ODR errors I suspect there is a fundamental issue like, e.g. the BlackOilTwoPhaseIndices for the gas-water simulator has not been properly included or some header file is missing. I'll look into it. |
@svenn-t Do you have any test cases for the gas water simulator? Then I can also have a look at it |
Yes, I have a modified SPE1CASE1 file here: |
|
@svenn-t Thanks for the test case!
I do not get any error when I compile, can you give more details? When I run a python script with these statements: The |
I see now that I get warnings and not errors. I have the build output here: and I just do a simple
I get the same crash report as you when I run the case as well. |
@svenn-t Interesting. I do not get any warnings. Here is my build log build.zip. I compiled like this from the terminal:
This is from and in gdb I can see thatthis->num_components_ is 2, whereas numWellConservationEq is 3.
|
When i build with Debug variant, I also get no compilation warnings. The build log I attached was with Release variant.
I suspect that the part in PyGasWaterSimulator.cpp where we set BlackOilTwoPhaseIndices is not done properly or even set at all. I tried to move that part to PyBaseSimulator.cpp, but I get the same crash when running the test case. |
@svenn-t I see, then I am able to reproduce the warnings also. There are many, the first one I get is: I believe this happens when building the Python shared library |
Split the simulators python shared object into two shared libraries BlackOil and GasWater to avoid ODR violations in the simulators shared library
@svenn-t Here is an example of a splitting into two separate modules: hakonhagland@1ff143a. This compiles without ODR violations for me. |
|
If we are going to split the python module into two, one issue is that each of the two python modules will duplicate much of opm-simulators and opm-common static libraries. To avoid this, we can compile opm-common, opm-grid and opm-simulators as shared libraries (option |
Restructured the pybind11 modules and python modules to make the unit tests pass
To also make the unit tests pass I needed to make some further changes to the pybind11 module names and python import statements, see: hakonhagland@6d24fe1 |
With the new commits, I also do not get the ODR warnings, and running both the gas-water and blackoil versions of the SPE1CASE1 case, I do not get any crashes. Thanks a lot for making these fixes and getting the simulators to work!
I admit this is beyond my knowledge with pybind, cmake, etc. Are there any issues with doing this? I will update this PR with the new commits since it seems to be compiling and working correctly. |
|
relying on dynamic linking to opm libraries is problematic for pip deployment. i much prefer if you track down the odr violation instead. something is instanced in the python module that should not be instanced in the python module. |
@akva2 Is it the auditwheel that is the problem? In principle we should be able to install all dependencies in a manylinux container, right? However, I assume it can be difficult find the correct versions of those that fit together?
@akva2 It is quite a lot of the ODR violations, I think it can become a difficult task. As a workaround we could build the two python modules with the static opm libraries (i.e. include them into the shared objects). The drawback is then only the code duplication in the two modules (that is the size of the modules will be larger than strictly necessary) |
|
while we can in principle install shared dependencies with the python module, it is opening a can of worms i'm very reluctant of taking aboard. the problem is that in the wild west of python package management, there are ways users can screw themself over and end up loading wrong versions of the shared libraries from system paths and not the shipped libs, in particular if the don't use venvs and such. while i'm usually a fan of shared linking, in this case i'll take larger modules any day to avoid the problems so if there is no other way that is what i'd do. |
@akva2 Interesting. By looking at the source it seems It would be interesting to test this, can I just run |
|
right, so there are new hacks in the tools to try to fix the situation then. my trust in python packaging tools is still exactly zero though, so i'm sure there are some tool that screws it up somehow, somewhere in the world.. basically yes, invocation on jenkins is where $1 is the output directory, $2 is the tag to append to the package version, and version is tag to build from (could be "master"). the build args default to tag="master" and version="", where i usually give .dev$DATE for the nightly builds, if not it will use the version (if given it's version from dune.module-$version) |
@akva2 Yes this worked for static opm libraries after a small change to the I will try to replace these packages with ones including the shared versions and see how far I get. |
It seems to work after these changes: hakonhagland@84a9041. I will try to install the generated wheels in some containers to check if they also installs successfully. |
|
I am no longer able to build in the container since it now uses cmake 4.0, see: #6122 |
Have now tested the wheels with both shared and static opm libraries in an Ubuntu 22.04 docker container, see #6136. All unit tests passed there. |
Added a Python gas-water simulator to be able to run f.ex. CO2-/H2STORE data files. Also added the option to input command-line arguments in constructor so both ways of instantiating the simulator classes have that ability.
I open this PR in draft mode because most of the code is duplicate of the PyBlackOilSimulator implementation, which is not optimal. I've made futile attempts at making a base simulator that the other Python simulators can inherit. If anybody have suggestions on how to make a base simulator (or want to take up the mantle?), I'm happy to hear them!