Update packages and methods for 2024 #384
Conversation
… environment for 2024; newer versions of software required some changes of syntax, and to avoid changing to much some software versions were pegged to specific releases (eg Pandas, which changes some SQL syntax and introduces an unnecessary deprecation warning if pyarrow isn't installed; pegged to 2.1.* to avoid issues from >=2.2.*).
…ghtweight alpine), environment.yml (to address dependabot alerts), and docker-build.bat to prepare multiplatform build; haven't fully tested, but so far at least the build looks successful on arm64, which is closer than we've gotten before (see #172 and #375); multiarch build sometimes still fails on a http error (docker suggests retrying as fix, and seems to work)
…unsupported svg features; see https://py-pdf.github.io/fpdf2/SVG.html
… render (use set_x instead of cell() when indenting; align='RIGHT' instead of align='R' for multi_cell) and reset set_x(0) after non-title page header
carlhiggs
added
enhancement
docker/Dockerfile
Outdated
| @@ -18,7 +18,7 @@ | |||
| # >>> docker rmi $(docker images -q) --force | |||
| ############################################################################## | |||
|
|
|||
| FROM continuumio/miniconda3:23.3.1-0-alpine as build | |||
| FROM continuumio/miniconda3:23.10.0-1 as build | |||
There was a problem hiding this comment.
Do we need to pin the version number that precisely? Can we just use latest here?
There was a problem hiding this comment.
Hi @gboeing, that's a good question -- perhaps we can. From memory, the reason I did a specific version at the time was, that was the latest release that supported Arm64. Previously, I had used the 'alpine' image, which is smaller size, but doesn't reliably support arm64 (mostly only amd64). Perhaps that is why the alpine image is half the size though. If you believe the 'latest' miniconda3 reliably supports arm64, and using 'latest' is unlikely to introduce incompatibilities, I can't see why not (they just released an updated version 3 days ago, so already advanced since pull request lodged).
I'll look to updating this after I hear back comments from others later in the week, in case there are other things to shift.
docker/docker-build.bat
Outdated
| docker build -t %DOCKERUSER%/%PACKAGE% . | ||
| docker run --rm -it --shm-size=2g --net=host -v "%CD%":/home/ghsci globalhealthyliveablecities/global-indicators /bin/bash -c "pip list --format=freeze > ./requirements.txt" | ||
| docker run --rm -it --shm-size=2g --net=host -v "%CD%":/home/ghsci %DOCKERUSER%/%PACKAGE% /bin/bash -c "pip list --format=freeze > ./requirements.txt" |
There was a problem hiding this comment.
I think this is because this 'docker run' command is launching the container using a historical snippet of code, overriding the deafult size that was (or perhaps was?) 64mb, since that would not be sufficient to adequately/reliably analyse most cities. However --- good point that in that particular context, setting shm-size isn't necessary, since all that the code is doing there is launching the container to grab a copy of the specific package versions that were installed and output requirements.txt. So I can remove that there, as its not intended to perform any analyses.
shm_size is set for analysis purposes in the compose yml now --- still to 2gb, although, for users with sufficient memory analysing larger cities, they may want/need to increase that:
https://github.com/global-healthy-liveable-cities/global-indicators/blob/931579fc4d375ac8dad25a532256ac84741f6745/docker-compose.yml#L6
Re: the batch file for building the docker image, I'll make a note to edit this later in the week. If you think the setting is no longer required in the compose file, feel free to let me know.
| - fpdf2=2.7.* | ||
| - sqlalchemy=1.4.* | ||
| - nicegui>=1.2.24 | ||
| - cryptography>=41.0.2 | ||
| - nicegui=1.4.13 |
There was a problem hiding this comment.
A lot of these deps are pinned down to the patch. Can we pin to the minor version instead? Patches should always be compatible across a minor version, and it would allow installers to bring in any important fixes.
There was a problem hiding this comment.
Nicegui is under rapid development (which is good) and they seem to change syntax regularly (can be a bit frustrating). I guess I was being explicit about which version I had designed our code to work with. For other packages, I was following dependabot recommendations.
But having said that, if you think that would be better practice, lets give it a go. As above, I'll aim to build another version of the image later in the week in case there is further feedback.
There was a problem hiding this comment.
@gboeing I just had a quick go at rebuilding the image removing the minor version pins; as expected, the web app didn't function with this modification (it doesn't load, due to a bug introduced in a recent minor patch zauberzeug/nicegui#2641 ). Its possible other incompatibilities / syntax changes could have been introduced too by one or other packages (even if they shouldn't). But the current selection of versions works for the Docker image according to my tests.
Given the current 4.4.9 build works, and we'll do a further environment update in a few months with OSMnx 2.0, I'm thinking the following:
- merge this branch into main now, and do a new release
- any reviewed and tested changes from @dapugacheva and @rychennn based off this image can then also be merged into main
- I'll then merge main changes into enhancements (or a branch off that, while I work through them to ensure it remains functional).
- We arrange for translations of report prose once this is finalised (I believe it should be soon)
- I'll update reporting templates, so those who want to use new templates in English can use Enhancements branch
- Once we have translations we can merge enhancements into main finally,
- then we can do a 4.5.0 release
then we'll look for a more thorough update of packages/environment and refactor code and do a 5.0 release then, perhaps?
For now, are you okay with the merging of this branch as is?
There was a problem hiding this comment.
Yes sounds like a reasonable plan.
| @@ -3,16 +3,21 @@ channels: | |||
| - conda-forge | |||
| dependencies: | |||
| - python | |||
| - osmnx=1.5.* # for constructing network graph from OpenStreetMap using Overpass API | |||
| - osmnx=1.5.* | |||
There was a problem hiding this comment.
1.9.* is the latest minor release as of now, if it's worth upgrading this dep
There was a problem hiding this comment.
We could give it a go --- it may have been I pinned to 1.5.* to avoid refactoring code due to other changes in dependencies (I tried to make software as new as possible, while re-writing code as least as possible). As above, I'll try the update and see how that goes with other dependencies/refactoring later in the week.
There was a problem hiding this comment.
Reflecting on this, your time is best served holding off on this one for now. We're working on some deprecations in advance of OSMnx v2 later this year. If it will require some refactoring in our codebase here to upgrade anyway, then we should just hold off until those deprecations for the OSMnx v2 API are all in place in a couple of months. That is, if you have to do some refactoring anyway, then might as well wait to just do it all at once rather than piecemeal.
There was a problem hiding this comment.
Sounds good @gboeing; thanks for heads up. I am sure there will be more patches requiring updates here by that stage in any case, so will be a good opportunity to review things at that point and see if we can better leverage OSMnx functionality and related packages and drop some other dependencies perhaps.
…om docker call to list installed packages as per #384
This pull request updates dependencies to the most recent compatible versions in the Docker image, and associated syntax to ensure these work. This will take advantage of various upstream enhancements in the packages used.
Additionally, as per #172 and #375, a multi-platform Docker build has been implemented to support users of devices with arm64 architectures (e.g. Apple users with devices having M2 chips).
Development branches based on the main code base prior to this update should merge these changes in to ensure they are up to date.
@dapugacheva @rychennn @gboeing, I have added you as reviewers, as we discussed in #375 and by e-mail other feature updates being based on these edits. Perhaps this will be easiest if we integrate these changes with main, and then your pull requests could leverage the updated main. Subseqently, we can merge main into Enhancements (mostly related to updated template implementation, which is on hold while others work on design aspects), once we are ready to do that.
I've tested this release with the example region within Python interactively, and using the web app and Jupyter lab interfaces for running analysis/generating resources/making comparisons. I also tested out the example tutorial in Jupyter lab, and restarted Kernel and cleared output cells so this is ready for users to use.
I don't necessarily expect you all to review this, but welcome your input if you have time/interest. I ran the unittests locally and these passed, so expect these to also be successful when the actions run after I action the pull request (we'll see!).