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

Add compatibility with shapely>=1.7 #1755

Merged

Conversation

Shylpx
Copy link
Contributor

@Shylpx Shylpx commented Dec 22, 2022

Context

shapely is a geopandas' dependency (>1.7), and with the version 2.0.0 released on December 12, some breaking changes were introduced. This means some parts of the code, like this one are not working anymore, since lgeos is no longer defined:

Screenshot from 2022-12-22 12-57-54

This makes some crucial parts of the core of CARTOFrames to fail, like the to_carto method.

Relevant PR changes

  • Adding shapely as requirement, with the following restrictions: >1.7,<=1.7.1 to avoid the breaking changes.
  • CI Changes:
    • Python 3.5 and 3.6 versions have reached end of life, so they are not available for ubuntu-latest versions. For that reason, we need to use ubuntu-20.04, and the last versions of actions/checkout@v2 and actions/setup-python@v4 for GitHub actions to be able to find them. Job
    • We need to disable fail-fast, otherwise we get a cancelled signal (Error: The operation was canceled.) every time we install Python dependencies. Job
  • Failing tests:
    • If we execute the test with the last version of markupsafe, the following error is raised for every test. So we need to downgrade it to version 2.0.1 (or 1.1.1 for Python 3.5). Job
     E   ImportError: cannot import name 'soft_unicode' from 'markupsafe' (/home/runner/work/cartoframes/cartoframes/.tox/py37/lib/python3.7/site-packages/markupsafe/__init__.py)
    
    • The first approach was to use <2.0 and that's enough to fix the reported issue. However, with that version, the following test was failing. To fix it, we would need to downgrade it to version 1.7.1. Job
    _______________ test_read_carto_basegeometry_as_null_geom_value ________________
      
      mocker = <pytest_mock.plugin.MockerFixture object at 0x7f483cf208d0>
      
          def test_read_carto_basegeometry_as_null_geom_value(mocker):
              # Given
              cm_mock = mocker.patch.object(ContextManager, 'copy_to')
              cm_mock.return_value = GeoDataFrame({
                  'cartodb_id': [1],
                  'the_geom': [
                      None
                  ]
              })
          
              # When
              gdf = read_carto('__source__', CREDENTIALS, null_geom_value=BaseGeometry())
          
              # Then
              expected = GeoDataFrame({
                  'cartodb_id': [1],
                  'the_geom': [
                      BaseGeometry()
                  ]
              }, geometry='the_geom')
          
              cm_mock.assert_called_once_with('__source__', None, None, 3)
      >       assert expected.equals(gdf)
      
      tests/unit/io/test_carto.py:97: 
      _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
      .tox/py37/lib/python3.7/site-packages/geopandas/base.py:3284: in equals
          return self._data.equals(other._data)
      .tox/py37/lib/python3.7/site-packages/pandas/core/internals/base.py:119: in equals
          return self._equal_values(other)
      .tox/py37/lib/python3.7/site-packages/pandas/core/internals/managers.py:1310: in _equal_values
          return blockwise_all(self, other, array_equals)
      .tox/py37/lib/python3.7/site-packages/pandas/core/internals/ops.py:140: in blockwise_all
          res = op(info.lvals, info.rvals)
      .tox/py37/lib/python3.7/site-packages/pandas/core/dtypes/missing.py:518: in array_equals
          return left.equals(right)
      .tox/py37/lib/python3.7/site-packages/pandas/core/arrays/base.py:8[82](https://github.com/CartoDB/cartoframes/actions/runs/3759544118/jobs/6389235514#step:5:83): in equals
          equal_values = self == other
      .tox/py37/lib/python3.7/site-packages/geopandas/array.py:1313: in __eq__
          return self._binop(other, operator.eq)
      .tox/py37/lib/python3.7/site-packages/geopandas/array.py:1307: in _binop
          res = [op(a, b) for (a, b) in zip(lvalues, rvalues)]
      .tox/py37/lib/python3.7/site-packages/geopandas/array.py:1307: in <listcomp>
          res = [op(a, b) for (a, b) in zip(lvalues, rvalues)]
      .tox/py37/lib/python3.7/site-packages/shapely/geometry/base.py:281: in __eq__
          tuple(self.coords) == tuple(other.coords)
      _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
      
      self = <shapely.geometry.base.BaseGeometry object at 0x7f4[83](https://github.com/CartoDB/cartoframes/actions/runs/3759544118/jobs/6389235514#step:5:84)cb9[90](https://github.com/CartoDB/cartoframes/actions/runs/3759544118/jobs/6389235514#step:5:91)50>
      
          def _get_coords(self):
              """Access to geometry's coordinates (CoordinateSequence)"""
      >       raise NotImplementedError("set_coords must be provided by derived classes")
      E       NotImplementedError: set_coords must be provided by derived classes
      
      .tox/py37/lib/python3.7/site-packages/shapely/geometry/base.py:338: NotImplementedError
    
    • With Python 3.8, the following test fails if numpy>1.22.4. This only affects Python 3.8 because Python 3.7 was dropped with NumPy 1.22.0. So I've also added the numpy<=1.22.4 requirement. Job
    ______________ ERROR collecting tests/unit/analysis/test_grid.py _______________
      tests/unit/analysis/test_grid.py:19: in <module>
          GDF_BOX = GeoDataFrame({'id': [1, 2], 'geom': [pol_1, pol_2]}, columns=['id', 'geom'], geometry='geom')
      .tox/py38/lib/python3.8/site-packages/geopandas/geodataframe.py:135: in __init__
          super().__init__(data, *args, **kwargs)
      .tox/py38/lib/python3.8/site-packages/pandas/core/frame.py:663: in __init__
          mgr = dict_to_mgr(data, index, columns, dtype=dtype, copy=copy, typ=manager)
      .tox/py38/lib/python3.8/site-packages/pandas/core/internals/construction.py:493: in dict_to_mgr
          return arrays_to_mgr(arrays, columns, index, dtype=dtype, typ=typ, consolidate=copy)
      .tox/py[38](https://github.com/CartoDB/cartoframes/actions/runs/3759652576/jobs/6389477230#step:5:39)/lib/python3.8/site-packages/pandas/core/internals/construction.py:123: in arrays_to_mgr
          arrays = _homogenize(arrays, index, dtype)
      .tox/py38/lib/python3.8/site-packages/pandas/core/internals/construction.py:617: in _homogenize
          val = sanitize_array(
      .tox/py38/lib/python3.8/site-packages/pandas/core/construction.py:6[42](https://github.com/CartoDB/cartoframes/actions/runs/3759652576/jobs/6389477230#step:5:43): in sanitize_array
          subarr = maybe_convert_platform(data)
      .tox/py38/lib/python3.8/site-packages/pandas/core/dtypes/cast.py:127: in maybe_convert_platform
          arr = construct_1d_object_array_from_listlike(values)
      .tox/py38/lib/python3.8/site-packages/pandas/core/dtypes/cast.py:1784: in construct_1d_object_array_from_listlike
          result[:] = values
      .tox/py38/lib/python3.8/site-packages/shapely/geometry/polygon.py:300: in __array_interface__
          raise NotImplementedError(
      E   NotImplementedError: A polygon does not itself provide the array interface. Its rings do.
    

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #279134: CARTOFrames not working with 'shapely' 2.0.0 version.

@Shylpx Shylpx changed the title Add 'shapely' requirement to avoid version 2.0.0 breaking changes Add 'shapely' requirement to avoid version breaking changes Dec 22, 2022
@Shylpx Shylpx marked this pull request as ready for review December 22, 2022 18:12
@Shylpx Shylpx requested a review from Jesus89 December 22, 2022 18:12
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@Shylpx Shylpx requested a review from Jesus89 January 13, 2023 10:16
@Jesus89 Jesus89 force-pushed the chore/sc-279134/cartoframes-not-working-with-shapely-2-0 branch from 5222eb6 to bb9c3b5 Compare February 6, 2023 12:03
@Jesus89
Copy link
Member

Jesus89 commented Feb 6, 2023

It seems this comes from geopandas/geopandas#2622 (more context here shapely/shapely#928)

I have added the <2.0 limit and it works OK. I also moved the markupsafe dependency to the setup.py and CI ✔️

However, I'll take a look at how hard it is to make it compatible with shapely >=2.0

@Jesus89
Copy link
Member

Jesus89 commented Feb 6, 2023

I have detected that the problem with Shapely 2.0 was related to lgeos functions for handling the srid.

I have made some changes to make this part compatible with shapely>=1.7. I would go for this option, since it is more aligned with the upstream.

@Jesus89 Jesus89 changed the title Add 'shapely' requirement to avoid version breaking changes Add compatibility with shapely>=1.7 Feb 6, 2023
@Shylpx Shylpx merged commit 1f56b75 into develop Feb 8, 2023
@Jesus89 Jesus89 deleted the chore/sc-279134/cartoframes-not-working-with-shapely-2-0 branch February 8, 2023 15:55
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