Add shim module and support for GDAL 3 #804

sgillies · 2019-10-17T18:27:39Z

I propose to merge this and make a 1.8.9 release without fixing the Appveyor build so that we can ease the PROJ 4/6 pain of users. Let's tackle the Appveyor updates in the master branch.

Summary of the PR:

Adds a shim module for GDAL 3, introduces two new functions (osr_get_name and osr_set_traditional_axis_mapping_strategy), and adds no-op versions of the functions to the older shims.
Everywhere we initialize an OGRSpatialReferenceH object using OSRImportFrom*, we set its axis order to the "traditional" one to preserve the existing axis order.
We disable two tests for GDAL 3 because towgs84 and wktext don't surface in GDAL's WKT output anymore.

@hobu you're interested in this, I know. Want to give it a quick scan? I realize that some of the Cython files are archaic and a bit opaque.

@snorfalorpagus any objections?

rbuffat · 2019-10-17T20:07:29Z

Regarding the "TypeError: attrib() got an unexpected keyword argument 'convert'" on travis this might be the reason: https://stackoverflow.com/questions/58189683/typeerror-attrib-got-an-unexpected-keyword-argument-convert

rbuffat · 2019-10-17T20:26:47Z

fiona/_shim3.pyx

@@ -0,0 +1,149 @@
+"""Shims on top of ogrext for GDAL versions >= 2.2"""


rbuffat · 2019-10-17T20:29:10Z

.travis.yml

-  - pip install -e .[test]
+  - "python -m pip wheel -r requirements-dev.txt"
+  - "python -m pip install -r requirements-dev.txt"
+  - "GDAL_CONFIG=$GDALINST/gdal-$GDALVERSION/bin/gdal-config python -m pip install --upgrade --force-reinstall --no-use-pep517 -e .[test]"


I suppose you didn't want to include the --upgrade again.

I'm going to leave it for now. This is copied from rasterio and we haven't had a problem there yet.

coveralls · 2019-10-17T21:11:56Z

Coverage increased (+0.03%) to 82.903% when pulling 7371b7d on gdal3 into da226d9 on maint-1.8.

sgillies · 2019-10-18T16:25:51Z

fiona/_shim1.pxd

@@ -19,6 +19,8 @@ cdef OGRFieldSubType get_field_subtype(void *fielddefn)
 cdef void set_field_subtype(void *fielddefn, OGRFieldSubType subtype)
 cdef bint check_capability_create_layer(void *cogr_ds)
 cdef void *get_linear_geometry(void *geom)
+cdef const char* osr_get_name(OGRSpatialReferenceH hSrs)


This function is copied from rasterio. It isn't called in fiona yet, but it will be in a future version as we change our modules to be more like those in rasterio. Therefore, I'm going to leave it in.

hobu · 2019-10-18T16:32:36Z

fiona/_crs.pyx

    else:
        raise ValueError("Invalid CRS")

-    # Fixup, export to WKT, and set the GDAL dataset's projection.
-    OSRFixup(cogr_srs)


I think this OSRFixup call should be still used in GDAL < 3.0. Supporting both version scenarios is probably complicated, however.

Do you see any harm in having osr_set_traditional_axis_mapping_strategy call OSRFixup for GDAL versions 1 and 2 @hobu ? That's one way to bring it back in.

No harm, but that's not quite where you want it. You want it right after any ImportFromWkt-type calls happen where the input might be ESRI content.

hobu · 2019-10-18T16:54:31Z

Some test failures in path finding stuff using @conda-forge:

(fiona) [hobu@kasai fiona (gdal3)]$ cs2cs
Rel. 6.2.0, September 1st, 2019
usage: cs2cs [-dDeEfIlrstvwW [args]] [+opt[=arg] ...]
                   [+to +opt[=arg] ...] [file ...]
(fiona) [hobu@kasai fiona (gdal3)]$ gdalinfo --version
GDAL 3.0.1, released 2019/06/28

_______________________ test_gdal_data_wheel ________________________

    @pytest.mark.wheel
    def test_gdal_data_wheel():
        """Get GDAL data path from a wheel"""
>       assert GDALDataFinder().search() == os.path.join(os.path.dirname(fiona.__file__), 'gdal_data')
E       AssertionError: assert None == '/Users/hobu/dev/git/fiona/fiona/gdal_data'
E        +  where None = <bound method GDALDataFinder.search of <fiona._env.GDALDataFinder object at 0x116194588>>()
E        +    where <bound method GDALDataFinder.search of <fiona._env.GDALDataFinder object at 0x116194588>> = <fiona._env.GDALDataFinder object at 0x116194588>.search
E        +      where <fiona._env.GDALDataFinder object at 0x116194588> = GDALDataFinder()
E        +  and   '/Users/hobu/dev/git/fiona/fiona/gdal_data' = <function join at 0x10f6cfe18>('/Users/hobu/dev/git/fiona/fiona', 'gdal_data')
E        +    where <function join at 0x10f6cfe18> = <module 'posixpath' from '/Users/hobu/miniconda3/envs/fiona/lib/python3.7/posixpath.py'>.join
E        +      where <module 'posixpath' from '/Users/hobu/miniconda3/envs/fiona/lib/python3.7/posixpath.py'> = os.path
E        +    and   '/Users/hobu/dev/git/fiona/fiona' = <function dirname at 0x10f6e7158>('/Users/hobu/dev/git/fiona/fiona/__init__.py')
E        +      where <function dirname at 0x10f6e7158> = <module 'posixpath' from '/Users/hobu/miniconda3/envs/fiona/lib/python3.7/posixpath.py'>.dirname
E        +        where <module 'posixpath' from '/Users/hobu/miniconda3/envs/fiona/lib/python3.7/posixpath.py'> = os.path
E        +      and   '/Users/hobu/dev/git/fiona/fiona/__init__.py' = fiona.__file__

tests/test_data_paths.py:16: AssertionError
_______________________ test_proj_data_wheel ________________________

    @pytest.mark.wheel
    def test_proj_data_wheel():
        """Get GDAL data path from a wheel"""
>       assert PROJDataFinder().search() == os.path.join(os.path.dirname(fiona.__file__), 'proj_data')
E       AssertionError: assert '/Users/hobu/...na/share/proj' == '/Users/hobu/d...ona/proj_data'
E         - /Users/hobu/miniconda3/envs/fiona/share/proj
E         + /Users/hobu/dev/git/fiona/fiona/proj_data

tests/test_data_paths.py:22: AssertionError
_____________________ test_env_gdal_data_wheel ______________________

    @pytest.mark.wheel
    def test_env_gdal_data_wheel():
        runner = CliRunner()
        result = runner.invoke(main_group, ['env', '--gdal-data'])
        assert result.exit_code == 0
>       assert result.output.strip() == os.path.join(os.path.dirname(fiona.__file__), 'gdal_data')
E       AssertionError: assert '/Users/hobu/...na/share/gdal' == '/Users/hobu/d...ona/gdal_data'
E         - /Users/hobu/miniconda3/envs/fiona/share/gdal
E         + /Users/hobu/dev/git/fiona/fiona/gdal_data

tests/test_data_paths.py:30: AssertionError
_____________________ test_env_proj_data_wheel ______________________

    @pytest.mark.wheel
    def test_env_proj_data_wheel():
        runner = CliRunner()
        result = runner.invoke(main_group, ['env', '--proj-data'])
        assert result.exit_code == 0
>       assert result.output.strip() == os.path.join(os.path.dirname(fiona.__file__), 'proj_data')
E       AssertionError: assert '/Users/hobu/...na/share/proj' == '/Users/hobu/d...ona/proj_data'
E         - /Users/hobu/miniconda3/envs/fiona/share/proj
E         + /Users/hobu/dev/git/fiona/fiona/proj_data

tests/test_data_paths.py:38: AssertionError
= 4 failed, 875 passed, 6 skipped, 1 xfailed, 6 xpassed in 11.08 seconds =

hobu · 2019-10-18T17:02:54Z

Small cast nit 1b5081a

hobu · 2019-10-18T18:07:48Z

Bumping up the requirements got me over the attrib issue on windows. The important one is coverage, but maybe it is time to bump the rest.

diff --git a/requirements-dev.txt b/requirements-dev.txt
index 975aeb3..7df9078 100644
--- a/requirements-dev.txt
+++ b/requirements-dev.txt
@@ -1,8 +1,8 @@
 -r requirements.txt
-coverage==4.5.1
-cython==0.28.3
-pytest==3.4.2
+coverage==4.5.4
+cython==0.29.13
+pytest==5.2.1
 pytest-cov==2.5.1
 setuptools==39.0.1
 boto3==1.9.19
-wheel==0.31.1
+wheel==0.31.1
\ No newline at end of file

sgillies · 2019-10-18T18:54:34Z

@hobu skip those tests with pytest -m "not wheel". I haven't yet figured out how to make these tests not run by default using a marker.

hobu · 2019-10-18T19:00:42Z

pytest -m "not wheel

ok, with that and the requirements bump, my windows tests are clean.

snowman2 · 2019-10-19T00:23:28Z

Everywhere we initialize an OGRSpatialReferenceH object using OSRImportFrom*, we set its axis order to the "traditional" one to preserve the existing axis order.

I would recommend against this approach as it will change the CRS and it will be different on export. I would recommend only doing so when doing a transformation operation where it is needed.

sgillies · 2019-10-21T04:55:33Z

@snowman2 I believe the code here is consistent with GDAL and OGR. See in https://gdal.org/development/rfc/rfc73_proj6_wkt2_srsbarn.html:

Vector layers mostly all call SetAxisMappingStrategy(OAMS_TRADITIONAL_GIS_ORDER) on the OGRSpatialReference* returned by GetSpatialRef().

jorisvandenbossche · 2019-10-22T08:40:02Z

On the conda-forge builds that use GDAL 3, it is complaining about "undefined symbol: OSRFixup" (conda-forge/fiona-feedstock#143), but that symbol should only be used when built against GDAL 1/2 ?

rbuffat · 2019-10-22T10:30:24Z

@jorisvandenbossche it is important that setup.py needs to know about the correct gdal version. I don't fully understand your build system. Maybe the problem is that you pin gdal to 2.4 in https://github.com/conda-forge/fiona-feedstock/blob/8a8798d39c129ebfc1817808ef7298aaf0f4e0ef/recipe/bld.bat#L2 ?

jorisvandenbossche · 2019-10-22T10:39:56Z

That's for windows, which does not yet have GDAL 3 builds I think (and the windows builds are also passing). For linux (https://github.com/conda-forge/fiona-feedstock/blob/master/recipe/build.sh) it does not seem to be pinned like that.
Fiona does not detect the available GDAL version automatically (if it can find it) when running setup.py ?

rbuffat · 2019-10-22T12:35:34Z

@jorisvandenbossche Maybe I found the issue: setup.py chooses _shim2.c instead of _shim3.c for gdal3. I created a PR to fix this: #807

@sgillies As a side note, we log information in setup.py. Do you know how to display these logs?

sgillies · 2019-10-22T14:52:07Z

@rbuffat thank you, that PR does the trick. I've made this mistake before and it comes from not building the wheels from a source distribution, but from a repo checkout. I need to change that.

Fiona 1.8.9.post1 is being built now.

jorisvandenbossche · 2019-10-22T14:56:53Z

@rbuffat thanks a lot for fixing that!

sgillies · 2019-10-22T15:15:17Z

See https://stackoverflow.com/a/8710759 for a pointer on setup.py logging.

jorisvandenbossche · 2019-10-22T20:18:57Z

The builds for GDAL 3 are passing, but now the builds for GDAL 2.4 are failing. Importing the build package gives:

  File "fiona/ogrext.pyx", line 1, in init fiona.ogrext
ImportError: No module named _shim

jorisvandenbossche · 2019-10-22T20:20:00Z

A typo in the merged PR (minor -> major) is the reason for that, I assume, see https://github.com/Toblerity/Fiona/pull/807/files#r337729479

jorisvandenbossche · 2019-10-23T06:40:46Z

With fiona 1.8.9.post2, all builds are passing now for conda-forge: conda-forge/fiona-feedstock#145

Sean Gillies added 4 commits October 17, 2019 12:26

Add shim module and support for GDAL 3

08e1e01

Add GDAL 2.4 and 3.0 to build matrix

48d51e1

Don't upgrade deps

4b4c368

Don't use setup.py directly

a010a3e

rbuffat reviewed Oct 17, 2019

View reviewed changes

Sean Gillies added 10 commits October 17, 2019 15:25

Add PROJ to the build matrix

ac9c97f

Add proj build script

aee61f8

Update GDAL build script

3386560

Allow travis to wait for GDAL build

58454a0

Wait 30 on GDAL build

84e829a

Increase wait to 40 and make jobs to 4

4a6dfcd

Remove wait so we can see what the problem is

062c465

Unsilence make

0e30757

Remove proj apt package

bc037e2

set PROJ_LIB correctly

20ecc7b

sgillies self-assigned this Oct 18, 2019

sgillies added this to the 1.8.9 milestone Oct 18, 2019

sgillies commented Oct 18, 2019

View reviewed changes

sgillies mentioned this pull request Oct 18, 2019

'_shim.pyx' doesn't match any files using GDAL 3.x #798

Closed

hobu reviewed Oct 18, 2019

View reviewed changes

Sean Gillies added 2 commits October 18, 2019 11:46

Call OSRFixup for GDAL 1 and 2

c45aa28

Cast open options for GDALOpenEx

e34fbd8

Add missing OSRFixup declarations

7371b7d

sgillies mentioned this pull request Oct 18, 2019

fiona when to support GDAL 3.X ? #803

Closed

stonebig mentioned this pull request Oct 20, 2019

fiona cannot be used because depends GDAL 2.X winpython/winpython#786

Closed

sgillies merged commit e0d015b into maint-1.8 Oct 21, 2019

mwtoews mentioned this pull request Nov 6, 2019

Establish support for Python 3.6, 3.7 and 3.8 #770

Merged

sgillies deleted the gdal3 branch October 11, 2020 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shim module and support for GDAL 3 #804

Add shim module and support for GDAL 3 #804

sgillies commented Oct 17, 2019 •

edited

rbuffat commented Oct 17, 2019

rbuffat Oct 17, 2019

rbuffat Oct 17, 2019

sgillies Oct 18, 2019

coveralls commented Oct 17, 2019 •

edited

sgillies Oct 18, 2019 •

edited

hobu Oct 18, 2019

sgillies Oct 18, 2019

hobu Oct 18, 2019

hobu commented Oct 18, 2019

hobu commented Oct 18, 2019

hobu commented Oct 18, 2019

sgillies commented Oct 18, 2019

hobu commented Oct 18, 2019

snowman2 commented Oct 19, 2019

sgillies commented Oct 21, 2019

jorisvandenbossche commented Oct 22, 2019

rbuffat commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

rbuffat commented Oct 22, 2019 •

edited

sgillies commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

sgillies commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

jorisvandenbossche commented Oct 23, 2019

		@@ -0,0 +1,149 @@
		"""Shims on top of ogrext for GDAL versions >= 2.2"""

Add shim module and support for GDAL 3 #804

Add shim module and support for GDAL 3 #804

Conversation

sgillies commented Oct 17, 2019 • edited

rbuffat commented Oct 17, 2019

rbuffat Oct 17, 2019

Choose a reason for hiding this comment

rbuffat Oct 17, 2019

Choose a reason for hiding this comment

sgillies Oct 18, 2019

Choose a reason for hiding this comment

coveralls commented Oct 17, 2019 • edited

sgillies Oct 18, 2019 • edited

Choose a reason for hiding this comment

hobu Oct 18, 2019

Choose a reason for hiding this comment

sgillies Oct 18, 2019

Choose a reason for hiding this comment

hobu Oct 18, 2019

Choose a reason for hiding this comment

hobu commented Oct 18, 2019

hobu commented Oct 18, 2019

hobu commented Oct 18, 2019

sgillies commented Oct 18, 2019

hobu commented Oct 18, 2019

snowman2 commented Oct 19, 2019

sgillies commented Oct 21, 2019

jorisvandenbossche commented Oct 22, 2019

rbuffat commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

rbuffat commented Oct 22, 2019 • edited

sgillies commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

sgillies commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

jorisvandenbossche commented Oct 22, 2019

jorisvandenbossche commented Oct 23, 2019

sgillies commented Oct 17, 2019 •

edited

coveralls commented Oct 17, 2019 •

edited

sgillies Oct 18, 2019 •

edited

rbuffat commented Oct 22, 2019 •

edited