-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add recipe for Scine Python bindings #18482
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
7d65f01
to
297c1fd
Compare
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/scine-utilities-python:
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@conda-forge/help-python-c This recipe for Python bindings to a C++ library is ready for review (pybind11, CMake). Some patching is required to make it work, this is mainly to avoid compiling the C++ library together with the Python bindings, since the C++ part already takes half an hour to compile and is huge. |
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/celluloid:
For recipes/eko:
For recipes/fps-kernels:
|
@conda-forge/staged-recipes This recipe is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @awvwgk,
Could you point me to the conda-forge feedstock(s) that build the other scine packages? I can't seem to find them. Thanks!
- boost-cpp | ||
- eigen | ||
- gtest | ||
- gmock | ||
- libblas |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What do you know about that?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).