surfe icon indicating copy to clipboard operation
surfe copied to clipboard

add surfe to conda forge

Open AndrewAnnex opened this issue 4 years ago • 11 comments

I would like to add surfe to the conda-forge ecosystem. This should be relatively easy to do once a release has been made of surfe on github, as the conda-forge recipes really recommend having relying on versioned release files. Afterwards it should be easy to follow the workflow in the github action to get surfe built in conda.

The advantage of conda-forge for this would be the ability to compile the project in a isolated environment for users without access to root user environments as well as more robust macos/linux/windows builds, as well as integration with other python projects.

Again the first step will be to make the current in draft release finalized and then the feedstock recipe can be made.

AndrewAnnex avatar Jun 25 '20 13:06 AndrewAnnex

@AndrewAnnex great idea! @MichaelHillier are you keen to help? This would let me add surfe as an interpolator in LoopStructural which would be awesome for people being able to use it!

lachlangrose avatar Jun 26 '20 08:06 lachlangrose

Yes this is a great idea. Will start looking into this this weekend.

MichaelHillier avatar Jun 26 '20 12:06 MichaelHillier

Well time flies....

I spent a bit of time this afternoon looking at how to pip install surfe. I found a way of calling cmake from setup.py using this example . I have created a pull request for this #6

@MichaelHillier if you want I can setup a pypi action on github so that the wheels are automatically generated and then sent to pypi. We just need a release to be made for surfe...

@AndrewAnnex - what is needed for setting up a condaforge recipe? If we can run python setup.py install, it should be easy?

lachlangrose avatar Jul 21 '20 06:07 lachlangrose

Sorry all for the delay. I've been swamped lately on multiple fronts. I've added a release (v1.0) for surfe 3 weeks ago.

@lachlangrose Thanks very much for getting a setup.py configured to work with cmake so that the binaries can get built. I will test the pull request tomorrow hopefully. If you can setup a pypi action that would be great if you can, do you need anything from me to do this?

MichaelHillier avatar Jul 22 '20 16:07 MichaelHillier

@lachlangrose the docs for conda-forge (https://conda-forge.org/docs/maintainer/adding_pkgs.html) have more details on what is needed for the recipe, but my instinct is that the C++ library for surfe would use cmake to build it instead of the steps in setup.py. I don't know enough about cmake to figure out what is needed in the conda forge recipe.

AndrewAnnex avatar Jul 27 '20 17:07 AndrewAnnex

but my instinct is that the C++ library for surfe would use cmake to build it instead of the steps in setup.py

My two pences (sorry for that) : if there is the need to use the C++ library on its own (i.e. as standalone C++ library) it could be more meaningfull to have two separate projetcts (surfe on one side and surfepy on the other side), the python bindings project depending on the C++ library. This way each project would come with "its logic" (and all related stuff... tests to begin with).

Simon-Lopez avatar Jul 29 '20 20:07 Simon-Lopez

Yeah that would my expectation, it depends on how surfe is intended to be used, @MichaelHillier what is your thinking?

-Andrew Annex

On Jul 29, 2020, at 4:29 PM, Simon-Lopez [email protected] wrote:

 but my instinct is that the C++ library for surfe would use cmake to build it instead of the steps in setup.py

My two pences (sorry for that) : if there is the need to use the C++ library on its own (i.e. as standalone C++ library) it could be more meaningfull to have two separate projetcts (surfe on one side and surfepy on the other side), the python bindings project depending on the C++ library. This way each project would come with "its logic" (and all related stuff... tests to begin with).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

AndrewAnnex avatar Jul 29 '20 20:07 AndrewAnnex

My view is it depends on what the goal of the surfepy package is. If it stays as it is and is as just direct bindings between the c++ library for python then I don't see any reason to change that. In LoopStructural I have added a wrapper to surfe so that you can interpolate surfaces using surfe https://github.com/Loop3D/LoopStructural/blob/master/LoopStructural/interpolators/surfe_wrapper.py this means that interpolating surfaces can be prepared by LoopStructural and then passed to the c++ library.

Regardless I think the current c++/pybind project should be kept as the one and then if you want to add extra python functionalities this should be done in another library.

Currently, the setup.py file is just calling the cmake for the project, so cmake could be run without python. We could add cmake options to build a binary that can run from csvs instead of the python bindings...

I just tested building a wheel and then pip installing the wheel and it works - I'll try and add this to the github action and then we can add it to pypi.

lachlangrose avatar Jul 29 '20 21:07 lachlangrose

There could be two options: 1) a stand alone C++ library - surfe 2) python version of surfe that uses the pybind11 bindings which depend on the C++ library. This can be easily achieved by just adding an cmake option that will add the surfepy project to the build if the needed. Currently both are always built. However, since as @lachlangrose mentioned surfepy is just bindings between C++ and python and there is no additional complications (for my perspective at least) in always building it. So for now I will probably leave as is if unless people are experiencing difficulties with how it's currently built.

The setup.py that @lachlangrose put together still uses cmake to setup the build, but automates the entire process of building the distribution. My understanding is this will allow for the surfepy package to be published to pypi or conda-fonge through GitHub actions when new releases of surfe are pushed to the repo.

MichaelHillier avatar Jul 29 '20 21:07 MichaelHillier

@MichaelHillier my understanding is that it should be possible to implement both of those options you give in a single codebase (no extra repositories) possibly requiring very little change. In looking at @lachlangrose did for cmake my impression is that is highly non standard, for example it requires using subprocess directly. I think that tutorial is redundant because pybind11 provides the functionality in setup.py directly (https://pybind11.readthedocs.io/en/stable/compiling.html#building-with-setuptools). My understanding would be that the cmake recipe could be changed to not build the python module, but the setup.py file would be updated to follow the pybind11 setuptools functionality would be used instead (also see https://github.com/pybind/python_example).

So in short, I think we are all roughly on the same page, but we may different understandings about cmake/pybind11/setuptools/conda-forge do.

Just so there is clarity, once the pybind11 setuptools method is working correctly it should allow the conda-forge to directly build without additional github actions in this repository.

edit: I did not fully read the code in #6 until now, I have not seen scikit-build before so maybe it is easier than the pybind11 setuptools method?

AndrewAnnex avatar Jul 29 '20 23:07 AndrewAnnex

indeed: https://github.com/scikit-build/scikit-build-sample-projects/tree/master/projects/hello-pybind11

AndrewAnnex avatar Jul 29 '20 23:07 AndrewAnnex