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

Upgrade analysis and Django containers to Django 3.2 #893

Merged
merged 5 commits into from
Jun 17, 2022

Conversation

ktohalloran
Copy link
Contributor

@ktohalloran ktohalloran commented Jun 9, 2022

Overview

This PR upgrades to Django 3.2 and also upgrades most dependencies within the Django and analysis containers.

Notes

Analysis Container Update

This PR is based on work by @jacobscarfmerrell and @KlaasH on #876; in the interest of citing my sources, I want to make it clear that due to a particularly grueling rebase process, a lot of that work is now represented by commits d8f59d0 and 9cc8be2.

Invalid Literal for int() Error

This PR addresses a failure while running analysis, where the the analysis process would fail right at the end while attempting to fetch the newly updated AnalysisJob object to add in the residential speed limit. Similarly, if you ran analysis on develop and then tried to view that analysis, either from the frontend or via AnalysisJob.objects.get(), you would see an invalid literal for int() of base 10 error. After quite a long time trying to follow the string back, it turns out that while the SQL command was generating the CSV with all original_score fields as having 4 decimal points, AnalysisJobManager was expecting a PositiveIntegerField while annotating each object with its population. To address this issue with new projects, this PR adds a line to clean_metric_dict() within load_overall_scores that first converts population_total to a float and then again into an integer. For past projects, this PR casts population to an integer during annotation.

As to where this error came from, I couldn’t exactly pin down which update to which package would have created this behavior. I initially thought it was the “Narrower exception catching in serializer fields, to ensure that any errors in broken get_queryset() methods are not masked” 3.12.2 update to Django Rest Framework; however, the error was ultimately not being thrown by the serializer. Similarly, Django has made upgrades to its annotations and PositiveIntegerField type, but none that would obviously result in this issue.

Errors Not Addressed in this PR

There were a few errors that I saw while working on this branch that did not appear to be a product of the upgrade and were therefore outside the scope of this PR, but may be worth investigating as separate issues:

On set up
Just prior to starting the Ansible playbook, two warnings appear:

/usr/local/lib/python3.8/dist-packages/requests/__init__.py:89: RequestsDependencyWarning: urllib3 (1.26.9)
or chardet (3.0.4) doesn't match a supported version! warnings.warn("urllib3 ({}) or chardet ({}) doesn't
match a supported "

This warning does not disappear even when the most recent version of these packages are specified in requirements.txt, which makes me think it’s either not coming from the Django or analysis containers or is being generated as a result of our outdated version of django-localflavor, which also lists these packages as dependencies.

[WARNING]: Skipping plugin (/usr/local/lib/python3.8/dist-packages/ansible/plugins/filter/core.py) as it
seems to be invalid: cannotimport name 'environmentfilter' from 'jinja2.filters'(/usr/local/lib/python3.8/
dist-packages/jinja2/filters.py)

This seems to be related to Ansible and is not fixed with an update to the Django or analysis containers.

While running Ansible playbook during ./scripts/setup, if you are running it over top of an existing VM (not creating it from scratch), an error appears during the 'Extract and install Terraform' task:

[DEPRECATION WARNING]: Using tests as filters is deprecated. Instead of using 
result|changed` use `result is changed`. This feature will be removed in version 2.9. 

Finally, there were a lot of deprecation warnings generated while building the angularjs container.

Running tests or server
Running tests or the server with the -Wall flag inside the Django container also results in a few errors I did not address:

A lot of django.utils.translation.ugettext_lazy() is deprecated in favor of django.utils.translation.gettext_lazy() warnings; these are a result of us keeping django-localflavor at version 2.2.

/usr/src/pfb_network_connectivity/settings.py:308: ResourceWarning: unclosed <ssl.SSLSocket fd=3,
 family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('172.18.0.3',
52626), raddr=('52.73.139.254', 443)>
revision = get_latest_job_definition(PFB_AWS_BATCH_ANALYSIS_JOB_DEFINITION_NAME)['revision']
ResourceWarning: Enable tracemalloc to get the object allocation traceback

This goes back to an error I’ve seen before within boto3, which this thread seems to describe as just noise. I did double check that all <file>.opens are either preceded by with or followed by <file>.close() and then left it as is.

/usr/local/lib/python3.10/site-packages/past/builtins/misc.py:45: DeprecationWarning: the imp module is
deprecated in favour of importlib and slated for removal in Python 3.12; see the module's documentation
for alternative uses
  from imp import reload

Seems to be an issue with the past module.

Testing Instructions

  • Run ./scripts/setup to apply necessary migrations
  • Inside the VM, run ./scripts/server and then navigate to http://localhost:9301/admin#/admin/analysis-jobs/
  • Verify that previously made projects are displaying as they should
  • Run a new analysis and verify it completes and displays on the analysis jobs page as expected
  • Back in the browser, verify that you can see the place you just analyzed on http://localhost:9301/#/places//// and that the high and low stress routes appear as expected
  • Verify that you can compare places
  • In the VM, run docker-compose run --rm --entrypoint bash django and then run python3 -Wall manage.py test --noinput
  • Verify that the only deprecation warnings you see are listed above and that the tests complete without errors
  • Still in the docker container, run python3 -Wall manage.py runserver and verify that:
    • the only deprecation warnings you see are listed above
    • the server is running Django 3.2.13
    • the server runs as expected

Checklist

  • Add entry to CHANGELOG.md

Resolves #876 & #877

KlaasH and others added 4 commits June 9, 2022 14:00
The syntax of these files changed. This doesn't change the substance of
the config at all, but updates it to work with the newer version.
This commit:
- Upgrades the analysis container to Postgres 13
- Updates the Dockerfile for the Django container to use Django
3.12.13 and python3.10
- Updates all dependencies in requirements.txt except for django-
localflavor and pyuca
- Replaces base_name with basename in pfb_network_connectivity/urls.py
- Removes the 'latest' filter from pfb_analysis/filters.py because
the upgrade to django-filter introduced a breaking change where
non-model fields that are declared via a method, e.g. filter_latest,
should not be listed in Meta.fields
- Updates the CHANGELOG
This was resulting in a 'invalid literal for int() of base 10' error
whenever attempting to use AnalysisJob.objects.get().
As a result, this commit:
- Adds a line to load_overall_scores to convert population to an
integer while loading the data from the CSV into the overall_scores
field to address the issue in future projects
- Casts population to an integer while annotating the AnalysisJob
objects to address the issue in existing projects
Copy link
Contributor

@lydiascarf lydiascarf left a comment

Choose a reason for hiding this comment

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

I only noticed one difference after the following step

In the VM, run docker-compose run --rm --entrypoint bash django and then run python3 -Wall manage.py test --noinput

/usr/src/pfb_analysis/models.py:321: FionaDeprecationWarning: Collection.__next__() is buggy and will be removed in Fiona 2.0. Switch to `next(iter(collection))`.
  feature = next(shp_handle)

Everything works and looks great, though! Nice job 🎉

Copy link
Contributor

@KlaasH KlaasH left a comment

Choose a reason for hiding this comment

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

👍 Looks good!

I ran an analysis and imported one, and poked around the admin and public sites. Seems like it's all working well.

@ktohalloran ktohalloran merged commit a9a4bc9 into develop Jun 17, 2022
@ktohalloran ktohalloran deleted the feature/kob/upgrade-django-3.2 branch June 17, 2022 19:32
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.

Update analysis base container and PostgreSQL version
3 participants