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

BLD/RLS: Update wheels to include GDAL 3.9.2 and add manylinux_2_28 wheels #466

Merged
merged 11 commits into from
Sep 5, 2024

Conversation

brendan-ward
Copy link
Member

@brendan-ward brendan-ward commented Aug 29, 2024

supersedes #451 , resolves #454

Uses GDAL 3.9.2 from VCPKG, which requires bumping this to the next minor release (0.10.0), and enables the libkml features (#447). Also adds tests for GDAL 3.9.2 to CI.

This switches to pulling the Arm64 MacOS wheel when testing on the macos-latest runner, which is MacOS / Arm64 (resolves #454) and uses --no-index flag to uv pip install to ensure it doesn't silently fall back to using an Arm64 wheel from PyPI.

This switches to uv for installing Python dependencies, in line with #438, which also gets around issues with Python 3.12 requiring a virtual environment (needed for testing the sdist using the gdal:ubuntu-small-3.9.2 base image which has Python 3.12).

(unlike #451 this does not drop manylinux2014)

@brendan-ward brendan-ward added this to the 0.10.0 milestone Aug 29, 2024
@brendan-ward
Copy link
Member Author

The failure on wheel building on Linux appears to be version conflicts with minizip (used by libkml) and zlib (installed via custom port). Looks like we need to add a custom port here for minizip at the same version as zlib.

@brendan-ward
Copy link
Member Author

Unfortunately, libkml 1.3.0 requires zlib / minizip >= 1.2.8. 1.3.0 was released in 2015, so already very old, and going for previous versions means not using cmake to build, which will likely be a pain to get working.

So - we have a couple of paths forward:

  • drop attempt to include libkml in wheels and direct users to conda-forge to use the libkml driver (status quo); this means we can also drop the custom port of minizip
  • drop use of manylinux2014, which enables us to stop using the custom ports of zlib and minizip and can successfully install libkml; however as mentioned in BLD/RLS: manylinux2014 at end of life #453, this may mean no longer supporting older OS versions

I think we should go with the first option; it allows us to upgrade GDAL in wheels, but is otherwise no worse than the wheels we're currently building.

@brendan-ward brendan-ward changed the title BLD/RLS: Update wheels to include GDAL 3.9.2 and libkml (attempt 2) BLD/RLS: Update wheels to include GDAL 3.9.2 Aug 29, 2024
@brendan-ward brendan-ward marked this pull request as ready for review August 29, 2024 18:44
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Looking good!

run: |
cd ..
python3 -m pytest --pyargs pyogrio.tests -v
python -c "import pyogrio; print(f'GDAL version: {pyogrio.__gdal_version__}\nGEOS version: {pyogrio.__gdal_geos_version__}')"
Copy link
Member

Choose a reason for hiding this comment

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

Not too important (this way is fine as well), but I think the uv run python -m pytest .. should work here, because we are not in the root directory with a pyproject.toml file because of the cd .. (in contrast to the docker tests, I suppose)

Copy link
Member Author

Choose a reason for hiding this comment

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

This worked fine, but we still need to set the environment variables to automatically activate uv, so it doesn't obviate the need for those.

Copy link
Member

Choose a reason for hiding this comment

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

Not that it matters too much, but just because I wanted to try to understand uv better (new to it), I think what is going on:

  • the reason we need to set the env variables (the reason uv does not find the venv automatically) is probably because of the cd ... From the uv run docs:

    When used outside a project, if a virtual environment can be found in the current directory or a parent directory, the command will be run in that environment. Otherwise, the command will be run in the environment of the discovered interpreter.

    so because of the cd .. the venv directory is actually in a directory down, and not in the current or parent directory.

  • We need the cd .. in this case to have the tests run properly on the installed package, but in case we didn't and wanted to run uv from the root without considering it as a project, that might work by putting a [tool.uv] managed = false in the pyproject.toml

.github/workflows/release.yml Outdated Show resolved Hide resolved
Comment on lines 253 to 307
python -m pip install --no-deps geopandas
uv pip install geopandas
Copy link
Member

Choose a reason for hiding this comment

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

It might not be strictly needed, but I would keep the --no-deps here for geopandas, just to avoid we install a released pyogrio that is then overwritten below

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it might definitely be needed, because from the logs it seems it then skips installing the local wheel because pyogrio is already installed?

We could also swap the order to prevent this (first install the local wheel before installing geopandas, that's also the order we use for the sdist tests). But would also still use --no-deps to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, yes, that was a mistake to remove --no-deps. Switched order.

run: |
cd ..
python3 -m pytest --pyargs pyogrio.tests -v
python -c "import pyogrio; print(f'GDAL version: {pyogrio.__gdal_version__}\nGEOS version: {pyogrio.__gdal_geos_version__}')"
Copy link
Member

Choose a reason for hiding this comment

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

Not that it matters too much, but just because I wanted to try to understand uv better (new to it), I think what is going on:

  • the reason we need to set the env variables (the reason uv does not find the venv automatically) is probably because of the cd ... From the uv run docs:

    When used outside a project, if a virtual environment can be found in the current directory or a parent directory, the command will be run in that environment. Otherwise, the command will be run in the environment of the discovered interpreter.

    so because of the cd .. the venv directory is actually in a directory down, and not in the current or parent directory.

  • We need the cd .. in this case to have the tests run properly on the installed package, but in case we didn't and wanted to run uv from the root without considering it as a project, that might work by putting a [tool.uv] managed = false in the pyproject.toml

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

One small last comment

@jorisvandenbossche jorisvandenbossche changed the title BLD/RLS: Update wheels to include GDAL 3.9.2 BLD/RLS: Update wheels to include GDAL 3.9.2 and add manylinux_2_28 wheels Sep 5, 2024
@jorisvandenbossche jorisvandenbossche merged commit a5fccad into main Sep 5, 2024
54 checks passed
@jorisvandenbossche jorisvandenbossche deleted the add_gdal_3.9.2 branch September 5, 2024 06:23
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.

BUG: wheel tests on macos-latest use the latest Arm64 version of pyogrio from PyPI
2 participants