-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat(template): Added pybind11 as an option for build system #163
Conversation
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 not super familiar with pybind11
but maybe it would be necessary to have an extra setup.py file
ref: https://github.com/pybind/python_example/blob/master/setup.py
@@ -4,6 +4,6 @@ namespace py = pybind11; | |||
|
|||
PYBIND11_MODULE(skcdemo, m) { | |||
m.def("hello", [](){ | |||
py::print("Hello, scikit-build-core!"); | |||
py::print("Hello !!!"); |
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.
maybe you should change the name of the file to something more generic than skcdemo
maybe example.cpp ... or as it uses pybind11 .. maybe pybind11.cpp ..
tests/smoke/base.sh
Outdated
@@ -55,6 +55,14 @@ elif command -v maturin &> /dev/null; then | |||
pip install -e . | |||
elif [ "$(pip list|grep -c scikit_build_core)" -ne "0" ]; then | |||
pip install -e . | |||
elif [ "$(pip list|grep -c pybind11)" -ne "0" ]; then | |||
# Assuming you are inside the root of the CMake source directory | |||
mkdir build |
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.
ideally you should run just pip install -e .
and it should run cmake under the hook
Discussions = "{{ cookiecutter.project_url }}/discussions" | ||
Changelog = "{{ cookiecutter.project_url }}/releases" | ||
|
||
[tool.hatch.version] |
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 don't think it needs hatch session here
0855584
to
e9c6226
Compare
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.
Great work, thanks, @ayeankit! Some comments below -
docs/guide.md
Outdated
@@ -334,7 +334,21 @@ packages. SciCookie support the following: | |||
build requirements. With its capabilities, it facilitates cross-platform | |||
builds using CMake and effortless integration with C/C++ libraries, making | |||
it a valuable asset for research software engineers. | |||
|
|||
|
|||
- [**Pybind11**](https://pybind11.readthedocs.io/en/stable/): It's build system designed |
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.
Should we call it something like setuptools + pybind11
to be more clear, given that pybind11
itself is not a backend?
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.
Or atleast using "setuptools + pybind11" in all the user-facing locations, and just "pybind11" in the internals of scicookie
.
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.
Or atleast using "setuptools + pybind11" in all the user-facing locations, and just "pybind11" in the internals of
scicookie
.
that makes sense to me.
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.
Or atleast using "setuptools + pybind11" in all the user-facing locations, and just "pybind11" in the internals of
scicookie
.that makes sense to me.
Wouldn't that be confusing since we are using setuptools as default build-sytem?
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.
not really ... setuptools is the default option because it was the first one historically, but a lot of people want to kill that. That is why a bunch of project are looking for new alternatives.
but for c/c++ code setuptools is still a great option.
so specifying setuptools + pybind11 makes a lot of sense, because it is clear about its structure.
|
||
install(TARGETS pybind11 DESTINATION .) | ||
|
||
{%- endif %} |
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.
{%- endif %} | |
{%- endif %} | |
m.def("hello", [](){ | ||
py::print("Hello, pybind11!"); | ||
}); | ||
} |
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.
} | |
} | |
name="OSL Python package", | ||
author="{{cookiecutter.author_full_name}}", | ||
author_email="{{cookiecutter.author_email}}", | ||
url="https://{{ cookiecutter.project_slug }}.com", | ||
description="A test project using pybind11", | ||
long_description="", | ||
#extras_require={"test": "pytest"}, | ||
# Currently, build_ext only provides an optional "highest supported C++ | ||
# level" feature, but in the future it may provide more features. | ||
python_requires=">=3.7", |
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.
All of this is already specified in pyproject.toml
. We would need something like this - https://github.com/scientific-python/cookie/blob/main/%7B%7Bcookiecutter.project_name%7D%7D/%7B%25%20if%20cookiecutter.backend%3D%3D'pybind11'%20%25%7Dsetup.py%7B%25%20endif%20%25%7D
22f7d50
to
ff88a54
Compare
@@ -1,3 +1,4 @@ | |||
{% if cookiecutter.build_system == "scikit-build-core" -%} |
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.
where are you declaring this variable? SKBUILD_PROJECT_NAME
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.
it doesn't look correct to resuse the name sk... for both build system
also the first two lines seem to be duplicated .. probably you could move it to some first lines in order to reuse that for both build system
|
||
namespace py = pybind11; | ||
|
||
PYBIND11_MODULE(skcdemo, m) { |
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.
skcdemo
can be called maybe pybinddemo
?
8e3b63a
to
7e83d22
Compare
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.
Some comments below that might push you in the right direction for fixing the CI error 🙂
@@ -115,6 +115,9 @@ build: | |||
hatch build | |||
{%- elif cookiecutter.build_system == "maturin" %} | |||
maturin build | |||
{%- elif cookiecutter.build_system == "pybind11" %} | |||
pybind11 build |
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 you sure this command exists?
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 think you can use something like python3 -m build
https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#packaging-your-project
ensure you have build
as a dependency in pyproject.toml/build-system
|
||
find_package(pybind11 CONFIG REQUIRED) | ||
|
||
pybind11_add_module(_core MODULE src/main.cpp) |
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 think the src
should not be here? Or it should be src/<package_name>/main.cpp
?
@@ -0,0 +1,121 @@ | |||
[build-system] | |||
requires = ["setuptools>=42", "wheel", "pybind11~=2.6.1"] |
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.
IIRC, editable install with pyproject.toml
support was first supported with setuptools version 65
. Could you please check and confirm?
@@ -0,0 +1,23 @@ | |||
from __future__ import annotations |
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.
We are not adding type annotations anywhere in this file. What purpose is this import serving then?
tests/smoke/base.sh
Outdated
@@ -55,6 +55,9 @@ elif command -v maturin &> /dev/null; then | |||
pip install -e . | |||
elif [ "$(pip list|grep -c scikit_build_core)" -ne "0" ]; then | |||
pip install -e . | |||
elif [ "$(pip list|grep -c pybind11)" -ne "0" ]; then | |||
# Assuming you are inside the root of the CMake source directory | |||
pip install -e . |
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.
Yes! This -e
would not work with pyproject.toml
and setuptools<65
, IIRC.
ext_modules = [ | ||
Pybind11Extension( | ||
"{{ cookiecutter.project_slug }}._core", | ||
["src/main.cpp"], |
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.
You should take a look at the path here again.
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.
this is an interesting discussion .. but I think we can have this discussion after we merge this PR
I am also checking this discussion with Leah (pyOpenSci) to see if there is already something recommended by pyOpenSci
@@ -55,6 +55,9 @@ elif command -v maturin &> /dev/null; then | |||
pip install -e . | |||
elif [ "$(pip list|grep -c scikit_build_core)" -ne "0" ]; then | |||
pip install -e . | |||
elif [ "$(pip list|grep -c pybind11)" -ne "0" ]; then | |||
# Assuming you are inside the root of the CMake source directory | |||
pip install . |
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.
This should still be -e
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.
in my last PR I removed -e
from everywhere, so we will be sure to test the package correctly (as much as possible). my main concern is about testing this extension files .. so we would ensure to test if that is in the package .. maybe the test is not good enough yet ... but at least it is a first step
build_system_dir = PROJECT_DIRECTORY / "build-system" | ||
src_system_dir = PROJECT_DIRECTORY/ "src" |
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 think PROJECT_DIRECTORY
automatically checks for flat or src
layout?
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 think it for main.cpp
which should be in src
directory like this.
your_project/
|-- setup.py
|-- pyproject.toml
|-- src/
| |-- main.cpp
| |-- other_cpp_files.cpp
| |-- your_module_name_here.cpp
| |-- your_module_name_here.h
|-- your_package/
| |-- init.py
| |-- your_module_name_here.py
|-- tests/
| |-- test_your_module_name_here.py
|-- README.md
|-- LICENSE
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.
this is an interesting discussion ... maybe we can return to this discussion to check how we should proceed not just for this build-system but maybe for all build-systems.
also maybe in our repo, maybe we could have this files in folders called by the programming language (cpp
, rust
, etc)
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Hey @xmnlab , All test cases passed !!, I have changed |
Co-authored-by: Ivan Ogasawara <ivan.ogasawara@gmail.com>
@@ -115,6 +115,9 @@ build: | |||
hatch build | |||
{%- elif cookiecutter.build_system == "maturin" %} | |||
maturin build | |||
{%- elif cookiecutter.build_system == "pybind11" %} | |||
python -m pip install build |
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.
you are installing build
here, but it is already defined by pyproject in the build section: https://github.com/osl-incubator/scicookie/pull/163/files#diff-ffc1694c12f08f07de37e5816475fd014df381d8fc20a600597559c8072b61fdR2
this step here is to build the package, not to install a dependency for building
@Saransh-cpp and I already told you what the command you should try here
because in another words, you are not doing what it should do, it should create a package in this 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.
thanks for working on that @Saransh-cpp
the only missing point (in my opinion) is this issue: https://github.com/osl-incubator/scicookie/pull/163/files#r1289103642
hey @xmnlab , I did the required changes, and added |
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.
thanks for working on that @ayeankit !
lgtm!
the only thing that is not working well is the version, as you can see in the logs:
creating osl-python-package-0.0.0
creating osl-python-package-0.0.0/src
creating osl-python-package-0.0.0/src/osl_python_package
creating osl-python-package-0.0.0/src/osl_python_package.egg-info
creating osl-python-package-0.0.0/tests
copying files to osl-python-package-0.0.0...
copying LICENSE -> osl-python-package-0.0.0
copying README.md -> osl-python-package-0.0.0
copying pyproject.toml -> osl-python-package-0.0.0
copying setup.py -> osl-python-package-0.0.0
copying src/main.cpp -> osl-python-package-0.0.0/src
copying src/osl_python_package/__init__.py -> osl-python-package-0.0.0/src/osl_python_package
copying src/osl_python_package/osl_python_package.py -> osl-python-package-0.0.0/src/osl_python_package
copying src/osl_python_package.egg-info/PKG-INFO -> osl-python-package-0.0.0/src/osl_python_package.egg-info
copying src/osl_python_package.egg-info/SOURCES.txt -> osl-python-package-0.0.0/src/osl_python_package.egg-info
copying src/osl_python_package.egg-info/dependency_links.txt -> osl-python-package-0.0.0/src/osl_python_package.egg-info
copying src/osl_python_package.egg-info/requires.txt -> osl-python-package-0.0.0/src/osl_python_package.egg-info
copying src/osl_python_package.egg-info/top_level.txt -> osl-python-package-0.0.0/src/osl_python_package.egg-info
copying tests/test_osl_python_package.py -> osl-python-package-0.0.0/tests
but we can address this issue in a follow-up.
🎉 This PR is included in version 0.6.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Pull Request description
Fixes #70
How to test these changes
...
Pull Request checklists
This PR is a:
About this PR:
Author's checklist:
Additional information
Reviewer's checklist
Copy and paste this template for your review's note: