CI: Add a separate CI job on Ubuntu to test geopandas v0.x #3420

seisman · 2024-08-29T09:14:08Z

Description of proposed changes

In #3247, we added codes to deal with compatibility with both GeoPandas v0.x and v1.x, but our CI jobs only install GeoPandas v1.x now, leaving the codes for geopandas v0.x untested (see https://app.codecov.io/gh/GenericMappingTools/pygmt/blob/main/pygmt%2Fhelpers%2Ftempfile.py#L146).

This PR adds a separate CI job in which geopandas v0.x is installed. The CI job has to use Python 3.11, otherwise other Python 3.10/3.12 jobs will be override by this one. The job also only runs on Ubuntu, to avoid using too many CI resources.

With the new CI job, the geopandas v0.x codes are covered and the code coverage increases by 0.32%.

.github/workflows/ci_tests.yaml

Co-authored-by: Yvonne Fröhlich <[email protected]>

weiji14

Thinking if we should generalize this to test minimum supported versions of all optional dependencies (contextily, geopandas, rioxarray, etc). uv has this --resolution lowest-direct flag (see here) that "will use the lowest compatible versions for all direct dependencies, while using the latest compatible versions for all other dependencies". Would be nice if mamba had something similar.

.github/workflows/ci_tests.yaml

weiji14 · 2024-09-04T03:12:52Z

.github/workflows/ci_tests.yaml

@@ -79,6 +79,14 @@ jobs:
            pandas-version: ''
            xarray-version: ''
            optional-packages: ' contextily geopandas ipython pyarrow rioxarray sphinx-gallery'
+          # The job below is for testing geopandas v0.x on Ubuntu. Python 3.11 is used


This PR adds a separate CI job in which geopandas v0.x is installed. The CI job has to use Python 3.11, otherwise other Python 3.10/3.12 jobs will be override by this one. The job also only runs on Ubuntu, to avoid using too many CI resources.

Could we test geopandas v0.x in ci_tests_legacy.yaml instead? Or do we want to test it more regularly than that?

We can, but in coverage reports the geopandas v0.x would be uncovered since the Legacy workflow doesn't produce coverage reports.

seisman · 2024-09-04T03:34:52Z

Thinking if we should generalize this to test minimum supported versions of all optional dependencies (contextily, geopandas, rioxarray, etc). uv has this --resolution lowest-direct flag (see here) that "will use the lowest compatible versions for all direct dependencies, while using the latest compatible versions for all other dependencies". Would be nice if mamba had something similar.

it sounds a good idea, but we are approaching the 10GB size limit of GitHub caches https://github.com/GenericMappingTools/pygmt/actions/caches.

Co-authored-by: Wei Ji <[email protected]>

seisman · 2024-09-13T00:28:25Z

Thinking if we should generalize this to test minimum supported versions of all optional dependencies (contextily, geopandas, rioxarray, etc).

We should note that we currently don't set the minimum required versions for the optional dependencies.

seisman · 2024-09-14T15:27:54Z

The CI job with geopandas v0.x worked in previous runs (e.g., https://github.com/GenericMappingTools/pygmt/actions/runs/10694655883/job/29646773799?pr=3420) and suddenly failed now (e.g., https://github.com/GenericMappingTools/pygmt/actions/runs/10857583544/job/30146872146?pr=3420).

=================================== FAILURES ===================================
______________________ test_geopandas_plot_int64_as_float ______________________
[gw0] linux -- Python 3.11.10 /home/runner/micromamba/envs/pygmt/bin/python3.11

args = ()
kwargs = {'gdf_ridge':    PLATEID1  ...                                           geometry
0     101.0  ...  POLYGON ((-30.5584....72844, -34.56910 36.768...
2     101.0  ...  POLYGON ((-38.18994 34.45598, -38.15845 34.514...

[3 rows x 21 columns]}

    def wrapper(*args, **kwargs):
>       store.return_value[test_name] = obj(*args, **kwargs)

../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: in wrapper
    store.return_value[test_name] = obj(*args, **kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: in wrapper
    store.return_value[test_name] = obj(*args, **kwargs)
../pygmt/tests/test_geopandas.py:234: in test_geopandas_plot_int64_as_float
    fig.plot(
../pygmt/helpers/decorators.py:609: in new_module
    return module_func(*args, **kwargs)
../pygmt/helpers/decorators.py:773: in new_module
    return module_func(*bound.args, **bound.kwargs)
../pygmt/src/plot.py:258: in plot
    with lib.virtualfile_in(
../../../../micromamba/envs/pygmt/lib/python3.11/contextlib.py:137: in __enter__
    return next(self.gen)
../pygmt/helpers/tempfile.py:164: in tempfile_from_geojson
    geojson.to_file(**ogrgmt_kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/geopandas/geodataframe.py:1249: in to_file
    _to_file(self, filename, driver, schema, index, **kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/geopandas/io/file.py:610: in _to_file
    _to_file_fiona(df, filename, driver, schema, crs, mode, **kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/geopandas/io/file.py:641: in _to_file_fiona
    colxn.writerecords(df.iterfeatures())
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/fiona/collection.py:541: in writerecords
    self.session.writerecs(records, self)
fiona/ogrext.pyx:1666: in fiona.ogrext.WritingSession.writerecs
    ???
fiona/ogrext.pyx:788: in fiona.ogrext.OGRFeatureBuilder.build
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   OverflowError: value too large to convert to int

fiona/ogrext.pyx:243: OverflowError

The most notable difference is fiona v1.9 in old runs and v1.10 in the latest runs.

seisman · 2024-09-25T00:40:03Z

The CI job with geopandas v0.x worked in previous runs (e.g., https://github.com/GenericMappingTools/pygmt/actions/runs/10694655883/job/29646773799?pr=3420) and suddenly failed now (e.g., https://github.com/GenericMappingTools/pygmt/actions/runs/10857583544/job/30146872146?pr=3420).

=================================== FAILURES ===================================
______________________ test_geopandas_plot_int64_as_float ______________________
[gw0] linux -- Python 3.11.10 /home/runner/micromamba/envs/pygmt/bin/python3.11

args = ()
kwargs = {'gdf_ridge':    PLATEID1  ...                                           geometry
0     101.0  ...  POLYGON ((-30.5584....72844, -34.56910 36.768...
2     101.0  ...  POLYGON ((-38.18994 34.45598, -38.15845 34.514...

[3 rows x 21 columns]}

    def wrapper(*args, **kwargs):
>       store.return_value[test_name] = obj(*args, **kwargs)

../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: in wrapper
    store.return_value[test_name] = obj(*args, **kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/pytest_mpl/plugin.py:125: in wrapper
    store.return_value[test_name] = obj(*args, **kwargs)
../pygmt/tests/test_geopandas.py:234: in test_geopandas_plot_int64_as_float
    fig.plot(
../pygmt/helpers/decorators.py:609: in new_module
    return module_func(*args, **kwargs)
../pygmt/helpers/decorators.py:773: in new_module
    return module_func(*bound.args, **bound.kwargs)
../pygmt/src/plot.py:258: in plot
    with lib.virtualfile_in(
../../../../micromamba/envs/pygmt/lib/python3.11/contextlib.py:137: in __enter__
    return next(self.gen)
../pygmt/helpers/tempfile.py:164: in tempfile_from_geojson
    geojson.to_file(**ogrgmt_kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/geopandas/geodataframe.py:1249: in to_file
    _to_file(self, filename, driver, schema, index, **kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/geopandas/io/file.py:610: in _to_file
    _to_file_fiona(df, filename, driver, schema, crs, mode, **kwargs)
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/geopandas/io/file.py:641: in _to_file_fiona
    colxn.writerecords(df.iterfeatures())
../../../../micromamba/envs/pygmt/lib/python3.11/site-packages/fiona/collection.py:541: in writerecords
    self.session.writerecs(records, self)
fiona/ogrext.pyx:1666: in fiona.ogrext.WritingSession.writerecs
    ???
fiona/ogrext.pyx:788: in fiona.ogrext.OGRFeatureBuilder.build
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   OverflowError: value too large to convert to int

fiona/ogrext.pyx:243: OverflowError

The most notable difference is fiona v1.9 in old runs and v1.10 in the latest runs.

Fixed in 63f1e74. The issue also highlights the importance of testing geopandas v0.x.

weiji14 · 2024-09-26T02:56:20Z

Thinking if we should generalize this to test minimum supported versions of all optional dependencies (contextily, geopandas, rioxarray, etc).

We should note that we currently don't set the minimum required versions for the optional dependencies.

Yeah, should we come up with a policy for this? E.g. extend SPEC 0 with a recommendation like:

Support for optional package dependencies be dropped 1 year after their initial release.

For reference, xarray seems to use 12 months as the default for most dependencies - https://github.com/pydata/xarray/blob/v2024.09.0/ci/min_deps_check.py#L35. Downside with this is more maintenance effort, since we need to update the minimum pins for optional dependencies more often. We could also just state this policy, but not have an explicit pin?

seisman · 2024-09-26T03:06:22Z

Downside with this is more maintenance effort, since we need to update the minimum pins for optional dependencies more often.

Yes, it's more maintenance burden to us. I actually prefer to not set any minimum pins for optional dependencies and rely on the package manager like mamba/conda to install whatever versions that are compatible with the core dependencies.

weiji14

Downside with this is more maintenance effort, since we need to update the minimum pins for optional dependencies more often.

Yes, it's more maintenance burden to us. I actually prefer to not set any minimum pins for optional dependencies and rely on the package manager like mamba/conda to install whatever versions that are compatible with the core dependencies.

Opened separate issue at #3456 to discuss this. Ok to merge this in now.

seisman added 3 commits August 29, 2024 17:02

Add a separate job for testing geopandas v0.x

8bd62d2

Python 3.11

603fd70

Remove name and add comments

d862ff9

seisman added maintenance Boring but important stuff for the core devs needs review This PR has higher priority and needs review. labels Aug 29, 2024

seisman added this to the 0.13.0 milestone Aug 29, 2024

seisman added 4 commits August 30, 2024 07:47

Merge branch 'main' into test/geopandas-v0

4018c6a

Merge branch 'main' into test/geopandas-v0

0cba582

Merge branch 'main' into test/geopandas-v0

3053fd6

Merge branch 'main' into test/geopandas-v0

53803a2

yvonnefroehlich reviewed Sep 4, 2024

View reviewed changes

.github/workflows/ci_tests.yaml Outdated Show resolved Hide resolved

Fix typos

ff57e87

Co-authored-by: Yvonne Fröhlich <[email protected]>

weiji14 reviewed Sep 4, 2024

View reviewed changes

Fix typo

0fd0681

Co-authored-by: Wei Ji <[email protected]>

seisman modified the milestones: 0.13.0, 0.14.0 Sep 4, 2024

seisman removed the needs review This PR has higher priority and needs review. label Sep 6, 2024

seisman marked this pull request as draft September 6, 2024 13:15

seisman added 2 commits September 14, 2024 07:46

Improve the comments

3dc8f2e

Merge branch 'main' into test/geopandas-v0

9ea946e

seisman added this to the 0.14.0 milestone Sep 13, 2024

seisman marked this pull request as ready for review September 13, 2024 23:47

seisman marked this pull request as draft September 14, 2024 15:41

seisman added 2 commits September 25, 2024 08:24

Merge branch 'main' into test/geopandas-v0

d83cc5e

Explicitely convert the column with overflow issue

63f1e74

seisman marked this pull request as ready for review September 25, 2024 00:42

seisman added the needs review This PR has higher priority and needs review. label Sep 25, 2024

seisman mentioned this pull request Sep 26, 2024

GeoPandas: Explicitly convert columns with overflow integers to avoid OverflowError with fiona 1.10 #3455

Merged

Merge branch 'main' into test/geopandas-v0

76b0e0b

Add one more comment

8b49046

weiji14 mentioned this pull request Sep 26, 2024

Support policy for optional dependencies (contextily, geopandas, rioxarray, etc) #3456

Closed

weiji14 approved these changes Sep 26, 2024

View reviewed changes

seisman removed the needs review This PR has higher priority and needs review. label Sep 26, 2024

seisman merged commit 6353926 into main Sep 26, 2024
16 checks passed

seisman deleted the test/geopandas-v0 branch September 26, 2024 05:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CI: Add a separate CI job on Ubuntu to test geopandas v0.x #3420

CI: Add a separate CI job on Ubuntu to test geopandas v0.x #3420

Uh oh!

seisman commented Aug 29, 2024 •

edited

Loading

Uh oh!

Uh oh!

weiji14 left a comment

Uh oh!

Uh oh!

weiji14 Sep 4, 2024

Uh oh!

seisman Sep 4, 2024

Uh oh!

seisman commented Sep 4, 2024

Uh oh!

seisman commented Sep 13, 2024

Uh oh!

seisman commented Sep 14, 2024

Uh oh!

seisman commented Sep 25, 2024 •

edited

Loading

Uh oh!

weiji14 commented Sep 26, 2024 •

edited

Loading

Uh oh!

seisman commented Sep 26, 2024

Uh oh!

weiji14 left a comment

Uh oh!

Uh oh!

Uh oh!

CI: Add a separate CI job on Ubuntu to test geopandas v0.x #3420

CI: Add a separate CI job on Ubuntu to test geopandas v0.x #3420

Uh oh!

Conversation

seisman commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weiji14 Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

seisman Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

seisman commented Sep 4, 2024

Uh oh!

seisman commented Sep 13, 2024

Uh oh!

seisman commented Sep 14, 2024

Uh oh!

seisman commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weiji14 commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seisman commented Sep 26, 2024

Uh oh!

weiji14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seisman commented Aug 29, 2024 •

edited

Loading

seisman commented Sep 25, 2024 •

edited

Loading

weiji14 commented Sep 26, 2024 •

edited

Loading