Skip to content

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Dec 22, 2022

This PR has evolved a bit over the implementation. Changes here are:

  • Export cmake targets symspg and spglib_f08
  • Make cmake the main build flow:
    • Version is defined in cmake project
    • All builds call the toplevel CMakeLists.txt
  • Added options: WITH_Fortran, WITH_Python and WITH_TESTS
  • Migrate ctest to the main cmake
  • Migrate python api to the main cmake

Closes #208

@LecrisUT
Copy link
Collaborator Author

There is one unclear usage in this project. Why is there a separate shared and static target? How about using BUILD_SHARED_LIBS to trigger building shared or static libs natively?

Signed-off-by: Cristian Le <cristian.le@mpsd.mog.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@atztogo
Copy link
Collaborator

atztogo commented Dec 23, 2022

Thanks @LecrisUT.

There is one unclear usage in this project. Why is there a separate shared and static target? How about using BUILD_SHARED_LIBS to trigger building shared or static libs natively?

From cmake documentation, "If no type is given explicitly the type is STATIC or SHARED based on whether the current value of the variable BUILD_SHARED_LIBS is ON."

Simply, I didn't know that. Can we build both? This change may affect to conda-forge build. Ping @jochym.

@LecrisUT
Copy link
Collaborator Author

How about running cmake twice with BUILD_SHARED_LIBS with both values?

@atztogo
Copy link
Collaborator

atztogo commented Dec 23, 2022

How about running cmake twice with BUILD_SHARED_LIBS with both values?

Yes, I think it is the solution. I will have a look into the detail of your PR.

@LecrisUT
Copy link
Collaborator Author

I think the build error now is of setup.py, I am fixing it in a bit. If there are anything hard-coded in the workflow can you handle that?

@atztogo
Copy link
Collaborator

atztogo commented Dec 23, 2022

How can version.h.in be used? I don't know the meaning of the file extension .in.

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Dec 23, 2022

Anything that is .in is configured by cmake using configure_file. It basically just moved the version definition to cmake in order to not have to define both in PROJECT_VERSION and header. It did create a bit of incompatibility with python because of that though.

Note version.h is only included in the build step in the CMAKE_CURRENT_BINARY_DIR instead of the source for common standards.

Also I am working on a fix for the fedora repo (https://copr.fedorainfracloud.org/coprs/lecris/spglib/)

@LecrisUT LecrisUT force-pushed the cmake-target branch 3 times, most recently from c0dd433 to 4e7600b Compare December 23, 2022 18:57
@LecrisUT
Copy link
Collaborator Author

Ok, I have the python api able to build and run, but there might be a few issues:

  • Currently (also before) installing via setup.py compiles the libsysmspg twice, once in the main cmake and once in setup.py calling cmake again (or manual compilation). Package managers complain about this because it is ambiguous and RPATH is not setup well in the setup.py. This might be solvable if we make the python api a separate project and have it link via the installed runtime one. Issue is, how can we bundle the cmake project in PyPi?
  • The ctests are a separate project, but usually these should be toggled by a flag and included for easier call of ctest

Otherwise let's try again the CI if things work

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

Ok, I've updated setup.py search for python interpreter and fixed ctests. I have embedded them in the main project, hope it's better

LecrisUT and others added 2 commits December 24, 2022 16:05
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jan 3, 2023

Btw, I should clarify what's going on with this. I was able to replicate the failures of the CI that is reported there using the commands.

python setup.py build
pip install -e .

I was previously testing with pip install . and for some reason that worked. I'm not sure how it interacts with pypi so I will try to fix that build eventually.

I was considering the design (feel free to stop me and nudge me in a more sensible direction) so that you run cmake once and have the pip get the libraries from a pre-configured installation. I was thinking about this because of package managers would have spglib package and then python3-spglib one for the python api and the latter should be linked to the system installed one. But at the same time installing with simple pip install should also work without OS package installed, so I am a bit lost there. I don't have many references of python-C programs that I can consult for best standards (the only other ones I see are using SWIG?).

@atztogo
Copy link
Collaborator

atztogo commented Jan 4, 2023

I was previously testing with pip install . and for some reason that worked. I'm not sure how it interacts with pypi so I will try to fix that build eventually.

pip install . might not work with under conda. To have a compiler portably, conda is very useful, so I use conda almost always.

@atztogo
Copy link
Collaborator

atztogo commented Jan 4, 2023

I was considering the design ...

I agree, it is difficult to find the information. About the parameters of setuptools.setup, this page may be useful, https://setuptools.pypa.io/en/latest/userguide/declarative_config.html.

Currently I don't yet understand what you really want to do. Could you give me some more information what you want to design? In the case of phonopy (https://github.com/phonopy/phonopy/blob/develop/setup.py), setup.py creates static link library calling cmake and link it to the python module. I am afraid of dynamic link library because users may have to set property LD_LIBRARY_PATH for linux and those convention can be different in different systems. The purpose of using cmake in phonopy's setup.py is (hopefully) automatically finding necessary libraries in complication of C code, e.g., openmp.

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jan 4, 2023

Ok, so let me clarify the scope of this PR since it evolved a bit:

  • Export cmake targets
  • Switch to central cmake build process: version is defined there, and all builds are controlled by main cmake
  • Migrate ctest to be included in main cmake
  • Migrate python build to use cmake
  • Fix setup.py to build for conda/CI

There might be 2 ways to make the build work like this:

  • Use static libraries and duplicate the spglib library into _spglib.so (similar to previous behavior)
  • Debug and fix the missing _spglib.cpython-39-x86_64-linux-gnu.so

I would lean towards the second one because it would enable the user to use whichever setup it is currently installed on (e.g. if they used module load spglib-omp to switch to an OpenMPI variant) with the same api built. Indeed correct LD_LIBRARY_PATH needs to be set, but it should be handled by the package manager, lmod, etc. that built spglib. The only part I don't know is if it loaded appropriately by pip or conda. If not, we can always do it in python with a try-catch here that defaults to a bundled version of spglib if the system does not have an appropriate version.

@atztogo
Copy link
Collaborator

atztogo commented Jan 4, 2023

Can your configuration work for macOS and Windows?

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jan 4, 2023

That's a good point, and I don't have a workspace to test those. If the CI had those, that would be great. For the cmake part, it should be perfectly ok, just the python one might need to specify for each os (e.g. DYLD_LIBRAY_PATH), although there might be some internal api to do for any of them. At the same time, I don't think shared library is very relevant for those, so we could have cmake build it statically for those. Usually in cmake it is handled by BUILD_SHARED_LIBS and it would be called by the builder, in this case setup.py, instead of hard-coding a standard.

@atztogo
Copy link
Collaborator

atztogo commented Jan 4, 2023

Having CI for windows and macOS is great. Writing workflow takes time for me and needs some skill, so I hesitated to invest my time for it unless having strong motivation.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jan 6, 2023

Ok, I think I've fixed the build. I have tested with pip install . which bundles the dependencies libsymspg.so so it works without LD_LIBRARY_PATH. I am not sure how to make pip install -e to also include the dependencies. But we have 3 options:

  • Switch to static library and include USE_OMP and other such configurations in setup.py somehow
  • Try migrating to scikit that could handle all of these issues in the backend instead of the manual hack I did
  • Migrate to swig interface. Most tutorials I see are using that, but I didn't see an equivalent setup.py to distribute it

- Bundle C++ library with python api
- Remove editable mode from CI (does not include libraries otherwise)
- Try to load C++ library and if fail use bundled version

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jan 6, 2023

I have switched this now to scikit, which should be able to handle all OSs. I don't know why the pip editable mode is enabled in the CI, but it does not bundle the libraries if it is enabled. If we want to keep that, the CI should also install the cmake libraries being built, but then can't confirm that the bundled version works or not.

Can you check if I included the dependencies correctly, and if it would still work for PyPi distribution?

@atztogo
Copy link
Collaborator

atztogo commented Jan 6, 2023

@LecrisUT, thanks for your effort. How about not using cmake for python spglib?

@atztogo
Copy link
Collaborator

atztogo commented Jan 6, 2023

We probably need to add cmake and scikit-build in these lines if we want to use cmake for python spglib build,

conda install --yes -c conda-forge numpy gcc_linux-64 pip pyyaml

conda install --yes -c conda-forge numpy gcc_linux-64 pip pyyaml

@atztogo
Copy link
Collaborator

atztogo commented Jan 6, 2023

Conda-force package of spglib is made in the other system, https://github.com/conda-forge/spglib-feedstock. Wheel package for pip install is build by another github workflow, https://github.com/spglib/spglib/blob/develop/.github/workflows/pypi-wheel-builds.yml using pypa/cibuildwheel. we have to take care of these, too, and they are not a very easy task. So though not very beautiful not to use cmake for building python package, in total, required effort for maintenance may be smaller.

OpenMP support can be removed.

@atztogo
Copy link
Collaborator

atztogo commented Jan 6, 2023

Writing CI is always annoying and packaging is even more. My concern is if pypa/cibuildwheel which is used for spglib wheel build works properly or not. This can be tested using https://test.pypi.org/. Currently this is automatically triggered by pushing to rc branch of spglib repository. We can try if it is our direction.

@LecrisUT LecrisUT changed the title Export cmake targets Improve cmake build Jan 10, 2023
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jan 10, 2023

How about not using cmake for python spglib?

I'm afraid that we would be kicking the can down the road. If we add/remove source code files, link to external libraries, etc., we would have to duplicate the build logic across these, if even realistically plausible. Although for the python api specifically, if we make sure that the api header is independent of build system, these should be compatible with any compiled environment (if possible we should check how it behaves when compiled with different compilers though).

The latest one should fix all issues on this repo side and the pypi CI. To test the RC we should be making another MR to rc branch?

I will check the conda forge you suggested and make an equivalent PR there: conda-forge/spglib-feedstock#82

@atztogo
Copy link
Collaborator

atztogo commented Jan 10, 2023

I see. Is there any documentation update needed, for example, test and fortran make?

@LecrisUT
Copy link
Collaborator Author

I think I've documented most of it, but I found a few spaces I forgot

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

Ok, I've done my best with the documentation. It should cover all aspects except ruby and documentation building that I did not touch. Let me know if any parts of it are unclear

@codecov-commenter
Copy link

Codecov Report

Base: 85.34% // Head: 64.34% // Decreases project coverage by -21.00% ⚠️

Coverage data is based on head (66da1c3) compared to base (2b06c29).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop     #210       +/-   ##
============================================
- Coverage    85.34%   64.34%   -21.01%     
============================================
  Files           18       18               
  Lines         1358     1321       -37     
============================================
- Hits          1159      850      -309     
- Misses         199      471      +272     
Impacted Files Coverage Δ
spglib/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
spglib/spglib.py 0.00% <0.00%> (-80.79%) ⬇️
setup.py 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@atztogo atztogo merged commit b8d068b into spglib:develop Jan 12, 2023
@atztogo
Copy link
Collaborator

atztogo commented Jan 12, 2023

Merged. Thanks @LecrisUT. I will try to make editable install possible.

@LecrisUT
Copy link
Collaborator Author

About that. It's technically possible, but not the scope of editable install. Afaiu, editable installs are for development allowing for live editing of the python code in place in the source folder. Its focus is on the pyrhon code so the dev would have to make sure the other environment is working properly like appropriate LD_LIBRARY_PATH.

But if we want to make that part seamless too, just need to make sure _spglib.so and libsymspg.so are present in python/spglib folder.

Note that pip install -e . is technically deprecated, but I don't know what replaces it, just know it's related to pyproject.toml

@atztogo
Copy link
Collaborator

atztogo commented Jan 12, 2023

@LecrisUT, yes, I know you are correct. But I expect editable install working until alternative appears.

@LecrisUT LecrisUT deleted the cmake-target branch January 20, 2023 09:37
@LecrisUT LecrisUT added this to the 2.1 milestone Feb 21, 2023
@LecrisUT LecrisUT added the enhancement Significant ehancements label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Significant ehancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export spglibConfig.cmake
3 participants