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.

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 scikit as an option for build system #161

Merged
merged 37 commits into from
Aug 1, 2023

Conversation

ayeankit
Copy link
Contributor

Pull Request description

Fixes #71

  1. Activate option in TUI to maturin
  2. Adding documentation about scikit
  3. Adding scikit as a build-system:
  4. Creating a scikit-pyproject.toml
  5. Editing post_gen_project.py
  6. Creating a smoke test (build-system.sh)
  7. Editing cookicutter.json
  8. Added scikit in Readme.md

How to test these changes

  • ...

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more complexity.
  • New and old tests passed locally.

Additional information

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved .

@xmnlab
Copy link
Member

xmnlab commented Jul 31, 2023

@ayeankit I just merged 2 PRs, could you rebase your branch on top of upstream/main pls?

@xmnlab
Copy link
Member

xmnlab commented Jul 31, 2023

could you check if you are using the instructions from here?
https://scikit-build.readthedocs.io/en/latest/

@ayeankit
Copy link
Contributor Author

could you check if you are using the instructions from here? https://scikit-build.readthedocs.io/en/latest/

yes, I am using official docs for reference. Thank you for asking.

README.md Outdated Show resolved Hide resolved
Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

@ayeankit thanks for working on that!
in general it looks good.
please change the references (names, links etc) from just scikit to scikit-build, also replace the references from scikit-hep by scikit-build

docs/guide.md Outdated
@@ -325,7 +325,14 @@ packages. SciCookie support the following:
compatibility with setuptools and Cargo make it an easy-to-use tool, offering
developers a simple solution to combine the strengths of Python and Rust within
a unified project.

- [**scikit**](https://scikit-hep.org):It's build system designed for Python
Copy link
Member

Choose a reason for hiding this comment

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

same thing here about scikit-build and https://scikit-build.readthedocs.io/

@@ -56,6 +56,9 @@ In addition, you should know that to build our package we use
{%- elif cookiecutter.build_system == "maturin" -%}
In addition, you should know that to build our package we use
[Maturin](https://pypi.org/project/maturin/0.8.2/):It's a Python packaging tool and build system for creating Python bindings from Rust projects. It enables seamless integration of Rust code into Python applications, offering efficient builds, cross-platform support, and compatibility with different Python versions. Maturin automates the process of generating Python modules that directly call Rust functions, leveraging Rust's performance and low-level capabilities in Python. With its easy-to-use interface and integration with setuptools and Cargo, Maturin provides a straightforward solution for developers seeking to combine the strengths of Python and Rust in a single project.
{%- elif cookiecutter.build_system == "maturin" -%}
Copy link
Member

Choose a reason for hiding this comment

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

probably this should be scikit-build

src/scicookie/cookiecutter.json Outdated Show resolved Hide resolved
src/scicookie/hooks/post_gen_project.py Outdated Show resolved Hide resolved
src/scicookie/hooks/post_gen_project.py Outdated Show resolved Hide resolved
src/scicookie/hooks/post_gen_project.py Outdated Show resolved Hide resolved
src/scicookie/profiles/base.yaml Outdated Show resolved Hide resolved
src/scicookie/{{cookiecutter.project_slug}}/conda/dev.yaml Outdated Show resolved Hide resolved
@@ -56,6 +56,9 @@ In addition, you should know that to build our package we use
{%- elif cookiecutter.build_system == "maturin" -%}
In addition, you should know that to build our package we use
[Maturin](https://pypi.org/project/maturin/0.8.2/):It's a Python packaging tool and build system for creating Python bindings from Rust projects. It enables seamless integration of Rust code into Python applications, offering efficient builds, cross-platform support, and compatibility with different Python versions. Maturin automates the process of generating Python modules that directly call Rust functions, leveraging Rust's performance and low-level capabilities in Python. With its easy-to-use interface and integration with setuptools and Cargo, Maturin provides a straightforward solution for developers seeking to combine the strengths of Python and Rust in a single project.
{%- elif cookiecutter.build_system == "maturin" -%}
In addition, you should know that to build our package we use
[scikit-hep](https://scikit-hep.org):It's a Python packaging tool and build system an improved build system generator for CPython C extensions. It provides better support for additional compilers, build systems, cross compilation, and locating dependencies and their associated build requirements.This tool improves package management in the scientific Python ecosystem, enabling cross-platform builds with CMake, and seamless integration with C/C++ libraries for research software engineers.
Copy link
Member

Choose a reason for hiding this comment

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

change scikit-hep to scikit-build text and refs, please

tests/smoke/build-system.sh Outdated Show resolved Hide resolved
docs/guide.md Outdated Show resolved Hide resolved
Co-authored-by: Ivan Ogasawara <[email protected]>
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @ayeankit! This PR confuses scikit-build with scikit-build-core. I can see that you use scikit-build-core in pyproject.toml, but you are calling it scikit-build everywhere in this PR. scikit-build and scikit-build-core are technically different build backends.

@ayeankit
Copy link
Contributor Author

ayeankit commented Jul 31, 2023

Thanks, @ayeankit! This PR confuses scikit-build with scikit-build-core. I can see that you use scikit-build-core in pyproject.toml, but you are calling it scikit-build everywhere in this PR. scikit-build and scikit-build-core are technically different build backends.

Thanks @Saransh-cpp , so should I change scikit-build to scikit-build-core, as scikit-build uses setup.py and scikit-build-core uses main.cpp.And I here uses main.cpp method.

@Saransh-cpp
Copy link
Member

We should ideally have both of them in scicookie. Given that you have already done most of the stuff keeping scikit-build-core in mind as the backend, you can switch all the text references to scikit-build-core.

@ayeankit
Copy link
Contributor Author

ayeankit commented Aug 1, 2023

@xmnlab Can you please review it once again?

docs/guide.md Outdated Show resolved Hide resolved
src/scicookie/cookiecutter.json Outdated Show resolved Hide resolved
src/scicookie/hooks/post_gen_project.py Outdated Show resolved Hide resolved
src/scicookie/hooks/post_gen_project.py Outdated Show resolved Hide resolved
src/scicookie/profiles/base.yaml Outdated Show resolved Hide resolved
src/scicookie/{{cookiecutter.project_slug}}/conda/dev.yaml Outdated Show resolved Hide resolved
tests/smoke/build-system.sh Outdated Show resolved Hide resolved
@ayeankit
Copy link
Contributor Author

ayeankit commented Aug 1, 2023

Hey @Saransh-cpp , I think there needs to be one more PR for scikit-build with using scikit-buildas backend for build-system.

@Saransh-cpp
Copy link
Member

Yes!

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

It would make sense to explicitly use scikit-build-core everywhere, given that you plan to add scikit-build as a backend too.

src/scicookie/hooks/post_gen_project.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Rename scikit-build-core-pyproject.toml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure!!

tests/smoke/base.sh Outdated Show resolved Hide resolved
tests/smoke/base.sh Outdated Show resolved Hide resolved
tests/smoke/base.sh Outdated Show resolved Hide resolved
src/scicookie/hooks/post_gen_project.py Outdated Show resolved Hide resolved
src/scicookie/{{cookiecutter.project_slug}}/conda/dev.yaml Outdated Show resolved Hide resolved
tests/smoke/base.sh Outdated Show resolved Hide resolved
Copy link
Member

@xmnlab xmnlab left a 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 !
@Saransh-cpp thank you so much for the review!

@xmnlab xmnlab merged commit 3dc8562 into osl-incubator:main Aug 1, 2023
11 checks passed
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

🎉 This PR is included in version 0.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for scikit-build build system
3 participants