Skip to content

Conversation

greole
Copy link
Collaborator

@greole greole commented Aug 28, 2025

Description

This PR adds an installation routine via pip, which is probably the required prerequisite to get pyGinkgo on to PyPi. It uses https://tttapa.github.io/py-build-cmake/index.html to trigger the cmake build.

It is now possible to build and install pyGinkgo with pip install or python -m build from source, also this should be a necessary step towards adding pyGinkgo to PyPi.

Copy link
Collaborator

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

another question: how does it detect user environment? for example cuda/hip/oneapi?

- name: Set up Python
uses: actions/setup-python@v2
with:
python-version: '3.8'
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe you can take a look on https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/run-job-variations
I think we need to publish several python version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

true, but this is just for testing. I was a bit reluctant to add more versions here since a single build takes already 40minutes. i fear that we might run out of ci credits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. When we open source this repo, github action will give unlimited credits for ci

@greole
Copy link
Collaborator Author

greole commented Aug 29, 2025

another question: how does it detect user environment? for example cuda/hip/oneapi?

This is how I understand it: pip just invokes cmake with some flags, thus it will run the AutoEnableDevice.cmake which checks if CUDA/HIP is present. This can be also overwritten by https://tttapa.github.io/py-build-cmake/reference/command-line-overrides.html and https://tttapa.github.io/py-build-cmake/reference/config.html#cmake

Copy link
Collaborator

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

I do not see the other weird changes currently.
I guess we need to trigger it to see what actually happens

include = ["CMakeLists.txt", "src", "cmake/*", "tests"]

[tool.py-build-cmake.cmake]
minimum_version="3.20"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
minimum_version="3.20"
minimum_version="3.21"

Is there any reason for 3.20?
When using ginkgo hip, we require 3.21.
Maybe for simplicity, we use 3.21 here?

- Pybind11
- Ninja # if you want to use cmake presets
- [pybind11-stubgen](https://pypi.org/project/pybind11-stubgen/) # if you want to use [stubs generation](#stubs-generation)

### Building the module
### Building the module via Cmake
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
### Building the module via Cmake
### Building the module via CMake

@@ -21,12 +21,12 @@ The tests successfully run on the following Python versions:
### Prerequisites

- Python 3.x
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Python 3.x
- Python 3.8+

name = "pyGinkgo"

[tool.py-build-cmake.sdist]
include = ["CMakeLists.txt", "src", "cmake/*", "tests"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
include = ["CMakeLists.txt", "src", "cmake/*", "tests"]
include = ["CMakeLists.txt", "src", "cmake/*", "tests", "CTestConfig.cmake"]

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

Successfully merging this pull request may close these issues.

2 participants