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?

Reason: There is only one OpmLog per simulation.

lisajulia · 2024-04-26T07:54:48Z

jenkins build this please

lisajulia · 2024-04-26T11:20:33Z

jenkins build this please

lisajulia · 2024-04-26T12:30:55Z

jenkins build this please

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.

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

lisajulia added 4 commits April 26, 2024 09:25

Put the Python SimulationConfig class in its own file

092aaa1

Add documentation string to the OpmLog class

0f54822

Use a shared pointer for the OpmLog python class

6f1a00d

Reason: There is only one OpmLog per simulation.

Fix typo in connection.cpp

2a547e1

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from 70e0b9b to 4f81718 Compare April 26, 2024 07:26

lisajulia mentioned this pull request Apr 26, 2024

Set the PYTHONPATH for the tests using python OPM/opm-simulators#5306

Closed

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from 4f81718 to c0f290b Compare April 26, 2024 11:19

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from c0f290b to a84cbcb Compare April 26, 2024 12:30

lisajulia force-pushed the fix/pyAction-separate-python-bindings-and-opm-embedded branch from a84cbcb to d99e29b Compare April 26, 2024 13:31

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

bska Apr 17, 2024

lisajulia Apr 18, 2024 •

edited

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 26, 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

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 26, 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

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