Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate Python bindings with CFFI in API mode #2

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

robertodr
Copy link

As discussed offline with @bast, this is supposedly the way to do Python bindings with CFFI.
Browsing the docs this has various advantages:

  1. No need to pass around environment variables, which should reflect in easier
    packaging and deployment but also less brittle usage patterns.
  2. Faster, because the bindings are already compiled into a Python module,
    rather than being generated on-the-fly from a header and a shared library.
  3. Less bug-prone, for the same reason as point 2.
  4. Allow to pass functions defined in Python to C (Fortran) as function
    pointers.

This is still kind of rough around the edges, but I am hoping we can finesse it
after some discussion. In particular, I think the packaging example should
become part of the CMake Cookbook repo in the not-so-distant future.

What's done

  • I created a builder.py script that reads in account.h and feeds it to
    CFFI to generate an _account.c file with the Python bindings. This file
    looks really ugly, but that's beyond the point. It is compiled, together
    with account.f90 into a Python module. Note that the CMake is
    significantly more involved now and I've used 3.14. The new
    FindPython.cmake (available since 3.12) is so much nicer to work with
    and I am doing symlinks of files (available since 3.14) quite extensively.
    The library is named similarly to what pybind11 generates and can be imported directly.
    It defines two things:

    • lib which are our API functions, and
    • ffi which is a helper from CFFI to do type casts and some decorators.
  • The shim.py file does some Python-inification of the C-style interface.
    In this case I went "bananas" and bound the flat interface to a dataclass in
    a resource-acquisition-is-initialization (RAII) fashion:

    • The class holds a _context. This is private, given the starting underscore.
    • __post_init__ calls lib.account_new.
    • __del__ calls lib.account_free.
  • Finally, __init__.py does its usual thing.

@bast
Copy link
Member

bast commented Jan 9, 2020

Thanks for the work! Just browsed the diffs. I am trying to summarize for my own understanding:

Advantages:

  • avoid environment variables
  • memory management more Pythonic (no manual allocation/deallocation)
  • allows passing function pointers
  • possibly faster

Cost:

  • more involved CMake code
  • functions are defined on both sides (Python-side and on C-side), previously the Python-side was generated on the fly (not 100% correct since we anyway redefined them for more Pythonic look)

@robertodr
Copy link
Author

Yes, your analysis is largely correct. I disagree with the second point in your "Cost" section though. In principle, only account.h has to be hand-written and you can use the functions with:

import account

acct = account._account.lib.account_new()

The business with the shim.py file is to make use of the library a) less verbose, and b) more Pythonic (especially the class-like RAII rewrapping) but both a) and b) are completely optional.

@bast
Copy link
Member

bast commented Jan 9, 2020

That's great. Thanks!

@bast
Copy link
Member

bast commented Jan 9, 2020

Should I help out with the testing part?

@robertodr
Copy link
Author

Yes, please :) I don't know if we want to be able to run through CTest. I tried and failed to do so, but the failure confuses me, as it should work.

@bast
Copy link
Member

bast commented Jan 10, 2020

I am looking into it.

@bast
Copy link
Member

bast commented Jan 10, 2020

The Linux failure is because the Travis script is trying to pip install a different version of the code (latest master version on the central repo). In other words that test is useful but ill defined anyway for future pull requests.

Now looking into the macOS testing breakage.

@bast
Copy link
Member

bast commented Jan 10, 2020

I got further, have fixed two things and will send them as PR to the forked branch. But one problem still remains: it seems macOS does not see CFFI in the activated conda environment.

@robertodr
Copy link
Author

I am an idiot 🤦‍♂️ In the install step I am downloading the Linux installer also on macOS... There needs to be an if-else to download either from Miniconda3-latest-Linux-x86_64.sh or Miniconda3-latest-MacOSX-x86_64.sh.

@bast
Copy link
Member

bast commented Jan 10, 2020

I am an idiot In the install step I am downloading the Linux installer also on macOS... There needs to be an if-else to download either from Miniconda3-latest-Linux-x86_64.sh or Miniconda3-latest-MacOSX-x86_64.sh.

I have already fixed that on a branch. I will send PR to your branch but had no time until now but also then it fails later but we can look at that later. I will clean up my patches and send them.

bast added 2 commits January 10, 2020 19:13
now we use the actual setup.py instead of an outdated setup.py
on a possibly different branch
@bast
Copy link
Member

bast commented Jan 10, 2020

Some problems remain:

  • Travis complains about too low CMake version during the pip install test. This is because I have deactivated the Conda environment. We should probably create one with only Python and CMake and derive from it another one that contains the dev dependencies.
  • Even with latest CMake import account after pip install fails (seen on my laptop).
  • I have seen macOS failing during CMake build because it could not see CFFI. Is it because CMake detected the wrong Python environment?

@bast
Copy link
Member

bast commented Jan 10, 2020

This is what is seen after local pip install:

>>> import account
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/user/venv/lib/python3.7/site-packages/account/__init__.py", line 1, in <module>
    from .shim import *
  File "/home/user/venv/lib/python3.7/site-packages/account/shim.py", line 4, in <module>
    from ._account import lib
ModuleNotFoundError: No module named 'account._account'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants