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

Add cibuildwheel workflow #2029

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

Conversation

yambati03
Copy link

@yambati03 yambati03 commented Feb 20, 2025

What is this PR?

This PR introduces a new pipeline that uses cibuildwheel to build platform-dependent python wheels. This is the PyPA recommended way for building compliant Python wheels across platforms.

Why modify has_ext_modules() in the distclass?

has_ext_modules() is called by setuptools to indentify if the built wheel is a pure-Python wheel or a platform-dependent wheel. However, has_ext_modules() only returns True if the ext_modules parameter is defined with the source C/C++ files to build. However, we pre-build our C++ library prior to building the wheel. Thus, we need to force has_ext_modules() to return True by overwriting it.

Note: This workflow is currently only set up to target Linux x86_64 architectures. However, since it is using cibuildwheel it can easily be extended to other architectures (such as Linux ARM). See the below image from the cibuildwheel docs.
image

cc @dellaert @akshay-krishnan @truher @ProfFan as interested parties :)

@yambati03 yambati03 self-assigned this Feb 20, 2025
@yambati03 yambati03 marked this pull request as ready for review February 20, 2025 23:50
@yambati03 yambati03 added the ci Related to the Continuous Integration pipeline label Feb 21, 2025
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

This is incredible! I left some comments, but main request is to rebrand to gtsam-develop and have merge commit hashes rather than dates. The base version of course should be in there as well. We are “pre-4.3”, so I guess GTSAM-develop.4.3.HASHHHH or something.

maybe find some docs on using commit hashes to see if it is standard practice somewhere, so in the docs (add a README file?) we can refer to that.

CMakeLists.txt Outdated
@@ -10,10 +10,16 @@ set (GTSAM_VERSION_PATCH 0)
set (GTSAM_PRERELEASE_VERSION "a0")
math (EXPR GTSAM_VERSION_NUMERIC "10000 * ${GTSAM_VERSION_MAJOR} + 100 * ${GTSAM_VERSION_MINOR} + ${GTSAM_VERSION_PATCH}")

if ("${GTSAM_PRERELEASE_VERSION}" STREQUAL "")
if (DEFINED ENV{NIGHTLY})
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to maybe re-brand to gtsam-develop, as in our slack discussion.

CMakeLists.txt Outdated
@@ -10,10 +10,16 @@ set (GTSAM_VERSION_PATCH 0)
set (GTSAM_PRERELEASE_VERSION "a0")
math (EXPR GTSAM_VERSION_NUMERIC "10000 * ${GTSAM_VERSION_MAJOR} + 100 * ${GTSAM_VERSION_MINOR} + ${GTSAM_VERSION_PATCH}")

if ("${GTSAM_PRERELEASE_VERSION}" STREQUAL "")
if (DEFINED ENV{NIGHTLY})
string(TIMESTAMP NOW "%Y.%m.%d.%H.%M")
Copy link
Member

Choose a reason for hiding this comment

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

And make this the PR merge commit hash

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 21, 2025

Ah, finally something from PyPa that does all the stuff we had to do in gtsam-manylinux-build! Please be sure to generate the .pyi type annotation files (refer to python-stubs target in CMake) as well and put that inside the wheels.

@ProfFan
Copy link
Collaborator

ProfFan commented Feb 21, 2025

Also please check the built macOS wheels locally if they work. delocate sometimes would leave some dylibs linking against system dependencies, not sure if they are good now.

@yambati03
Copy link
Author

yambati03 commented Feb 21, 2025

@dellaert It looks like having the Git commit hash in the version string may not be possible. See the PyPA docs.

It seems that version specifiers must comply with the format [N!]N(.N)*[{a|b|rc}N][.postN][.devN]. So, something like 4.3a0 complies with this. If we wanted to add the commit hash to gtsam-develop builds, it would have to look like 4.3a0+xxxxxxx. This is because an arbitrary string, such as the commit hash, does not comply with the above format. Thus, it can only be included as a "local identifier" (which must be delineated with a +). However, local identifiers are explicitly disallowed on PyPI. See this PR reply, and the quote from PyPA below.

Local version identifiers SHOULD NOT be used when publishing upstream projects to a public index server, but MAY be used to identify private builds created directly from the project source.

What do you think about the versioning scheme {major}.{minor}{prerelease}.dev{datetime} for use on PyPI instead?

@dellaert
Copy link
Member

@dellaert It looks like having the Git commit hash in the version string may not be possible. See the PyPA docs.

It seems that version specifiers must comply with the format [N!]N(.N)*[{a|b|rc}N][.postN][.devN]. So, something like 4.3a0 complies with this. If we wanted to add the commit hash to gtsam-develop builds, it would have to look like 4.3a0+xxxxxxx. This is because an arbitrary string, such as the commit hash, does not comply with the above format. Thus, it can only be included as a "local identifier" (which must be delineated with a +). However, local identifiers are explicitly disallowed on PyPI. See this PR reply, and the quote from PyPA below.

Local version identifiers SHOULD NOT be used when publishing upstream projects to a public index server, but MAY be used to identify private builds created directly from the project source.

What do you think about the versioning scheme {major}.{minor}{prerelease}.dev{datetime} for use on PyPI instead?

That works. Maybe make sure then that you always use 2 digits for the month. E.g.,
25.02.21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to the Continuous Integration pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants