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

RFC: Tracking progress to integrate with SciPy #112

Open
HaoZeke opened this issue Nov 25, 2023 · 19 comments
Open

RFC: Tracking progress to integrate with SciPy #112

HaoZeke opened this issue Nov 25, 2023 · 19 comments
Labels
python Issues related to the Python interface or implementation scipy Issues related to SciPy

Comments

@HaoZeke
Copy link

HaoZeke commented Nov 25, 2023

As noted over at SciPy, I'll be able to spend some time ensuring the integration of PRIMA into SciPy via Python-C extension module. :)

The first step is to have a meson build backend. This is needed also because the goal is to use prima as a subproject, e.g. as is done for HiGHs.

@nbelakovski
Copy link
Contributor

Sounds great! Do you want to take a stab at adding it or would you like me to make an initial branch with meson and go from there? I took a look at meson and I have a branch that basically just builds the necessary library, but it's pretty basic.

We will want to publish our own Python package in addition to providing PRIMA via SciPy, so the bulk of this work ought to be usable for both use cases.

We currently have both CMake and Make build systems that were contributed by two different people (@jschueller and @zaikunzhang, respectively). I think we will want to consolidate to at most 2 or better yet even 1, but that work can go on in parallel to the work necessary for SciPy or afterwards.

@HaoZeke
Copy link
Author

HaoZeke commented Nov 26, 2023

Sounds great! Do you want to take a stab at adding it or would you like me to make an initial branch with meson and go from there? I took a look at meson and I have a branch that basically just builds the necessary library, but it's pretty basic.

Awesome, I will probably take a stab at makes for an easy onboarding task for me to start delving into the code.

We will want to publish our own Python package in addition to providing PRIMA via SciPy, so the bulk of this work ought to be usable for both use cases.

If the idea is to provide bindings within PRIMA, then I wonder if it would be preferable (HiGHS does this) to have a C++ interface and use Pybind11. For SciPy alone I would have suggested (and was planning to implement) a Python-C handwritten extension, because much of the user interface comes from SciPy.

The reason to prefer a C++ binding followed by Pybind11 over a pure Python-C extension is just that it would be easier to also integrate with other C++ libraries (it would be just a minor OOP wrapper over the C code). It will of course depend on the exact scoping of what level of interface is to be provided from PRIMA / pyPRIMA (assumed name) and what interface is OK to farm out to SciPy.

We currently have both CMake and Make build systems that were contributed by two different people (@jschueller and @zaikunzhang, respectively). I think we will want to consolidate to at most 2 or better yet even 1, but that work can go on in parallel to the work necessary for SciPy or afterwards.

Generally meson is simpler, but there are edge cases where CMake still holds the advantage (but those are reducing). From a maintenance perspective I agree having one build system makes sense, but that as noted can be done later :)

@nbelakovski
Copy link
Contributor

I've used pybind11 before and I appreciate how easy it makes everything. I haven't made a Python-C extension so I can't really compare and contrast on that front, but we do generally want to provide bindings to as many languages as possible, which obviously includes C++. If you're suggesting using pybind11 for both the SciPy work as well as for our own Python packages I'm definitely on board, I think it would make things a lot simpler.

@HaoZeke
Copy link
Author

HaoZeke commented Nov 26, 2023

I've used pybind11 before and I appreciate how easy it makes everything. I haven't made a Python-C extension so I can't really compare and contrast on that front, but we do generally want to provide bindings to as many languages as possible, which obviously includes C++.

Yup, Pybind11 is great, writing a Python-C extension by hand is just a lot more fragile and verbose.

If you're suggesting using pybind11 for both the SciPy work as well as for our own Python packages I'm definitely on board, I think it would make things a lot simpler.

Awesome, then the plan will be:

  • Meson build
  • C++ wrapper over ISO_C interface
  • Pybind11 wrappers to C++ interface
  • Additional Python features as needed for SciPy (e.g. callback support)

@HaoZeke HaoZeke changed the title ENH: Add meson as a build system RFC: Tracking progress to integrate with SciPy Nov 26, 2023
@nbelakovski
Copy link
Contributor

Yup, Pybind11 is great, writing a Python-C extension by hand is just a lot more fragile and verbose.

Just wondering, since we both seem to like pybind11, why did you suggest Python-C extension in the first place? Are there some advantages to it compared to pybind11 or disadvantages of pybind11 compared to Python-C extension?

I will start working on your last 3 bullet points. Callback support is implemented in a branch, it's just waiting for the options PR to merge since it's built on top of it. How long do you think it will take you to get the meson build going? Just looking for a rough idea here not necessarily an exact date.

@HaoZeke
Copy link
Author

HaoZeke commented Nov 27, 2023

Yup, Pybind11 is great, writing a Python-C extension by hand is just a lot more fragile and verbose.

Just wondering, since we both seem to like pybind11, why did you suggest Python-C extension in the first place? Are there some advantages to it compared to pybind11 or disadvantages of pybind11 compared to Python-C extension?

Well, for some projects, having an intermediate call through C++ might be too much of an overhead. In theory, there are probably some more micro optimizations possible if one writes hand-written Python-C code. Even Cython is yet another attempt to not write Python-C wrappers by hand just because of the same fragility. SciPy itself has some light Python-C wrappers (e.g. for minpack) and f2py also generates Python-C wrappers, so that's why it was my first suggestion. However, we now do have C++/Pybind11 stuff too (e.g. Highs) and since you mentioned C++ support is a goal, then it really is just more easily maintained at the C++ / Pybind11 level so I think that's better here.

I will start working on your last 3 bullet points. Callback support is implemented in a branch, it's just waiting for the options PR to merge since it's built on top of it. How long do you think it will take you to get the meson build going? Just looking for a rough idea here not necessarily an exact date.

I expect it to be done by next Monday, I'll start it on Friday (I have limited time until the weekend).

@jschueller
Copy link
Collaborator

jschueller commented Nov 27, 2023

while pybind11 is great, I think it is overkill for prima because it will be just one function now (#114 ); the ctypes wrapper is quite simple and requires no compilation on top of the C interface, see #52

@HaoZeke
Copy link
Author

HaoZeke commented Nov 28, 2023 via email

@zaikunzhang
Copy link
Member

zaikunzhang commented Nov 28, 2023

Hi, @jschueller , @nbelakovski , @HaoZeke , thank you very much for your precious contributions. Sorry for missing the discussion here. Nickolai @nbelakovski is visiting me in Hong Kong, and I have told him my opinions --- nothing technical (as I know almost nothing about the technology involved), just some general comments on how things should be organized (in the technical/coding sense). Many thanks!

@nbelakovski
Copy link
Contributor

@HaoZeke I've implemented pybind11 bindings for newuoa in a branch: https://github.com/nbelakovski/prima/tree/python_binding.

To use it you'd do the following, assuming you are starting in the repo root (I can add these to a README if we go this direction)

mkdir build
cd build
cmake ..
make -j8
export PYTHONPATH=$(pwd)/python
cd ../python/examples
python example_newuoa.py

We could do either pybind11 or ctypes. I would tend to prefer something compiled since I think that means the compiler is doing some more checks for us. Also with pybind11 we can compile everything into a single shared library to distribute around, whereas with ctypes we need to distribute both the compiled library and the python file on top of it.

@HaoZeke
Copy link
Author

HaoZeke commented Dec 1, 2023 via email

@nbelakovski
Copy link
Contributor

@HaoZeke Just FYI I just pushed an update to that branch that includes the unified interface that was recently merged. I have 3/5 algorithms working through the python bindings, just need to do LINCOA and COBYLA and I hope to finish those this week. Once that's done I may start working on the meson stuff myself since we'll need it for publishing our own python package anyway.

@nbelakovski
Copy link
Contributor

@HaoZeke We just merged the callback PR and I've made a lot of progress on the python bindings. It works for all 5 algorithms, I just have two outstanding items: COBYLA needs to be able to accept a list that can include LinearConstraint and NonlinearConstraint, and I'm seeing a GIL related issue that I need to debug.

However, I'm having a really hard time getting the python bindings to compile correctly on Windows. It seems that the python module itself needs to be compiled with MSVC, which does not include support for Fortran. And CMake does not like mixing MSVC and gfortran, although I'm installing the intel compiler now so we'll see if that is any better. Do you have any advice or suggestions here? Do we need to use a specific compiler? Do these problems go away upon switching to Meson?

@jschueller
Copy link
Collaborator

yes it seems there could be a limitation with msvc not liking mixing c and fortran sources, but that's not specific to cmake, a simple solution is to split fortran and c libs

@nbelakovski
Copy link
Contributor

I managed to find a solution by copying the compile/link flags used by SciPy. It turns out you can compile Python modules on Windows with GCC, you just have to link ucrt statically (and a couple other flags for C). I also wasn't sure what the right path was since we have so many Fortran compilers that we support, but after talking about it with @zaikunzhang we agreed that since we don't expect users to be building the Python bindings themselves (for the most part) we can just pick one compiler to support the Python bindings. I think gfortran makes sense since that's how SciPy is doing things on Windows (and I couldn't quite get ifx and MSCV to work :D ).

@eli-schwartz
Copy link

Using the Intel compiler is probably not a good solution since that introduces distribution complexity. You want a FOSS compiler for this so that you can guarantee you're legally able to upload prebuilt binaries (e.g. python wheels).

I also wasn't sure what the right path was since we have so many Fortran compilers that we support, but after talking about it with @zaikunzhang we agreed that since we don't expect users to be building the Python bindings themselves (for the most part) we can just pick one compiler to support the Python bindings.

The build system should really be agnostic to this, why do you need the path?

@nbelakovski
Copy link
Contributor

so that you can guarantee you're legally able to upload prebuilt binaries

Good point

why do you need the path?

I didn't mean path as in system path, I meant it as in "path forward"

@eli-schwartz
Copy link

Ah, sorry for the confusion then. :P

@zaikunzhang zaikunzhang added python Issues related to the Python interface or implementation scipy Issues related to SciPy labels Jan 9, 2024
@nbelakovski
Copy link
Contributor

@HaoZeke Good news! The Python bindings are merged 🥳

I started working on the Meson setup here: https://github.com/nbelakovski/prima/tree/meson

It doesn't include the python bindings yet, I was waiting until they were merged so that I didn't have to keep track of a branch on top of a branch. Do you have bandwidth to carry them through or should I keep going on them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Issues related to the Python interface or implementation scipy Issues related to SciPy
Projects
None yet
Development

No branches or pull requests

5 participants