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.

Sign up for GitHub

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

Jump to bottom

Upgrade tested Python and Django versions #123

Merged
merged 2 commits into from
May 13, 2024
Merged

Upgrade tested Python and Django versions #123

merged 2 commits into from
May 13, 2024

Conversation

adamchainz
Copy link
Contributor

@adamchainz adamchainz commented Apr 4, 2024 •
edited
Loading

This PR:

  1. Drops Python 3.7, since it is EOL.
  2. Adds Python 3.11 and 3.12.
  3. Drops Django 3.2. The Ubuntu CI environment uses SpatiaLite 5.0, but that is only supported by Django 4.0+, since Ticket #32575. It’s easiest to drop Django 3.2, which ends its long term support this month anyway.
  4. Adds Django 4.2 and 5.0. No code changes required.
  5. Changes GitHub actions to run one task per Python version, running all Django versions underneath. That is faster and removes the need to duplicate test matrices between tox and GHA.
  6. Upgrades GitHub Actions actions repositories.
  7. Re-applies isort, which makes some changes from version 5, mostly due to working on all kinds of import blocks.
  8. Adds a workaround for a bug when running SQLite 3.36+ with SpatiaLite 5 - see comment in quicktest.py.
  9. Adjusts a test to pass with slightly altered values - I think caused by SpatiaLite 5, which changed its topology library, among other things that may change calculations.

@adamchainz adamchainz force-pushed the upgrade_versions branch 6 times, most recently from f301744 to 59e6027 Compare April 4, 2024 21:50
@adamchainz adamchainz marked this pull request as ready for review April 4, 2024 21:53
@adamchainz
Copy link
Contributor Author

Alright, ready for review.

@smithdc1 : If you have any time to check the workaround in quicktest.py, that would be appreciated. Your name appeared a lot whilst I was working on this :)

@smithdc1
Copy link
Contributor

smithdc1 commented Apr 5, 2024

Hi Adam 👋 -- yes happy to have a look at this in this morning.

Just one initial comment, it seems that the install_requires and python_requires in setup.py need a version bump too.

smithdc1
smithdc1 reviewed Apr 5, 2024
View reviewed changes
djgeojson/tests.py Outdated Show resolved Hide resolved
adamchainz
adamchainz commented Apr 5, 2024
View reviewed changes
djgeojson/__init__.py
Comment on lines -1 to -12
#: Module version, as defined in PEP-0396.
from pkg_resources import DistributionNotFound

pkg_resources = __import__('pkg_resources')
try:
distribution = pkg_resources.get_distribution('django-geojson')
__version__ = distribution.version
except (AttributeError, DistributionNotFound):
__version__ = 'unknown'
import warnings
warnings.warn('No distribution found.')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using pkg_resources causes a warning on the latest setuptools:

  File "/.../django-geojson/djgeojson/__init__.py", line 2, in <module>
    from pkg_resources import DistributionNotFound
  File "/.../python3.10/site-packages/pkg_resources/__init__.py", line 102, in <module>
    warnings.warn(
DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html

That warning crashes the test suite thanks to the -W error::DeprecationWarning option we’re using since 1761635.

I think removing the __version__ attribute is the most sensible approach. I’ve done this in all my packages. There’s now a standard API for retrieving a package version in the standard library, importlib.metadata.version().

@adamchainz @smithdc1
Upgrade tested Python and Django versions
8ec70cc 
co-authored-by: David Smith <smithdc@gmail.com>
@adamchainz
Copy link
Contributor Author

Just one initial comment, it seems that the install_requires and python_requires in setup.py need a version bump too.

Thanks, done.

smithdc1
smithdc1 approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@smithdc1 smithdc1 left a comment

Choose a reason for hiding this comment

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

Hi Adam

This looks good to me. It's interesting to see how this issue can be worked around!

It seems that SpatiaLite 5.1 fixed this issue so when that's more easily available the workaround can be removed. I'm guessing it's not worth the effort to try and install that in place of workaround in quicktest.py now that this is working nicely.

djgeojson/tests.py
@@ -680,9 +679,9 @@ def test_within_viewport(self):
response = self.view.render_to_response(context={})
geojson = json.loads(smart_str(response.content))
self.assertEqual(len(geojson['features']), 2)
self.assertAlmostEqual(geojson['features'][0]['geometry']['coordinates'][0], 6.843322039261242)
self.assertAlmostEqual(geojson['features'][0]['geometry']['coordinates'][0], 6.843321961076886)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think proj / gdal introduced some calculation changes at some point. In my testing with spatialite==5.0 I saw this.

AssertionError: 6.843322040120945 != 6.843321961076886 within 7 places (7.904405929792802e-08 difference)

quicktest.py
@@ -69,6 +69,22 @@ def run_tests(self):

django.setup()

# Workaround for incompatibility between SQLite 3.36+ and SpatiaLite 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Workaround for incompatibility between SQLite 3.36+ and SpatiaLite 5,
# Workaround for incompatibility between SQLite 3.36+ and SpatiaLite 5.0,

I've not tested many version combinations but it seems that SpatiaLite 5.0 is the issue. The tests pass for me with SpatiaLite 5.1 and SQLite 3.45.2 (appreciate it is also a much more modern version of SQLite)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for testing. Yeah, I guess to update SpatiaLite, we would need to update the Ubuntu version on GitHub Actions, when 24.04 becomes available.

Are there any clear release notes for SpatiaLite 5.1? I found the site hard to navigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't find any release notes either.

@Gagaro
Copy link
Member

Gagaro commented May 13, 2024

Thanks for the PR 👍 . I'll merge as soon as the CI finishes.

@Gagaro Gagaro merged commit 47a29cf into makinacorpus:master May 13, 2024
5 checks passed
@adamchainz adamchainz deleted the upgrade_versions branch July 10, 2024 14:39
@adamchainz adamchainz mentioned this pull request Nov 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smithdc1 smithdc1 smithdc1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamchainz @smithdc1 @Gagaro