Adds endpoints for caching ARDAC in Varnish cache - #86
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for caching ARDAC (Arctic-based community) endpoints in the Varnish cache by targeting the most frequently requested communities based on Umami analytics data. The implementation follows the approach from the ardac-quick-cache project to avoid recaching all Arctic communities globally.
- Adds ARDAC endpoint caching functionality to the existing recache system
- Integrates with Umami database to identify frequently accessed communities
- Implements coordinate lookup using the geospatial-vector-veracity repository
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| recache_api/recache_tasks.py | Adds core ARDAC caching functionality including database connection, community coordinate lookup, and URL formatting |
| recache_api/recache.py | Updates default parameters to include "ardac" in cached applications |
| recache_api/luts.py | Defines ARDAC API endpoints with f-string formatting for latitude/longitude substitution |
…ates from being gathered for every route.
brucecrevensten
left a comment
brucecrevensten
left a comment
There was a problem hiding this comment.
Strongly consider the git submodule, and do rework code to only use f-strings for the list of places to cache.
| psql_host = "umami.snap.uaf.edu" | ||
| psql_port = "5432" | ||
| psql_database = "umami" | ||
| psql_user = "umami" |
There was a problem hiding this comment.
Should user also be in secrets?
There was a problem hiding this comment.
Both the username and database name have been made secrets.
|
|
||
| def connect_to_umami_db(): | ||
| umami_website_ids = ( | ||
| "1f4a98e7-d5cb-4295-82fc-5a4d41328038", # EDS |
There was a problem hiding this comment.
Strictly, should these also be secrets?
There was a problem hiding this comment.
These Umami website IDs can be seen by anyone in plain text via browser developer tools, so not sensitive info:
|
|
||
| def get_community_coords(): | ||
| if not os.path.exists("./geospatial-vector-veracity"): | ||
| subprocess.run( |
There was a problem hiding this comment.
Should this instead be a git submodule? If so then the line above can change to throwing a fatal error saying "do git submodule update first" or something. Also if you agree this should be a submodule, update the README.
There was a problem hiding this comment.
If we set this up as a git submodule, that locks us to a version of the GVV and while we don't necessarily change the GVV all that often, I would think that we would want to have the newest version of the GVV cloned for each time we run the recache. If we make this into a submodule, we have to remember to update the submodule and commit that as a change each time the GVV changes if we want to see that change available.
| "/cmip6/point/{latitude}/{longitude}?vars=rsds,rlds,hfss,hfls,clt", | ||
| "/cmip6/point/{latitude}/{longitude}?vars=prsn,snw", | ||
| "/indicators/cmip6/point/", | ||
| "/indicators/base/point/", |
There was a problem hiding this comment.
Hmm, don't some of these already get cached as part of NCR/EDS? I suppose it's harmless to repeat it, and this does make it explicit which things the ARDAC uses... hmm. I think I'd prefer that these stay explicitly listed like this, actually!
There was a problem hiding this comment.
Yeah, I don't think there is any harm in running through duplicates again, just makes sure that things are actually living in cache since they will return instantly.
| communities = [] | ||
| for row in rows: | ||
| path = row[0] | ||
| match = re.search(r"[A-Z]{2}\d+", path) |
There was a problem hiding this comment.
For readability, this could use a comment. I can see what it's doing, tho. Optional.
There was a problem hiding this comment.
Added a set of comments for clarity.
| return sort_out_communities(rows) | ||
|
|
||
|
|
||
| def is_f_string(route): |
There was a problem hiding this comment.
Make them all f-strings. That way, instead of testing (here) and then doing string concatenation to build the URL for the other case, they are all consistent.
There was a problem hiding this comment.
I made all of the LUTs URLs into f-strings so that we don't need this check anymore, and changed the way we inject the latitude, longitude, and areaID for each.
|
|
||
| for place in places: | ||
| community_coord = community_coords.get(place) | ||
| if not community_coord: |
There was a problem hiding this comment.
What is this error state trying to say? Instead of continue, should it warn/exit?
There was a problem hiding this comment.
If the place listed is not in the community coordinates, it returns a None rather than latitude and longitude and thus we should continue to the next place. The logic is correct to prevent errors when passing the community_coord to the generate endpoints.
cstephen
left a comment
cstephen
left a comment
There was a problem hiding this comment.
I ran this new Prefect flow successfully and the log output looks good. Thanks for doing the extra work of porting this to Prefect 👍
|
|
||
| def connect_to_umami_db(): | ||
| umami_website_ids = ( | ||
| "1f4a98e7-d5cb-4295-82fc-5a4d41328038", # EDS |
There was a problem hiding this comment.
These Umami website IDs can be seen by anyone in plain text via browser developer tools, so not sensitive info:
This PR adds the ability to cache the most frequently requested communities according to our Umami statistics. This is to prevent having to recache every Arctic-based community around the globe when we recache the API in the future. After this is run, we would expect our most frequently requested communities would be cached and available instantly in our ARDAC features.
This addition is heavily adapted from the work done here: https://github.com/ua-snap/ardac-quick-cache