BLD: Wheels! #2197

greglucas · 2023-06-25T21:03:49Z

~~This is one fairly large commit on top of the commits removing GEOS in PR #2083.~~

Updates the pyproject.toml file to reduce the size of setup.py and move towards the standard
Removes all of the conda environment setup and testing and switches over to pip for all of the installations on GitHub Actions (also removes appveyor and uses GitHub Actions for the windows runners now)
Adds cibuildwheel to the release for building wheels for the project

~~Right now, this is mostly just to demonstrate that we can build wheels (tested the produced artifacts from my fork and seemed to be good from both the sdist and wheels).~~

Closes #2216

greglucas · 2023-07-14T23:47:28Z

Alright, GEOS is now gone from Cartopy and we can start building wheels 🚀

This should be ready for review now, and it would be good if someone could look at this and make sure it all makes sense. Maybe pull down this branch and trying building it locally to make sure that the contents of the wheels are what we expect. I am definitely not an expert on build contents.

Once this PR goes in, we will need to remove Appveyor from the repository because it appears like it is triggering even without a .appveyor file in this PR.

varchasgopalaswamy · 2023-07-15T00:38:36Z

I was able to build a wheel (pip wheel --wheel-dir='./wheelhouse') and install from that wheel (pip install wheelhouse/Cartopy-0.20.2-cp311-linux_x86_64.whl). Python 3.11, WSL ubuntu on Win11. Looks like it loaded up just fine! Ran the example of a line from NYC to Delhi and it looks good to me.

rcomer · 2023-07-15T07:46:52Z

I can’t help with the technical side of this, but it might be good to also update this page.

greglucas · 2023-07-16T19:08:19Z

@varchasgopalaswamy, thank you for checking!

pip install wheelhouse/Cartopy-0.20.2-cp311-linux_x86_64.whl

That version doesn't seem quite right to me. My initial guess is you just haven't pulled down the latest tags? Can you try pulling the remote tags git fetch --tags and pulling again to make sure we are getting the right version on your system. For reference, this is what I get locally on a mac following your instructions above: pip install wheelhouse/Cartopy-0.21.1.dev183+g2dbf5b34-cp311-cp311-macosx_10_15_x86_64.whl

I can’t help with the technical side of this, but it might be good to also update this page.

@rcomer, sure I updated some of that page, let me know what you think of the rewording.

INSTALL

rcomer · 2023-07-16T19:43:18Z

I found a typo, buy otherwise the rewording looks good to me 😀

dopplershift

🎉 Awesome to see this. Overall this seems pretty sensible, though the cibuildwheel stuff was new dark magic to me.

.github/workflows/release.yml

dopplershift

Looks good to me. I'll leave it open for a bit for any comments, but I'm happy to see this go in and get us closer to a 0.22 release.

dopplershift · 2023-07-17T22:43:29Z

@greglucas Note there's a conflict now in environment.yml.

.github/workflows/ci-testing.yml

.github/workflows/release.yml

QuLogic · 2023-07-19T08:08:47Z

.github/workflows/release.yml

+          path: ./wheelhouse/*.whl
+
+  build_sdist:
+    name: Build source distribution


We may, as in Matplotlib's current build, want to build sdist first and instruct cibuildwheel to build out of that.

I'm a bit out of my element here. I tried to do that with a new commit on a separate branch.
https://github.com/greglucas/cartopy/tree/setup-updates-tmp

Failed job: https://github.com/greglucas/cartopy/actions/runs/5601696424/jobs/10245962338

I think it is failing because the sdist isn't being built quite right. It is not cythonizing to convert from .pyx to .cpp, where we need to include the .cpp in the sdist to distribute into the wheel. I'm not sure where we are supposed to build the extension then, do we just call python setup.py build_ext as a separate step to make sure the .cpp is created?

If it is easier for you to take over some of this, feel free to branch off yourself or push directly to my branch. I'm definitely happy to have some help on this.

OK, I think we just needed to call cythonize() if we are going down the non-sdist route, so I've reworked the logic a bit in setup.py to try and simplify things a bit more for whether we are in the Cython route or the pre-made cpp route.
Here is a CI run from a separate branch on my fork that demonstrates it working now when building the wheels from the sdist: https://github.com/greglucas/cartopy/actions/runs/5612238838/jobs/10269964395

Maybe this is out of scope for this PR, but any thoughts about using numba to replace the .pyx file?
Upside: No need to compile the .pyx file during build and deal with correctly packaging it. Plus numba has support for automatic parallelization.
Downside: Another dependency.

Definitely out of scope for this PR. If you can show that it is faster than Cython or makes things easier in some way, then a PR would definitely be considered!

As much as I think Numba is cool, it has its own challenges due to its reliance on bridging LLVM and Python. For instance, Numba didn't support Python 3.11 until May 2023, which IMO is unacceptable for something we'd consider as a dependency. I understand the dev team is aware how problematic this is and intends to do better for future releases, but I'd want to see a good track record going forward before we begin to consider that.

Now that we are preparing to build wheels, I wonder if all the work to make Cython optional is all that useful? It was a convenience to not require it, I suppose, but with stuff like build-requires and isolated build venvs and the fact that we're going to make wheels, reducing the need to build from source, maybe that isn't so great as it once was.

Possibly we could take it out... The Cython docs do mention to use an environment variable USE_CYTHON to control this, so I think we are doing similar to their recommendations. Doesn't hurt to keep it I guess?
https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#distributing-cython-modules

.github/workflows/release.yml

lib/cartopy/trace.pyx

pyproject.toml

greglucas · 2023-07-28T01:54:55Z

@QuLogic, thanks for the review comments! I think I addressed them all, but let me know if there was anything else I missed. Otherwise, I think it could still use a once-over to see if you spot anything I did odd when making those updates too.

.circleci/config.yml

.github/workflows/ci-testing.yml

.github/workflows/release.yml

Simplify some of the setup.py build work now that we have removed more extension libraries. Move the static content over to pyproject.toml and migrate more content to the modern build utilities.

QuLogic · 2023-08-01T02:34:38Z

.github/workflows/release.yml

+    # Only publish on releases
+    if: github.event_name == 'release'


Isn't this redundant with the entire workflow's on trigger? Should that be removed?

You beat me to this :) I was just going to add a comment here indicating I added this. I agree it is the same as the trigger, but this is an extra safeguard that I think is worth it in this case. For example, when I was testing building wheels on my fork, I removed the "on: release", but this would fail because I don't have credentials. However, then I force-pushed to this branch, and all of a sudden triggered this workflow on the PR. I'm not entirely clear it would have gone through the action but it seems prudent to keep to me just to make sure I don't fat finger something again on accident. In the future, we may want to change the trigger to let us trigger it automatically on certain pushes or on request.

PRs shouldn't have any of the credentials/write permissions necessary.

Also, there's no way this workflow as written should be able to trigger on a PR.

That said, there's no harm in being careful.

QuLogic

We can followup the other questions in other PRs.

greglucas added the Component: installation label Jun 25, 2023

greglucas added this to the 0.22 milestone Jun 25, 2023

rcomer mentioned this pull request Jul 2, 2023

cartopy not working on Windows, neither with python 3.10 nor with python 3.11 #2205

Closed

greglucas force-pushed the setup-updates branch 3 times, most recently from 62f96de to 0625e41 Compare July 14, 2023 23:39

greglucas force-pushed the setup-updates branch from 0625e41 to 2dbf5b3 Compare July 16, 2023 18:59

rcomer reviewed Jul 16, 2023

View reviewed changes

INSTALL Outdated Show resolved Hide resolved

greglucas force-pushed the setup-updates branch from 2dbf5b3 to 55e6a67 Compare July 16, 2023 19:47

dopplershift reviewed Jul 17, 2023

View reviewed changes

.github/workflows/release.yml Show resolved Hide resolved

dopplershift approved these changes Jul 17, 2023

View reviewed changes

greglucas force-pushed the setup-updates branch 2 times, most recently from d142c49 to 800d05a Compare July 19, 2023 01:57

QuLogic reviewed Jul 19, 2023

View reviewed changes

greglucas force-pushed the setup-updates branch 2 times, most recently from 1d780c9 to 4475052 Compare July 20, 2023 14:39

QuLogic reviewed Aug 1, 2023

View reviewed changes

BLD: cleanup setup.py and pyproject.toml

c3ea9f3

Simplify some of the setup.py build work now that we have removed more extension libraries. Move the static content over to pyproject.toml and migrate more content to the modern build utilities.

greglucas force-pushed the setup-updates branch from 4475052 to c3ea9f3 Compare August 1, 2023 02:28

QuLogic reviewed Aug 1, 2023

View reviewed changes

QuLogic approved these changes Aug 1, 2023

View reviewed changes

QuLogic merged commit 3ff2b13 into SciTools:main Aug 1, 2023
11 checks passed

greglucas deleted the setup-updates branch August 1, 2023 20:40

greglucas mentioned this pull request Aug 4, 2023

CI: Change pip install pin for releasing #2226

Merged

JoranAngevaare mentioned this pull request Aug 9, 2023

Move cartopy to pip JoranAngevaare/optim_esm_base#141

Closed

BSchilperoort mentioned this pull request Mar 27, 2024

Reorganize optional dependencies WAM2layers/WAM2layers#345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD: Wheels! #2197

BLD: Wheels! #2197

greglucas commented Jun 25, 2023 •

edited

Loading

greglucas commented Jul 14, 2023

varchasgopalaswamy commented Jul 15, 2023

rcomer commented Jul 15, 2023

greglucas commented Jul 16, 2023

rcomer commented Jul 16, 2023

dopplershift left a comment

dopplershift left a comment

dopplershift commented Jul 17, 2023

QuLogic Jul 19, 2023

greglucas Jul 20, 2023

greglucas Jul 20, 2023

greglucas Jul 20, 2023

varchasgopalaswamy Jul 20, 2023

greglucas Jul 20, 2023

dopplershift Jul 20, 2023

QuLogic Aug 1, 2023

greglucas Aug 1, 2023

greglucas commented Jul 28, 2023

QuLogic Aug 1, 2023

greglucas Aug 1, 2023

dopplershift Aug 1, 2023

QuLogic left a comment

		# Only publish on releases
		if: github.event_name == 'release'

BLD: Wheels! #2197

BLD: Wheels! #2197

Conversation

greglucas commented Jun 25, 2023 • edited Loading

greglucas commented Jul 14, 2023

varchasgopalaswamy commented Jul 15, 2023

rcomer commented Jul 15, 2023

greglucas commented Jul 16, 2023

rcomer commented Jul 16, 2023

dopplershift left a comment

Choose a reason for hiding this comment

dopplershift left a comment

Choose a reason for hiding this comment

dopplershift commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglucas commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic left a comment

Choose a reason for hiding this comment

greglucas commented Jun 25, 2023 •

edited

Loading