Fix/pyAction: Separate classes for python bindings and opm embedded #4017

lisajulia · 2024-04-17T11:50:57Z

This PR contains 5 minor commits, one larger change and one commit adding a stub-file for opm_embedded as well as an entry about that to the python/README.md.

The larger change:
Before, there were three modules exported to Python:
- opmcommon_python
- opm_embedded, as embedded Python module
- opm_embedded, as the shared library

Now, opm_embedded is not built as a shared library anymore, but uses opmcommon_python

PR #4029 removes the creation of a shared library entirely, then only opmcommon_python is built.

lisajulia · 2024-04-17T11:51:45Z

@hakonhagland: Can you also have a look at this? Thanks! (I'm tagging you here because for some reason you don't appear in the reviewers dropdown)

lisajulia · 2024-04-17T11:52:57Z

jenkins build this please

lisajulia · 2024-04-17T14:45:53Z

jenkins build this please

bska

Since I don't really know enough about pybind11 I just have a tangential question. Overall this looks good to me and I appreciate the refactoring.

bska · 2024-04-17T15:12:38Z

python/cxx/export.hpp

 void export_ScheduleState(py::module& module);
 void export_TableManager(py::module& module);
 void export_Well(py::module& module);
 void export_Log(py::module& module);
 void export_IO(py::module& module);
 void export_EModel(py::module& module);
 void export_SummaryState(py::module& module);
+void export_SummaryState_embedded(py::module& module);


Is there a formal requirement that all functions which export something have to be named export_*? If not, could we conceivably have

namespace python::common::export { void SummaryState_embedded(py::module& module); // &c }

instead?

export is a keyword in C++, if we use another one, like python::common::export_opm then that's possible, then I'd suggest

python::common::export_opm { void summaryState_embedded(py::module& module); // &c }

python/cxx/export.cpp

lisajulia · 2024-04-18T07:05:11Z

jenkins build this please

python/cxx/eclipse_config.cpp

blattms · 2024-04-18T07:47:30Z

I was a bit unsure whether the macro magic really works with CMake and I checked the output of make VERBOSE=1:

[ 52%] Building CXX object CMakeFiles/opmcommon.dir/python/cxx/simulation_config.cpp.o
/usr/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_SYSTEM_DYN_LINK -DEMBEDDED_PYTHON -DFMT_SHARED -DHAVE_CONFIG_H=1 -I/home/mblatt/src/dune/opm-2.9/opm-common/opm-parallel -I/home/mblatt/src/dune/opm-2.9/opm-common/opm-parallel/include -I/home/mblatt/src/dune/opm-2.9/opm-common -I/usr/include/cjson -isystem /usr/include/python3.11 -pipe -Wall -Wextra -Wshadow -fopenmp -pthread -O3 -DNDEBUG -mtune=native -UNDEBUG -fPIC -fopenmp -std=c++17 -Wno-shadow -MD -MT CMakeFiles/opmcommon.dir/python/cxx/simulation_config.cpp.o -MF CMakeFiles/opmcommon.dir/python/cxx/simulation_config.cpp.o.d -o CMakeFiles/opmcommon.dir/python/cxx/simulation_config.cpp.o -c /home/mblatt/src/dune/opm-2.9/opm-common/python/cxx/simulation_config.cpp

[ 98%] Building CXX object CMakeFiles/opmcommon_python.dir/python/cxx/simulation_config.cpp.o
/usr/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_SYSTEM_DYN_LINK -DEMBEDDED_PYTHON -DFMT_SHARED -DHAVE_CONFIG_H=1 -Dopmcommon_python_EXPORTS -I/home/mblatt/src/dune/opm-2.9/opm-common/opm-parallel -I/home/mblatt/src/dune/opm-2.9/opm-common/opm-parallel/include -I/home/mblatt/src/dune/opm-2.9/opm-common -I/usr/include/cjson -isystem /usr/include/python3.11 -pipe -Wall -Wextra -Wshadow -fopenmp -pthread -O3 -DNDEBUG -mtune=native -UNDEBUG -fPIC -fvisibility=hidden -flto -fno-fat-lto-objects -fopenmp -std=c++17 -Wno-shadow -MD -MT CMakeFiles/opmcommon_python.dir/python/cxx/simulation_config.cpp.o -MF CMakeFiles/opmcommon_python.dir/python/cxx/simulation_config.cpp.o.d -o CMakeFiles/opmcommon_python.dir/python/cxx/simulation_config.cpp.o -c /home/mblatt/src/dune/opm-2.9/opm-common/python/cxx/simulation_config.cpp

[ 99%] Building CXX object CMakeFiles/opm_embedded.dir/python/cxx/simulation_config.cpp.o
/usr/bin/c++ -DBOOST_ALL_NO_LIB -DBOOST_SYSTEM_DYN_LINK -DEMBEDDED_PYTHON -DFMT_SHARED -DHAVE_CONFIG_H=1 -Dopm_embedded_EXPORTS -I/home/mblatt/src/dune/opm-2.9/opm-common/opm-parallel -I/home/mblatt/src/dune/opm-2.9/opm-common/opm-parallel/include -I/home/mblatt/src/dune/opm-2.9/opm-common -I/usr/include/cjson -isystem /usr/include/python3.11 -pipe -Wall -Wextra -Wshadow -fopenmp -pthread -O3 -DNDEBUG -mtune=native -UNDEBUG -fPIC -fvisibility=hidden -flto -fno-fat-lto-objects -fopenmp -std=c++17 -Wno-shadow -MD -MT CMakeFiles/opm_embedded.dir/python/cxx/simulation_config.cpp.o -MF CMakeFiles/opm_embedded.dir/python/cxx/simulation_config.cpp.o.d -o CMakeFiles/opm_embedded.dir/python/cxx/simulation_config.cpp.o -c /home/mblatt/src/dune/opm-2.9/opm-common/python/cxx/simulation_config.cpp

Seems link -DOPM_EMBEDDED somehow wins.

blattms · 2024-04-18T07:50:38Z

That might have been with an old version

lisajulia · 2024-04-18T07:59:24Z

EMBEDDED_PYTHON

Clarified in a quick phone call - I also found what you meant: The link -DEMBEDDED_PYTHON comes from this line: https://github.com/OPM/opm-common/blob/master/CMakeLists.txt#L502

blattms · 2024-04-18T08:08:19Z

Well, I am too slow pulling, or you just too fast for me. Sorry for the noise. With the new version it works

lisajulia · 2024-04-18T11:28:35Z

jenkins build this please

lisajulia · 2024-04-18T13:24:39Z

jenkins build this please

lisajulia · 2024-04-22T15:13:44Z

jenkins build this please

lisajulia · 2024-04-22T15:13:55Z

@hakonhagland: Can you have another look?

lisajulia · 2024-04-26T13:33:27Z

jenkins build this please

lisajulia · 2024-04-26T18:22:51Z

@blattms @bska @akva2 @hakonhagland this PR and #4029 are finally green and ready :)

…create opm.common.* Now, there are two modules created - opmcommon_python - opm_embedded, as embedded Python module, needed for PYACTION and PYINPUT

lisajulia · 2024-04-29T06:46:49Z

jenkins build this please

lisajulia · 2024-04-29T07:41:51Z

jenkins build this please

blattms · 2024-04-29T09:44:49Z

I tested this one on Ubuntu noble (version of this morning at 8:00) and the tests in opm-common build fine. So this seems to work for Python 3.12. We still have OPM/opm-simulators#5322 (but as it turns out we do not run the (failing) python tests on Debian and Ubuntu and the problem is there for older python versions).

Problem is that currently I am really nervous and will also try to run a few of the models in the regression test suite on Ubuntu noble. Let's see...

lisajulia · 2024-04-29T10:01:51Z

Did you run this one or #4029? But in any case: good that we have sth. working on Ubuntu noble!!! 🎉

blattms · 2024-04-29T11:39:00Z

Turns out I am a bit stupid and somehow missed activating OPM_EMBEDDED

Last commit is "Add file and entry in README.md for autocompletion in VS Code"

lisajulia · 2024-04-30T09:00:51Z

@blattms: I've changed the stub file now - your idea of putting it at opm_embedded/init.pyi worked 👍
Thanks!

lisajulia · 2024-04-30T09:16:00Z

jenkins build this please

blattms · 2024-04-30T10:00:23Z

Looks good and is tested. Merging.

GitPaean · 2024-05-27T11:19:03Z

I was using the super cmake file (https://github.com/OPM/opm-utilities/tree/master/opm-super) without building everything but only flow_blackoil and the things it depends on.

The cmake flags is -DOPM_ENABLE_EMBEDDED_PYTHON=1 -DOPM_ENABLE_PYTHON=1.

after make -j 8 flow_blackoil, it was successfully compiled.

But when I tried to python3 -c "import opm_embedded" , it complains as follows,

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/kaib/OPM-master-test/build/opm-common/python/opm_embedded/__init__.py", line 6, in <module>
    from opm.opmcommon_python.embedded import current_ecl_state, current_summary_state, current_schedule, current_report_step
ModuleNotFoundError: No module named 'opm.opmcommon_python

Basically, it means that the module opm_embedded is built while the module opmcommon_python is not. But module opm_embedded requires opmcommon_python .

It is nice that dependence handling can be improved when coming to this. Thanks. @lisajulia

lisajulia · 2024-05-27T11:31:05Z

@GitPaean: Thanks for noticing!
Can you please doublecheck in the CMakeCache.txt file of opm-common if the flags OPM_ENABLE_EMBEDDED_PYTHON OPM_ENABLE_PYTHON are really set to true? Thanks!

GitPaean · 2024-05-27T11:38:51Z

@GitPaean: Thanks for noticing! Can you please doublecheck in the CMakeCache.txt file of opm-common if the flags OPM_ENABLE_EMBEDDED_PYTHON OPM_ENABLE_PYTHON are really set to true? Thanks!

Yes,

lisajulia · 2024-05-27T11:44:51Z

Ok, thanks, can you check if the python library file opmcommon_python is really not in <build-folder-of-opm-common>/python/opm/?

akva2 · 2024-05-27T11:46:28Z

The important point here is that he uses the super-build and not the separate buildsystem per module. Do not try to use a separate buildsystem build / dune-control build to analyze this situation.

GitPaean · 2024-05-27T11:48:10Z

Ok, thanks, can you check if the python library file opmcommon_python is really not in <build-folder-of-opm-common>/python/opm/?

opmcommon_python.cpython-310-x86_64-linux-gnu.so is not in the opm-common/python/opm before I manually make opmcommon_python -j 12.

lisajulia · 2024-05-27T11:54:27Z

Ok, thanks for all the info!
I'll then investigate why for the super-build file opmcommon_python is not build even though the flags are set to true.

akva2 · 2024-05-27T11:56:32Z

at the core the problem is very simple: he built a specific target, not the all target, and this specific target does not get a dependency on the opm_embedded target set. one could argue that is the correct behavior but i guess it can be seen as a little inconvenient.

lisajulia · 2024-05-27T12:00:05Z

at the core the problem is very simple: he built a specific target, not the all target, and this specific target does not get a dependency on the opm_embedded target set. one could argue that is the correct behavior but i guess it can be seen as a little inconvenient.

ok - well I'd say opmcommon_python should be built in any case if the flag OPM_ENABLE_EMBEDDED_PYTHON (which implies OPM_ENABLE_PYTHON) is set to true, no?

akva2 · 2024-05-27T12:01:13Z

and it will be if you build the all target. but he specifically said only build flow_blackoil.

blattms · 2024-05-27T12:49:12Z

opm_embedded is just there and not built. Library opmcommon_python needs library opmcommon. For this we have a dependency of opmcommon_python on opmcommon. Now it seems that your superbuild would need to know that library opmsimulators depends on both opmcommon_python and opmcommon.

There seems to be no way to resolve this via dependencies within opm-common. (Assuming circular dependencies are evil and we cannot have opmcommon depend on opmcommon_python in addition).

I am not 100% sure how your superbuilds work. The only clean solution seems to be to have another exported target within the build system of opm-common for library opmcommon_python and makes sure that library opmsimulators depends on this, too. This will be some work.

akva2 · 2024-05-27T12:54:37Z

the super-build just does add_subdirectory() on each module, https://github.com/OPM/opm-utilities/blob/master/opm-super/CMakeLists.txt
you simply do

if(TARGET opm_embedded)
  add_dependencies(flow_blackoil opm_embedded)
endif()

in the opm-simulators buildsystem, just like e.g. https://github.com/OPM/opm-simulators/blob/master/CMakeLists.txt#L643

so there is not really any voodoo required. the question is, as I said, whether this is a correct change or not.

lisajulia requested review from bska and blattms April 17, 2024 11:50

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from ccc241b to e97bc99 Compare April 17, 2024 11:51

lisajulia added this to the Release 2024.04 milestone Apr 17, 2024

lisajulia changed the title ~~Fix/py action separate python bindings and opm embedded~~ Fix/pyAction: Separate classes for python bindings and opm embedded Apr 17, 2024

lisajulia mentioned this pull request Apr 17, 2024

Add documentation #4018

Open

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from e97bc99 to 39387f5 Compare April 17, 2024 14:45

bska approved these changes Apr 17, 2024

View reviewed changes

hakonhagland reviewed Apr 17, 2024

View reviewed changes

python/cxx/export.cpp Outdated Show resolved Hide resolved

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch 2 times, most recently from 7df6d2f to 2f6f260 Compare April 18, 2024 05:39

akva2 reviewed Apr 18, 2024

View reviewed changes

python/cxx/eclipse_config.cpp Outdated Show resolved Hide resolved

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from 95c13af to 6f22c00 Compare April 18, 2024 07:10

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from 6f22c00 to 006c283 Compare April 18, 2024 11:25

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from 006c283 to 085fdf2 Compare April 22, 2024 15:13

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch 3 times, most recently from 4b98e51 to 8972bb9 Compare April 23, 2024 16:18

Create the opm_embedded from the same shared library that is used to …

b1a39fc

…create opm.common.* Now, there are two modules created - opmcommon_python - opm_embedded, as embedded Python module, needed for PYACTION and PYINPUT

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from d99e29b to 1590f66 Compare April 29, 2024 06:44

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from 1590f66 to a41887b Compare April 29, 2024 07:37

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from a41887b to 2589a48 Compare April 30, 2024 08:42

Add file and entry in README.md for autocompletion in VS Code

d2f45c5

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from 2589a48 to d2f45c5 Compare April 30, 2024 08:46

blattms merged commit 2a34fc4 into OPM:master Apr 30, 2024
1 check passed

lisajulia deleted the fix/pyAction-separate-python-bindings-and-opm-embedded branch April 30, 2024 11:52

Fix/pyAction: Separate classes for python bindings and opm embedded #4017

Fix/pyAction: Separate classes for python bindings and opm embedded #4017

Conversation

lisajulia commented Apr 17, 2024 • edited

lisajulia commented Apr 17, 2024 • edited

lisajulia commented Apr 17, 2024

lisajulia commented Apr 17, 2024

bska left a comment

Choose a reason for hiding this comment

bska Apr 17, 2024

Choose a reason for hiding this comment

lisajulia Apr 18, 2024 • edited

Choose a reason for hiding this comment

lisajulia commented Apr 18, 2024

blattms commented Apr 18, 2024 • edited by bska

blattms commented Apr 18, 2024

lisajulia commented Apr 18, 2024

blattms commented Apr 18, 2024

lisajulia commented Apr 18, 2024

lisajulia commented Apr 18, 2024

lisajulia commented Apr 22, 2024

lisajulia commented Apr 22, 2024

lisajulia commented Apr 26, 2024

lisajulia commented Apr 26, 2024

lisajulia commented Apr 29, 2024

lisajulia commented Apr 29, 2024

blattms commented Apr 29, 2024

lisajulia commented Apr 29, 2024

blattms commented Apr 29, 2024

lisajulia commented Apr 30, 2024

lisajulia commented Apr 30, 2024

blattms commented Apr 30, 2024

GitPaean commented May 27, 2024

lisajulia commented May 27, 2024

GitPaean commented May 27, 2024

lisajulia commented May 27, 2024

akva2 commented May 27, 2024

GitPaean commented May 27, 2024

lisajulia commented May 27, 2024

akva2 commented May 27, 2024

lisajulia commented May 27, 2024

akva2 commented May 27, 2024

blattms commented May 27, 2024 • edited

akva2 commented May 27, 2024 • edited

lisajulia commented Apr 17, 2024 •

edited

lisajulia commented Apr 17, 2024 •

edited

lisajulia Apr 18, 2024 •

edited

blattms commented Apr 18, 2024 •

edited by bska

blattms commented May 27, 2024 •

edited

akva2 commented May 27, 2024 •

edited