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.

Sign up for GitHub

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

Add recipe for Scine Python bindings #18482

Merged
merged 3 commits into from
Apr 7, 2022
Merged

Add recipe for Scine Python bindings #18482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6747e0
Add recipe for Sparrow Python bindings
awvwgk Mar 26, 2022
df8506f
Pin version number of scine-utilsos exactly
awvwgk Mar 28, 2022
a8cf6a0
Address comments from review
awvwgk Apr 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions recipes/scine-utilities-python/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
{% set name = "scine-utilities-python" %}
{% set version = "4.0.0" %}

package:
name: {{ name }}
version: {{ version }}

source:
- url: https://github.com/qcscine/utilities/archive/refs/tags/{{ version }}.tar.gz
sha256: 3bcabd8f4270f0c1efd235a0d22506f4750b0ccffa99184d4362b95a1da7a1d4
fn: {{ name }}.tar.gz
patches:
- skip_library_build.patch
- url: https://github.com/qcscine/development-utils/archive/refs/tags/4.0.0.tar.gz
sha256: 54002c2082b6bb75672ec66bf9cf3935bbdf6b085ed9b4d7174cbdedb7c2275d
fn: dev.tar.gz
folder: dev
patches:
- pybind11_pin.patch

build:
number: 0
skip: true # [win or osx]
script:
- set -ex
- >
cmake
${CMAKE_ARGS}
-B _build -G Ninja
-DPYTHON_EXECUTABLE={{ PYTHON }}
-DSCINE_MARCH=""
-DSCINE_SKIP_LIBRARY=ON
-DSCINE_BUILD_TESTS=OFF
-DSCINE_BUILD_PYTHON_BINDINGS=ON
- cmake --build _build
- cmake --install _build

requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- cmake
- ninja
- pkg-config
- llvm-openmp # [osx]
- libgomp # [linux]
host:
- boost-cpp
- eigen
- gtest
- gmock
- libblas
Comment on lines +48 to +52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all of these included because your patched build fails if they aren't present? In theory python bindings should only depend on the library it is binding. i.e. scine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upstream package has the bad habit of downloading everything that is not present in the host environment. Since I patched the build files I need most of those present as well, alternative I can try to patch the build files further to only require the essential ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned about overlinking from the python module to libraries that are not actually used directly. It looks like the module should only link to scine and yaml, but it is also linking to BLAS.

https://github.com/qcscine/utilities/blob/493b8db45772b231bc0296535a09905b8e292f77/src/Utils/CMakeLists.txt#L135

What do you know about that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about over-linking because conda-build will not be able to detect it since BLAS is listed in the host section in order to satisfy the build system requirements.

Copy link
Member Author

@awvwgk awvwgk Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python export of the utilities library is actually quite heavy and more than just a thin wrapper around the respective C++ library, making use of most of the libraries in host, which are also found correctly by conda-build:

   INFO: sysroot: '/home/conda/staged-recipes/build_artifacts/scine-utilities-python_1649310291400/_build_env/x86_64-conda-linux-gnu/sysroot/' files: '['/home/conda/staged-recipes/build_artifacts/scine-utilities-python_1649310291400/_build_env/x86_64-conda-linux-gnu/sysroot/usr/share/zoneinfo/zone.tab', '/home/conda/staged-recipes/build_artifacts/scine-utilities-python_1649310291400/_build_env/x86_64-conda-linux-gnu/sysroot/usr/share/zoneinfo/tzdata.zi', '/home/conda/staged-recipes/build_artifacts/scine-utilities-python_1649310291400/_build_env/x86_64-conda-linux-gnu/sysroot/usr/share/zoneinfo/right/Zulu', '/home/conda/staged-recipes/build_artifacts/scine-utilities-python_1649310291400/_build_env/x86_64-conda-linux-gnu/sysroot/usr/share/zoneinfo/right/WET']'
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libutilsos.so found in conda-forge::scine-utilsos-4.0.0-hab723ef_0
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libcore.so found in conda-forge::scine-core-4.0.0-h6e2fe03_0
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libyaml-cpp.so.0.6 found in conda-forge::yaml-cpp-0.6.3-he1b5a44_4
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libblas.so.3 found in conda-forge::libblas-3.8.0-11_h6e990d7_netlib
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libstdc++.so.6 found in conda-forge::libstdcxx-ng-11.2.0-he4da1e4_14
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/libm.so.6 found in CDT/compiler package conda-forge::sysroot_linux-64-2.12-he073ed8_15
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libgcc_s.so.1 found in conda-forge::libgcc-ng-11.2.0-h1d223b6_14
   INFO (scine-utilities-python,lib/python3.8/site-packages/scine_utilities/scine_utilities.cpython-38-x86_64-linux-gnu.so): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/libc.so.6 found in CDT/compiler package conda-forge::sysroot_linux-64-2.12-he073ed8_15
WARNING (scine-utilities-python): interpreter (Python) package conda-forge::python-3.8.13-ha86cf86_0_cpython in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
WARNING (scine-utilities-python): run-exports library package conda-forge::_openmp_mutex-4.5-1_gnu in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)
WARNING (scine-utilities-python): run-exports library package conda-forge::pybind11-abi-4-hd8ed1ab_3 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

I can further patch the build files to remove dependencies like gtest/gmock, which are actually superfluous here, but downloading a few additional packages in the host environment which don't add any run exports or pins not already present from the C++ library seems not very harmful to me, especially when the C++ library is pinned with an exact version constraint.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having libblas in host is useful to have the netlib variant installed via conda-forge-pinning, without the OpenBLAS variant would get in the build step.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python export of the utilities library is actually quite heavy and more than just a thin wrapper around the respective C++ library

Good enough for me.

- pip
- pybind11
- pybind11-abi
- python
- scine-core
- scine-utilsos {{ version }}
- yaml-cpp
run:
- numpy
- python
- scine-utilsos {{ version }}
- scipy
- yaml-cpp

test:
imports:
- scine_utilities
source_files:
- src/Utils/Python/Tests/
requires:
- pip
- pytest
commands:
- pip check
- pytest src/Utils/Python/Tests/

about:
license: BSD-3-Clause
license_file: LICENSE.txt
summary: |
Contains functionality which is used in most SCINE modules (Python bindings).
home: https://scine.ethz.ch/
dev_url: https://github.com/qcscine/utilities

extra:
recipe-maintainers:
- awvwgk
13 changes: 13 additions & 0 deletions recipes/scine-utilities-python/pybind11_pin.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/cmake/ImportPybind11.cmake b/cmake/ImportPybind11.cmake
index 34d009c..a819e4c 100644
--- a/cmake/ImportPybind11.cmake
+++ b/cmake/ImportPybind11.cmake
@@ -6,7 +6,7 @@
macro(import_pybind11)
# If the target already exists, do nothing
if(NOT TARGET pybind11::pybind11)
- find_package(pybind11 2.6.2 EXACT QUIET)
+ find_package(pybind11)
if(TARGET pybind11::pybind11)
message(STATUS "Found pybind11 at ${pybind11_DIR}")
else()
64 changes: 64 additions & 0 deletions recipes/scine-utilities-python/skip_library_build.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
diff --git a/src/Utils/CMakeLists.txt b/src/Utils/CMakeLists.txt
index f435593..cfc8fb7 100644
--- a/src/Utils/CMakeLists.txt
+++ b/src/Utils/CMakeLists.txt
@@ -29,6 +29,7 @@ if(SCINE_PARALLELIZE)
find_package(OpenMP REQUIRED)
endif()

+if(NOT SCINE_SKIP_LIBRARY)
# Obey standard CMake behavior regarding shared/static libraries
add_library(UtilsOS ${UTILS_HEADERS} ${UTILS_CPPS})
if(NOT BUILD_SHARED_LIBS)
@@ -119,6 +120,10 @@ if(SCINE_BUILD_TESTS)
DESTINATION ${CMAKE_CURRENT_BINARY_DIR}
)
endif()
+else()
+include(ImportUtilsOS)
+import_utils_os()
+endif()

# Python bindings
if(SCINE_BUILD_PYTHON_BINDINGS)
@@ -164,11 +169,13 @@ if(SCINE_BUILD_PYTHON_BINDINGS)
${CMAKE_CURRENT_BINARY_DIR}/scine_utilities/__init__.py
)
file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/Python/README.rst DESTINATION ${CMAKE_CURRENT_BINARY_DIR})
+ if(NOT SCINE_SKIP_LIBRARY)
add_custom_command(TARGET UtilsOSModule POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:UtilsOSModule> ${CMAKE_CURRENT_BINARY_DIR}/scine_utilities
COMMENT "Copying UtilsOS module into python package directory"
)
add_dependencies(scine_utilities UtilsOSModule)
+ endif()

# Figure out which targets need to be copied along
set(_py_targets_to_copy Scine::Core) # Core is always shared
@@ -187,11 +194,13 @@ if(SCINE_BUILD_PYTHON_BINDINGS)
string(APPEND utils_PY_DEPS ", \"${_target_filename}\"")
endforeach()

+ if(NOT SCINE_SKIP_LIBRARY)
add_custom_command(TARGET UtilsOS POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy ${_py_target_gen_exprs} ${CMAKE_CURRENT_BINARY_DIR}/scine_utilities
COMMENT "Copying dependent shared libraries into python package directory"
)
message(STATUS "Targets to copy for python bindings: ${_py_targets_to_copy}")
+ endif()
unset(_py_targets_to_copy)

# Typing stubs
@@ -227,10 +236,12 @@ if(SCINE_BUILD_PYTHON_BINDINGS)
OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/doc-py
DOCTEST
)
+ if(NOT SCINE_SKIP_LIBRARY)
# The UtilsOSModule is technically also a dependency of the documentation
if(TARGET scine_utilitiesDocumentation)
add_dependencies(scine_utilitiesDocumentation UtilsOSModule)
endif()
+ endif()
endif()

if(SCINE_BUILD_PYTHON_BINDINGS)