search

bbopt / nomad

NOMAD - A blackbox optimization software
https://nomad-4-user-guide.readthedocs.io/
GNU Lesser General Public License v3.0
110 stars 24 forks source link

Experimental support for statically linked NOMAD #126

Closed jan-provaznik closed 7 months ago

jan-provaznik commented 1 year ago

Early experimental support for statically linked NOMAD libraries, NOMAD executable and PyNomad interface.

Disclaimer

Overview

Statically linked PyNomad I have added a brand new setup script (setup_PyNomadStatic.py) for statically linked PyNomad interface. Its current version only supports Linux. I'll test (and extend) the script on Windows. Currently one can execute the following to build the binary distribution wheel

export NOMAD_INSTALL=/path/to/build/release
export NOMAD_VERSION=4.3.1
python3 setup_PyNomadStatic.py bdist_wheel

where the exported NOMAD_INSTALL variable should point to the place NOMAD was installed to (through cmake --install) and NOMAD_VERSION is arbitrary version string which should eventually match the release of NOMAD.

Please note that the build directory must be removed before running the setup setup script, especially if the interface was built through cmake -DBUILD_INTERFACE_PYTHON=ON .... Both the static and the dynamic version of the package share identical name (i.e., PyNomad) and this confuses setuptools if one or the other version already exists.

jan-provaznik commented 1 year ago

Welp. This did not go as planned. Time to boot a Windows machine up.

ctribes commented 1 year ago

Let me know if I can help. I have access to a Windows/Linux/OSX machine.

jan-provaznik commented 1 year ago

Let me know if I can help. I have access to a Windows/Linux/OSX machine.

Thank you for the offer. I'll first look into what went wrong before having you waste time on a wild goose chase.

Just finished setting up MSYS+MINGW on Windows, built the PR and failed with identical error as the CI. I am now in position to investigate.

With a single nomad static library (nomad.a or libnomad.a) we can keep the name nomadUtils, nomadEval and nomadAlgo for dynamic library. A flag to have CMake (-DStaticBuild=ON) produce the static library or not could be convenient.

This could be a good way to go. It should make the proof of concept a lot more straightforward: essentially just replace SHARED with STATIC in the original CMakeFiles.txt. I'll try this

Usually, we first do PR to the Develop branch. Once testing and merging in Develop works fine we do a PR Develop->Master.

I understand. (I'll change the merge target once I get MSVC to build.) Like I said in the beginning, I made this PR mostly as a demonstration that static build could be done, especially in the context of PyNomad. I expected there would be hiccups on other platforms, although frankly I hoped it would be much smoother. :)

jan-provaznik commented 1 year ago

It seems to be getting into shape. I have not realized MSVC was so finicky about function declarations, but then again MS had to bring their own calling conventions so I should have expected there would be trouble.

For some reason FreeBSD build fails even before it starts due to missing package manager.

Overall changes

With a single nomad static library (nomad.a or libnomad.a) we can keep the name nomadUtils, nomadEval and nomadAlgo for dynamic library. A flag to have CMake (-DStaticBuild=ON) produce the static library or not could be convenient.

Following your comment I'll merge the nomad?Static libraries into a single nomadStatic library in the next iteration.

jan-provaznik commented 1 year ago

Current iteration.

jan-provaznik commented 1 year ago

@ctribes Just checking in.

Current state (brief overview)

I believe the current version shows static builds are possible even though the current build process could be improved upon. I have verified its functionality on Linux (gcc), on Windows (mingw) and Windows (msvc) builds. I have also checked that the statically linked variant of PyNomad works on these platforms.

Questions and notes about possible directions of development

  1. Would it be reasonable to link the whole project (nomad binary, interfaces) statically by default?
    • Currently an equivalent of -Wl,-rpath is used, effectively rendering the binaries (and the interfaces) non-portable1
    • Since the project is LGPL already, static linking should not be a problem.
  2. A note related to the first question: dynamic libraries will be built and used in library-mode examples anyway. These binaries will suffer (and benefit) from -Wl,-rpath.
  3. Would it be reasonable to modify the relevant build process to allow building PyNomad interface without having to run cmake --install ... first?
    • This would make PyNomad consistent with C interface, which is built during cmake --build ... phase.
  4. A final remark. I am well aware all of this introduces a large number of changes and should be broken down into smaller patches before being eventually merged (if at all). I am also aware that additional unit tests should be written to thoroughly test the new functionality.

I would love to hear your thoughts on this whole endeavor.

1 One can always use patchelf utility.

ctribes commented 1 year ago

@jan-provaznik, I ran some tests on MacOSX (x86_64). It required some minor modifications. Your experiment shows that static builds work fine and can be used to create python wheels. Thanks a lot for this contribution.

Would it be reasonable to link the whole project (nomad binary, interfaces) statically by default?

A note related to the first question: dynamic libraries will be built and used in library-mode examples anyway. These binaries will suffer (and benefit) from -Wl,-rpath.

I think that building both dynamic and static libraries by default is a good idea. The nomad binary and interfaces will be more portable. The library mode examples will work as long as the user do not move/erase the shared object library. Also, having shared object library allows someone to build its own optimizer using only libnomadUtils and libnomadEval for example.

Would it be reasonable to modify the relevant build process to allow building PyNomad interface without having to run cmake --install ... first?

I did such modifications on my side. Still need to test on windows. I could contribute whenever we have a branch for this in bbopt/nomad.

A final remark. I am well aware all of this introduces a large number of changes and should be broken down into smaller patches before being eventually merged (if at all). I am also aware that additional unit tests should be written to thoroughly test the new functionality.

The modifications are not too big and it really makes PyNomad (probably other interfaces too) building more simple. For a start, they can be broken in several steps: 1- Manage both dynamic and static builds (no need for -DWITH_STATIC_BUILD) for nomad libraries and sgtelib. Modify root CMakelists.txt and src/CMakelists.txt 2- Work on setup.py to have PyNomad static build only and create a wheel. 3- Modify Cl action to build PyNomad for different platforms. 4- Add some tests for PyNomad. 5- Create the distribution package for PyPi. Automate the push to PyPi whenever changes to Master branch are made and action is ran. 6- Make Matlab interface use static library. Several steps are already in your fork. It can be done with the objective to merge into develop branch first and master branch whenever everything works smoothly.

jan-provaznik commented 1 year ago

Thank you for your feedback.

@jan-provaznik, I ran some tests on MacOSX (x86_64). It required some minor modifications. Your experiment shows that static builds work fine and can be used to create python wheels. Thanks a lot for this contribution.

I am quite happy to contribute. A lot of my own work has been achieved with NOMAD and it feels only appropriate to give something back in return.

Could you please share the necessary modifications? While I'll probably get my hands on some OSX virtual machine eventually, this could speed things up.

I think that building both dynamic and static libraries by default is a good idea. The nomad binary and interfaces will be more portable.

Yup.

The library mode examples will work as long as the user do not move/erase the shared object library. Also, having shared object library allows someone to build its own optimizer using only libnomadUtils and libnomadEval for example.

On this note, perhaps -Wl,-rpath (the CMAKE_INSTALL_RPATH_USE_LINK_PATH variable) could be de-activated with some option for advanced users that do not want to patch their CMakeFiles or the compiled binaries. Standard behavior would remain the same. With this optional behavior one could adjust their equivalent of LD_LIBRARY_PATH (either by setting the environment variable or dropping a file into /etc/ld.so.conf.d). It would allow for certain flexibility in deployment, one I would personally appreciate.

I did such modifications on my side. Still need to test on windows. I could contribute whenever we have a branch for this in bbopt/nomad.

Alright. I have probably cooked up something similar. We'll get there.

The modifications are not too big and it really makes PyNomad (probably other interfaces too) building more simple. For a start, they can be broken in several steps: 1- Manage both dynamic and static builds (no need for -DWITH_STATIC_BUILD) for nomad libraries and sgtelib. Modify root CMakelists.txt and src/CMakelists.txt 2- Work on setup.py to have PyNomad static build only and create a wheel. 3- Modify Cl action to build PyNomad for different platforms. 4- Add some tests for PyNomad. 5- Create the distribution package for PyPi. Automate the push to PyPi whenever changes to Master branch are made and action is ran. 6- Make Matlab interface use static library. Several steps are already in your fork. It can be done with the objective to merge into develop branch first and master branch whenever everything works smoothly.

Thanks for the outline. I'll get to it. Should I make these (steps) as separate PRs against the devel branch? Or would another approach be preferable for you? Thanks!

ctribes commented 1 year ago

@jan-provaznik. We can have a branch and a single PR against the development branch for everything that is already in jan-provaznik:feature-experimental-static (steps 1 and 2). We can do other PRs for the remaining steps.

I let you create this branch and the PR. I will transfer what I have done (if necessary) when it is available. Without the WITH_STATIC_BUILD option, current Cl actions will build both types of libraries and we will see what breaks.

On this note, perhaps -Wl,-rpath (the CMAKE_INSTALL_RPATH_USE_LINK_PATH variable) could be de-activated with some option for advanced users that do not want to patch their CMakeFiles or the compiled binaries. Standard behavior would remain the same. With this optional behavior one could adjust their equivalent of LD_LIBRARY_PATH (either by setting the environment variable or dropping a file into /etc/ld.so.conf.d). It would allow for certain flexibility in deployment, one I would personally appreciate.

I got used to -Wl,-rpath. But if what you suggest can bring flexibility, I vote for it. Once available, I will try it to see how it works for OSX in particular.

jan-provaznik commented 1 year ago

Update incorporating the first two steps.

Tested manually on Linux (GCC) & Windows (both MSVC and MINGW). Unfortunately it seems that parts of the CI pipeline are disabled.

Second commit fixes my mistake which accidentally removed dllexport / dllimport attributes from the C interface. This happened because C interface re-used DLL_ALGO_API macro from nomad_platform.hpp. However, this macro must be disabled when linking against nomadStatic (and when building nomadStatic) and consequently the C interface had no exported symbols. In the first commit this was solved by forcing MSVC to export all symbols. In the second commit, the dllexport attributes are set correctly.

ctribes commented 1 year ago

@jan-provaznik, I cannot contribute to your branch jan-provaznik:feature-experimental-static. For now, I will just comment the proposed modifications file by file.

ctribes commented 1 year ago
  • C interface is statically linked against nomadStatic now, I am not 100% sure this is desirable, can be reverted

I am not sure. The C-Interface is used by the Julia interface. This is done apart from bbopt-nomad. I asked the people if it is convenient or not.

jan-provaznik commented 1 year ago

@jan-provaznik, I cannot contribute to your branch jan-provaznik:feature-experimental-static. For now, I will just comment the proposed modifications file by file.

I'll figure out how to give you access (probably by adding you as a contributor I guess). I have incorporated your modifications / feedback. I'll put the separated CMakeFiles* back together before the final merge.

  • C interface is statically linked against nomadStatic now, I am not 100% sure this is desirable, can be reverted

I am not sure. The C-Interface is used by the Julia interface. This is done apart from bbopt-nomad. I asked the people if it is convenient or not.

Alright. I'll leave it in for now. Please let me know how they respond.

PS: Do you know why the M1 and BSD builds fail occasionally with missing brew (on M1) or pkg (on BSD) binaries?

ctribes commented 1 year ago
  • C interface is statically linked against nomadStatic now, I am not 100% sure this is desirable, can be reverted

I am not sure. The C-Interface is used by the Julia interface. This is done apart from bbopt-nomad. I asked the people if it is convenient or not.

Alright. I'll leave it in for now. Please let me know how they respond.

They prefer to keep the C-interface linked to dynamic libraries for now.

ctribes commented 1 year ago

@jan-provaznik I still cannot contribute to the forked repo.

image

It is too early to merge the PR into the master branch. I am not super familiar with forks. Any idea what is the best approach ?

jan-provaznik commented 1 year ago

They prefer to keep the C-interface linked to dynamic libraries for now.

Changed the relevant CMakeFile.

@jan-provaznik I still cannot contribute to the forked repo.

I am sorry. You should be a contributor now. You should be able to do whatever you desire with the fork.

It is too early to merge the PR into the master branch. I am not super familiar with forks. Any idea what is the best approach ?

Truth be told, I have no idea. I am aware NOMAD has a private development branch, the public repository is essentially an official fork of it. I suppose if this experimental feature proves to work well and there are no apparent regressions we can clean it up and leave it waiting around until the the right time for merge (next release window?) comes.

In the meantime I'll move to the next steps we've outlined earlier. I'll do that in a different branch forked from the cleaned up version of this one and we'll see how well that goes.

ctribes commented 1 year ago

@jan-provaznik I am now able to contribute to your forked repo. Thanks. As you probably realized, I have made a few minor changes to the branch feature-experimental-static. Also, I have created a pull request from your forked repo branch feature-experimental-static to the bbopt/nomad develop branch. I have tested the PyNomad build on Windows with VisualStudio (python 3.8). Everything works fine.

I will work on the Cl for Actions to build the Python interface. When this is done and the tests pass. I will merge the PR to the develop branch and remove the PR to the master branch.

jan-provaznik commented 1 year ago

@jan-provaznik I am now able to contribute to your forked repo. Thanks.

You are welcome.

As you probably realized, I have made a few minor changes to the branch feature-experimental-static.

Thanks.

Also, I have created a pull request from your forked repo branch feature-experimental-static to the bbopt/nomad develop branch. I have tested the PyNomad build on Windows with VisualStudio (python 3.8). Everything works fine.

Thanks for testing. I've only had a chance to test with python 3.10 (both msvc and mingw).

I will work on the Cl for Actions to build the Python interface. When this is done and the tests pass. I will merge the PR to the develop branch and remove the PR to the master branch.

Alright. Glad to hear!

I've started the necessary work to make automated wheel construction work. I'll keep you posted.

jan-provaznik commented 1 year ago

Huzzah! @ctribes

I've managed to put together a working wheel-building action over at feature-pypi on top of the latest feature-experimental-static branch. The workflow builds binary wheels for different versions of Python targeted for various Linux platforms, for OS X and for Windows. These are then automatically uploaded to PyPi. I am currently using its testing repository for development.

It is possible to test these pre-built binary wheels with

pip install --user --index-url https://test.pypi.org/simple --force-reinstall PyNomad

I've done some manual testing on Windows 10 and Linux, but alsas I do not have OS X available.

The prototype workflow is triggered manually. The final version should be triggered by release of new version.

ctribes commented 1 year ago

Great work @jan-provaznik. Your workflow is much better that my tentative. It's much cleaner to separate the Nomad regular build and the PyNomad build.

I tested the pip install on my OS X and PyNomad is working.

jan-provaznik commented 1 year ago

Thanks @ctribes. Thanks for testing on OS X.

The approach I took in my workflow was chosen out of sheer necessity. There are several constraints one has deal with, namely

Python community decided to handle the second constraint by defining some compatibility-levels which can be used to target as many distributions as possible. This is where manylinux images come to play. One builds their wheels on these systems, does some black magic and hopes for the best. It is where the cibuildwheel package shines: it makes it easy again by automating builds on different manylinux images.

The workflow uses this tool. It builds only the nomadStatic target (on every platform) to save up on resources and (for every version of Python) builds those wheels (through a glorified pip wheel -- . call).

Even without the first constraint, it would make no sense to publish PyPi packages on every run of the CI pipeline. Of course, it makes sense to build and test PyNomad (and other interfaces) in the CI pipeline.

On this note: currently it is not possible to build PyNomad with OpenMP active. Whether this is an artificial constraint or not (*) is to be determined, however, the CI pipeline should test both combinations: one where OpenMP is active (and PyNomad is not built) and one where OpenMP is disabled and PyNomad (or the MATLAB interface) is built.

I'll improve the proposed PyPi workflow to include tests. Fortunately cibuildwheel supports testing out of the box; it should only be a matter of writing appropriate tests.

(*) I suspect it is artificial — to prevent PyNomad users from shooting themselves in their feet. I've been using OpenMP enabled NOMAD in nomadlad without any issues, that is, as long as NB_THREADS_OPENMP 1 is enforced. I suspect that PyNomad could work like that as well.

ctribes commented 1 year ago

As I am a novice in PyPi, this is very valuable information. I am so glad you work on this!

On this note: currently it is not possible to build PyNomad with OpenMP active. Whether this is an artificial constraint or not (*) is to be determined, however, the CI pipeline should test both combinations: one where OpenMP is active (and PyNomad is not built) and one where OpenMP is disabled and PyNomad (or the MATLAB interface) is built.

PyNomad is supposed to work with OpenMP enabled. But on Windows (if I recall correctly) I had some weird crashes. I decided to let it go and work on it later. I will add a new issue about that to investigate what is going on.

jan-provaznik commented 1 year ago

As I am a novice in PyPi, this is very valuable information. I am so glad you work on this!

You are welcome. :+1:

I've added some unit tests in the feature-pypi branch. These are executed automatically against each binary wheel built. The action is still running but it looks promising. I suppose the whole thing is becoming ready to be merged into feature-experimental-static.

PyNomad is supposed to work with OpenMP enabled. But on Windows (if I recall correctly) I had some weird crashes. I decided to let it go and work on it later. I will add a new issue about that to investigate what is going on.

I see. Good luck with your investigation!

jan-provaznik commented 1 year ago

@ctribes A quick overview of the changes.

jan-provaznik commented 1 year ago

@ctribes Is there anything else I can do in this direction? I suppose some README files need to be updated.

There is one point which I am not sure of and would like to hear your opinion on.

A final note, if https://github.com/bbopt/nomad/issues/132 proves stable, we'll probably have to modify setup.py to link against OpenMP. Likewise the workflow will have to be modified to use OpenMP in wheels.

ctribes commented 1 year ago

@ctribes Is there anything else I can do in this direction? I suppose some README files need to be updated.

@jan-provaznik I believe updating the README files and the tests configuration are the two things remaining. Thanks for adding the basic unit tests.

For the PyPi package only the "HOW TO USE" section from the current readme.txt can be reused. Also, a link to User Guide is needed.

I could migrate the content of the current readme.txt into a new section of the main README.txt. It would be better I think. I can take care of these changes. I will probably also do the same for the readme of Matlab and Java interface.

There is one point which I am not sure of and would like to hear your opinion on.

  • The cibuildwheel toolkit runs tests after the wheels are built. How the tests are run can be configured either through a special section of pyproject.toml file (which we use anyway) or through environment variables set in the PyPi workflow, where all other parameters for the toolkit are set anyway.
  • I believe it would be better to configure tests through environment variables in the workflow, so that the entire configuration of the toolkit is in one place.
  • What do you think?

For me this is rather equivalent. I will follow your judgement on that.

A final note, if #132 proves stable, we'll probably have to modify setup.py to link against OpenMP. Likewise the workflow will have to be modified to use OpenMP in wheels. I am still not sure about this change. I prefer to play it safe by not enabling OpenMP right now. I will make this change in a future version when I have more time to test on different OS/compilers/Problems/Options.

ctribes commented 7 months ago

Modifs applied to branch feature/release44