Bind to Python with pybind11 #96

robertodr · 2019-10-26T12:36:27Z

Description

I've redone the Python bindings using pybind11. The bindings are now more transparent and easier to read/extend than the SWIG bindings were.

If pybind11 is not around, it will be fetched. This trick requires use of CMake 3.11 at least.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Questions

Can we tag 2.0.0 when this is approved and merge to master?

Status

Ready to go

codecov-io · 2019-10-27T22:05:47Z

Codecov Report

Merging #96 into master will not change coverage.
The diff coverage is 75%.

@@           Coverage Diff           @@
##           master      #96   +/-   ##
=======================================
  Coverage   49.46%   49.46%           
=======================================
  Files          77       77           
  Lines        1785     1785           
=======================================
  Hits          883      883           
  Misses        902      902

Impacted Files	Coverage Δ
src/xcfun.cpp	`71.99% <75%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca137ba...db470d4. Read the comment docs.

stigrj · 2019-10-29T07:58:03Z

./setup --pybindings

gives the following error

CMake Error at python/CMakeLists.txt:8 (find_package):
  Could not find a package configuration file provided by "pybind11"
  (requested version 2.3.0) with any of the following names:

    pybind11Config.cmake
    pybind11-config.cmake

  Add the installation prefix of "pybind11" to CMAKE_PREFIX_PATH or set
  "pybind11_DIR" to a directory containing one of the above files.  If
  "pybind11" provides a separate development package or SDK, be sure it has
  been installed.

robertodr · 2019-10-29T08:48:01Z

Yeah, you need to have pybind11 installed. I still have to set up the superbuild.

…

On Tue, Oct 29, 2019, 08:58 Stig Rune Jensen ***@***.***> wrote: ./setup --pybindings gives the following error CMake Error at python/CMakeLists.txt:8 (find_package): Could not find a package configuration file provided by "pybind11" (requested version 2.3.0) with any of the following names: pybind11Config.cmake pybind11-config.cmake Add the installation prefix of "pybind11" to CMAKE_PREFIX_PATH or set "pybind11_DIR" to a directory containing one of the above files. If "pybind11" provides a separate development package or SDK, be sure it has been installed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#96?email_source=notifications&email_token=AA4JOEMBJAQ6NAHJITXH74TQQ7UIXA5CNFSM4JFNDZWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECPSC2Q#issuecomment-547299690>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4JOEKSKYB34PJD33EOJLDQQ7UIXANCNFSM4JFNDZWA> .

This bumps CMake version requirement up to 3.11

stigrj · 2019-10-29T14:45:11Z

Each download failed!

    error: downloading 'https://github.com/pybind/pybind11/archive/v2.3.0.tar.gz' failed
         status_code: 1
         status_string: "Unsupported protocol"
         log:
         --- LOG BEGIN ---
         Protocol "https" not supported or disabled in libcurl

  Closing connection -1

robertodr · 2019-10-29T14:47:15Z

Ehm... It works on Travis 🤷‍♂️ Where are you trying to build?

stigrj · 2019-10-29T14:48:39Z

~~It's the same~~

./setup --pybindings

...sorry, I read "what"...

I'm building on my Ubuntu

robertodr · 2019-10-29T14:54:03Z

I cannot reproduce it. Can you try to cURL pybind11 by hand?

stigrj · 2019-10-29T15:11:34Z

I'm able to run the command in .ci/pybind11.sh

curl -Ls https://github.com/pybind/pybind11/archive/v${pybind11_VERSION}.tar.gz | tar -xz -C pybind11 --strip-components=1

Is this the same command FetchContent_Populate runs?

robertodr · 2019-10-29T15:14:31Z

I don't think it's the exact same, but something similar.

robertodr · 2019-10-29T15:15:12Z

Googling turned this one up: https://github.com/ruslo/hunter/issues/972#issuecomment-323687499

stigrj · 2019-10-29T15:23:58Z

Argh, the annoyance

robertodr · 2019-11-08T08:29:33Z

Gentle nudge here. It'd be good to get this merged so I can move on with conda packaging and redoing the Fortran interface with iso_c_binding

bast · 2019-11-08T08:58:58Z

I will review today ...

robertodr · 2019-11-08T09:06:20Z

Thanks 🐙

bast

Thumbs up from me but before we merge we need Andre and Christoph to comment since they depend most on the SWIG interface which would be removed by this merge.

I also agree that we target a v2.0.0 with this merge but would like if we test it a bit first. In other words this would be v2.0.0-beta or v2.0.0-rc.

chjacob-tubs · 2019-11-08T12:00:14Z

Sorry, was busy / travelling the last two weeks. I will try to test over the weekend.

robertodr · 2019-11-08T12:47:28Z

Yes, this should be a beta release. We can do rc when the Fortran bindings are refurbished.

chjacob-tubs · 2019-11-10T12:14:58Z

python/export_xcfun.cpp

+
+  m.def("xcfun_version", &xcfun_version, "XCFun version");
+  m.def("xcfun_splash", &xcfun_splash, "XCFun splash screen");
+


Could you please add
m.def("xcfun_test", &xcfun_test, "XCFun self test");

We are calling xcfun_test from the PyADF test suite to make sure xcfun is working correctly.

Thanks for the review. Good catch! pybind11 won't generate binding code unless you explicitly define the function/class to be bound.

chjacob-tubs

Looks good, except for my comment everything is working fine in PyADF.

chjacob-tubs · 2019-11-10T13:43:23Z

It works on my Mac, but I somehow dont get it to work on Ubuntu. Setup now fails with
CMake Error: Could not open file for write in copy operation /home/christoph/sources/xcfun/build/include/XCFun/XCFunExport.h.tmp CMake Error: : System Error: Not a directory CMake Error at /home/christoph/sources/cmake-3.15-install/share/cmake-3.15/Modules/GenerateExportHeader.cmake:394 (configure_file): configure_file Problem configuring file Call Stack (most recent call first): /home/christoph/sources/cmake-3.15-install/share/cmake-3.15/Modules/GenerateExportHeader.cmake:410 (_do_generate_export_header) src/CMakeLists.txt:4 (generate_export_header)

Any idea what the problem might be?

robertodr · 2019-11-10T13:47:32Z

Yes, I made a silly CMake mistake. Should be fixed in the latest commit.

chjacob-tubs · 2019-11-10T13:54:18Z

OK, now it works! (One minor issue: It seems that CMake does not discover the system-installed pybind11 but always downloads and builds itself. Thats fine for me, I actually dont know how / where CMake is looking for pybind11)

robertodr · 2019-11-10T13:57:32Z

How was pybind11 installed? I can look at this later next week.

chjacob-tubs · 2019-11-10T14:01:29Z

On Ubuntu both with apt-get and with pip, an the Mac with macports

robertodr · 2019-11-10T14:21:43Z

The PyPI package does not install the CMake support files. The package from apt-get might be too old? What version is it?

chjacob-tubs · 2019-11-10T15:21:19Z

OK, I got it to work, I guess that was the problem.

chjacob-tubs · 2019-11-13T12:27:45Z

Should we merge this now? How are you handling this / who is pushing the button?

robertodr · 2019-11-13T13:53:22Z

Sorry, should have been clearer. I wanted to give @aspgomes one more week for comments, then anyone with write access can push the button (I usually prefer not to self-merge)

robertodr added 4 commits October 27, 2019 22:56

Prepare infrastructure for pybind11

1878bdb

Remove SWIG files

fa45b47

Remove unused CMake files

515a597

Install pybind11 on Travis

e61146d

robertodr force-pushed the pybind11 branch from 26ebfe5 to e61146d Compare October 27, 2019 21:56

robertodr marked this pull request as ready for review October 27, 2019 21:58

robertodr requested review from bast and chjacob-tubs October 27, 2019 21:58

robertodr requested review from aspgomes and stigrj October 28, 2019 09:10

robertodr added 4 commits October 29, 2019 15:19

Set up automatic fetching of pybind11

1f106f4

This bumps CMake version requirement up to 3.11

Exercise both superbuild and build

ff269ca

Update to xenial

a85d52e

Install decent CMake by hand

06f077e

bast approved these changes Nov 8, 2019

View reviewed changes

chjacob-tubs reviewed Nov 10, 2019

View reviewed changes

chjacob-tubs requested changes Nov 10, 2019

View reviewed changes

robertodr and others added 3 commits November 10, 2019 13:55

Implement @chjacob-tubs suggestions

09ee6cf

Fix compilation error on Travis

8eb853b

More fixes for compilation

db470d4

chjacob-tubs approved these changes Nov 10, 2019

View reviewed changes

aspgomes merged commit dd662ae into dftlibs:master Nov 25, 2019

robertodr mentioned this pull request Jan 17, 2020

Suggestion: Port Python interface from SWIG to either CFFI or pybind11 #44

Closed


		m.def("xcfun_version", &xcfun_version, "XCFun version");
		m.def("xcfun_splash", &xcfun_splash, "XCFun splash screen");

Bind to Python with pybind11 #96

Bind to Python with pybind11 #96

Uh oh!

Conversation

robertodr commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Questions

Status

Uh oh!

codecov-io commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stigrj commented Oct 29, 2019

Uh oh!

robertodr commented Oct 29, 2019 via email

Uh oh!

stigrj commented Oct 29, 2019

Uh oh!

robertodr commented Oct 29, 2019

Uh oh!

stigrj commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertodr commented Oct 29, 2019

Uh oh!

stigrj commented Oct 29, 2019

Uh oh!

robertodr commented Oct 29, 2019

Uh oh!

robertodr commented Oct 29, 2019

Uh oh!

stigrj commented Oct 29, 2019

Uh oh!

robertodr commented Nov 8, 2019

Uh oh!

bast commented Nov 8, 2019

Uh oh!

robertodr commented Nov 8, 2019

Uh oh!

bast left a comment

Choose a reason for hiding this comment

Uh oh!

chjacob-tubs commented Nov 8, 2019

Uh oh!

robertodr commented Nov 8, 2019

Uh oh!

chjacob-tubs Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

robertodr Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

chjacob-tubs left a comment

Choose a reason for hiding this comment

Uh oh!

chjacob-tubs commented Nov 10, 2019

Uh oh!

robertodr commented Nov 10, 2019

Uh oh!

chjacob-tubs commented Nov 10, 2019

Uh oh!

robertodr commented Nov 10, 2019

Uh oh!

chjacob-tubs commented Nov 10, 2019

Uh oh!

robertodr commented Nov 10, 2019

Uh oh!

chjacob-tubs commented Nov 10, 2019

Uh oh!

chjacob-tubs commented Nov 13, 2019

Uh oh!

robertodr commented Nov 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

robertodr commented Oct 26, 2019 •

edited

Loading

codecov-io commented Oct 27, 2019 •

edited

Loading

stigrj commented Oct 29, 2019 •

edited

Loading