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

Modernize CI/CD Pipeline with cibuildwheel #612

Closed
wants to merge 31 commits into from

Conversation

cvanelteren
Copy link

Changes

This PR modernizes our GitHub Actions workflows by consolidating the existing four workflow files into two streamlined workflows using cibuildwheel.

Main improvements:

  • Replaces custom Docker-based builds with industry-standard cibuildwheel
  • Adds proper MacOS wheel building support (previously manual)
  • Adds TestPyPI deployment stage for safer releases
  • Implements modern trusted publishing for PyPI uploads
  • Consolidates redundant workflows while maintaining functionality
  • Improves cross-platform compatibility
  • Simplifies maintenance and reduces potential points of failure

Technical Details:

  • Consolidated workflows:
    • build.yml: Handles main basemap package (replaces manylinux and windows workflows)
    • build-data.yml: Handles data packages (replaces both data package workflows)
  • Added proper testing across all platforms
  • Upgraded to latest GitHub Actions features (v4)
  • Implemented sequential TestPyPI → PyPI deployment

Required Actions:

  • Set up repository environments for TestPyPI and PyPI
  • Configure trusted publishing on both PyPI and TestPyPI
  • Update repository secrets if needed

Testing

The new workflows have been tested with:

  • Python versions 3.9 through 3.13
  • Linux, Windows, and MacOS platforms
  • Both TestPyPI and PyPI deployments

Related to issue PR #611

@cvanelteren
Copy link
Author

Not entirely sure why those extra commits are in this PR. My local log shows it clean

commit f488008bbb3a3ae09f126585e18d10cae584877f (HEAD -> workflow-fix, origin/workflow-fix)
Author: cvanelteren <[email protected]>
Date:   Sun Feb 16 10:32:38 2025 +0100

    drop eol py versions

commit 4d96b1f305d30d28a7334f3baf66e37c7d192a89
Author: cvanelteren <[email protected]>
Date:   Sun Feb 16 10:28:35 2025 +0100

    consolidate workflows

commit 8c4f7f49026be9c75f7ef8ae9f41e8923978706f
Author: cvanelteren <[email protected]>
Date:   Sun Feb 16 10:27:04 2025 +0100

    consolidate workflows into singular file

commit c461fa224abd19683431e4ac60879ec96a0be84a (tag: v1.4.1, mpl/master, master)
Merge: 9921f135 01b2218c
Author: Víctor Molina García <[email protected]>
Date:   Thu Feb 15 14:29:59 2024 +0100

@cvanelteren
Copy link
Author

cvanelteren commented Feb 16, 2025

Note I would also say we could use pyproject.toml to determine the supported versions similar to what we did here. This would make pyproject.toml the only point of entry to determine what versions are supported rather than manually specifying it in two places.

@molinav
Copy link
Member

molinav commented Feb 16, 2025

Note I would also say we could use pyproject.toml to determine the supported versions similar to what we did here. This would make pyproject.toml the only point of entry to determine what versions are supported rather than manually specifying it in two places.

I think it is a good idea to migrate, step by step, all these legacy remainders into the current standards wherever possible, so please feel free to make these changes in other PRs. A lot of the legacy debt comes from keeping support for older Python versions that do not understand the new packaging standards. Now that we want to get rid of this in the develop branch, we do not have to constrain ourselves to the old way of doing things.

I really appreciate all your help, @cvanelteren, I think I will also learn a lot thanks to you, because I have been a bit stuck in the last couple of years: the Python packaging has moved a lot and I could not keep track of it.

@molinav
Copy link
Member

molinav commented Feb 16, 2025

The PR is mentioning a conflict because I think you forked from the master branch and not from the develop branch. The develop branch had a minor change in the basemap-for-manylinux workflow that is not relevant anymore because you are deleting the file. In your local copy, you need to merge the develop branch into your PR branch, and resolve the conflict by saying that your change is the one that needs to remain (i.e. the deletion of the file).

@cvanelteren
Copy link
Author

Ah thanks @molinav. Rebased it to develop. I believe the workflow had some other issues that I will focus on now.

@molinav
Copy link
Member

molinav commented Feb 16, 2025

I think the GEOS changes will need a revisit. The idea is that we keep control of the version that we bundle with the basemap wheels. For that, my helper class GeosLibrary takes care of downloading a specific GEOS version from upstream and builds it from source. In principle, the only requirements for GeosLibrary are a C++ toolchain, Git and Python. This means that we do not need to install anything GEOS-related from the host machine.

The GEOS version remains in 3.6.x because from 3.7.x it was not possible to build anymore with GCC 4.x. This GEOS version can be bumped in a separate PR once the new workflows work.

@molinav
Copy link
Member

molinav commented Feb 16, 2025

I see now that a lot of files have changed, also some that are not directly related to the cibuildwheel problem. The setup.py file used to handle the build appropriately if the GEOS_DIR environment variable was defined, but now this part has changed too and I cannot have the whole overview anymore.

Maybe it is better if the PR is started again with the focus only in the cibuildwheel thing. Even if the setup.py and setup.cfg files are old style, cibuildwheel should be able to build the package in its original state. The only issue with the old CI is the incapability of using the v4 of the artifacts actions.

Once the cibuildwheel migration is ready, then the packaging migration can take the focus, but not both at the same time, because it is easy to end up with a messy situation.

@cvanelteren
Copy link
Author

I was searching in the wrong direction. I couldn't figure out why the build was failing. Apparantly cibuildwheels does not expand {project} for gcc which is now fixed. I can restored these files or leave them in. There was also an issue with the pyx file that needed fixing. The other stuff move the setup towards a pyproject.toml which would still within the modernization I reckon, but it is up to you.

@cvanelteren
Copy link
Author

I can remove

commit a282d482e7a35f0702679d26c2470a6f4d90e137
Author: cvanelteren <[email protected]>
Date:   Sun Feb 16 15:48:25 2025 +0100

    prepping basemap for pyproject.toml

commit 7d8c6b32da2c33d1dc738e38ff6e10f13e05019d
Author: cvanelteren <[email protected]>
Date:   Sun Feb 16 15:47:34 2025 +0100

    prepping data_hires for pyproject.toml

commit 354278d3baac27256c3af9761dd6f49a3d9af7aa
Author: cvanelteren <[email protected]>
Date:   Sun Feb 16 15:47:06 2025 +0100

    prepping data for pyproject.toml

commit c9e25c7b8296be06fb97e377eb4a47a8790fd87e
Author: cvanelteren <[email protected]>
Date:   Sun Feb 16 15:46:22 2025 +0100

    prepping to convert to toml

to clean up the commits and keep the original focus on CI.

@cvanelteren cvanelteren marked this pull request as draft February 16, 2025 18:58
@cvanelteren
Copy link
Author

still some issues with the path. It's bit annoying to figure out as locally the builds are running fine and I have to push to emulate the github workflow. I will setup act to check before converting it back to a PR.

@molinav
Copy link
Member

molinav commented Feb 16, 2025

@cvanelteren I would suggest that you continue debugging in the current PR, but the final changes should rather be proposed in new separate PRs, because I cannot follow the changes.

I would like to describe my view a bit more. Due to my lack of free time, the repository has been outdated for long and this brought several issues. These issues are, however, clearly separable:

  1. The GitHub workflows cannot run because of the sunset of the artifact actions previous to version 4.
  2. basemap has outdated dependencies.
  3. basemap uses old-style configuration files to define the packaging.

People realise at some point that there is a problem with basemap because they cannot install it with Python 3.13 or because they cannot install it in combination with numpy>=2.0 (Issue 2 above), and this needs to be fixed. However, without proper GitHub workflows running (Issue 1 above), the changes to solve the end-user problem (Issue 2 above) cannot be tested. None of these problems involve Issue 3 above, which is a desire but not a mandatory change to have everything running again.

To properly address the problem, the first thing is to fix the GitHub workflows (Issue 1). It must be noted that the basemap repository in its current state builds successfully for the operating systems, Python versions and dependencies that it defines as supported. Solving Issue 1 means that we can guarantee again that the repository is found in a stable state, with old-style configuration files, with outdated dependencies. That is still fine. The basemap package is not broken, the basemap package is outdated.

Once the GitHub workflows are running again, then we can start addressing Issue 2 and Issue 3 (and only one at a time). Not before, because any changes in Point 2 and Point 3 without solving Point 1 first will be untested. This is fine when the changes do not introduce bugs, but it is problematic when the changes introduce unexpected behaviour, and we cannot predict in which situation we will find ourselves after some change.

In the case of using cibuildwheel to solve Issue 1, there is only one thing from Issue 2 that needs to be touched due to cibuildwheel limitations in its latest versions: set a new minimum supported Python version (>=3.6). Nothing else needs to be touched regarding dependencies (Issue 2) or configuration files (Issue 3), because basemap should build in its current state even using python -m build despite of its old style.

I want to list two items (touching Issue 3 while addressing Issue 1) that support my proposal from above and why it is better to address one problem at a time:

  • The removal of the cython_directives in the setup.py file from basemap is the reason why later the Cython extension was not compiling without explicitly writing noexcept in _geoslib.pyx. This was addressed correctly in the setup.py before the PR and has nothing to do with the migration of the GitHub workflows, but introduced an issue that had to be solved.
  • The build of a basemap wheel needs only the definition of the environment variable GEOS_DIR, and only if the GEOS library is not found in standard locations (i.e. when building ourselves from source in the GitHub workflows). As the setup.py file was written before, no CFLAGS or LDFLAGS or LD_LIBRARY_PATH are needed, because the setup.py prepares everything correctly in advance, independently of the operating system. If these variables are now really needed, it implies that something has been broken in the former setup.py while touching it.

I still want to stress that I appreciate a lot all your efforts, @cvanelteren. I am aware that my limited availability has been blocking basemap in the last months, and you are making valuable changes with your knowledge of cibuildwheel that nobody else has done here before. All my comments above are only directed to guarantee that we merge into develop something in a stable state, and this is much easier when PRs address one specific problem at a time.

I recently had the bad experience with basemap-feedstock in which changes were merged into the main branch with errors in the recipe, and I still feel guilty for not reviewing the proposed changes on time, which resulted in the erroneous PR being merged into main without external reviewers and having garbage packages uploaded to conda-forge that now will stay there forever, because conda-forge does not allow the removal of uploaded packages. It was like 3 weeks ago and I still think of it, because I would have seen the error immediately if I had had the time to review the PR (I had to overcome this error appropriately when I joined the basemap-feedstock project some years ago).

@cvanelteren
Copy link
Author

Ok completely understand. Don't worry I will fix it here s d then ea. It up on a separate PR 🫡.

@cvanelteren
Copy link
Author

Closing in favor of #614

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