-
Notifications
You must be signed in to change notification settings - Fork 34
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
Google Floods Part 3: Adding Inundation Maps; COUNTRY=mozambique #1332
base: feature/google-floods/1317
Are you sure you want to change the base?
Google Floods Part 3: Adding Inundation Maps; COUNTRY=mozambique #1332
Conversation
@gislawill this is updated to call |
…forecast-chart/1317
d358674
to
bb77ff7
Compare
Build succeeded and deployed at https://prism-1332.surge.sh |
…/google-floods-inundation
…forecast-chart/1317
…/google-floods-inundation
…/WFP-VAM/prism-app into feature/google-floods-inundation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two files are each about 4 MB — which is unfortunately how large the response from Google is at this endpoint.
Note: on 11/7/24, I received this error message intermittently but very frequently. Hoping the google floods has resolved it. Or will need to reach out to them Request failed at url post https://floodforecasting.googleapis.com/v1/floodStatus:searchLatestFloodStatusByArea?key={GOOGLE_FLOODS_API_KEY}: {'regionCode': 'ML'}
|
…/WFP-VAM/prism-app into feature/google-floods-inundation
looks good from my side Will. some surprising discrepancies in Cambodia between NASA SERVIR flood extent and the Google model, but we've also known there to be overestimation by the SERVIR tool. |
The 502 errors from Google Floods have continued and seem to be a result of us hitting the searchLatestFloodStatusByArea endpoint too frequently. I've just emailed the google floods team about this issue. In the meantime, I've added google floods response caching which gets around the issue for now. I've set the cache timeout to only 10 minutes as we know this data can change quickly. @ericboucher, I'm curious for your thoughts on this caching (a45de24). Specifically,
|
…/WFP-VAM/prism-app into feature/google-floods-inundation
api/app/caching.py
Outdated
@@ -98,13 +98,13 @@ def cache_file(url: str, prefix: str, extension: str = "cache") -> FilePath: | |||
|
|||
|
|||
@timed | |||
def cache_geojson(geojson: GeoJSON, prefix: str) -> FilePath: | |||
def cache_geojson(geojson: GeoJSON, prefix: str, cache_key: str) -> FilePath: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explicit the inputs here, esp. the cache_key and how it should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some add comments and updated the params to be more explicit
@gislawill thanks for this. Yes I think it could be useful to add the cache timeout mechanism for other requests as well. And for the cache clearing, I think it would be worth implementing but not sure if there is an easy way. We could do something simple like delete any file that's more than 30 days old or something like this. @wadhwamatic any thoughts on that point? |
@ericboucher - yes, we should delete older files in this case and 30 days sounds like a good initial period to try |
@wadhwamatic and @ericboucher, we have many different options for clearing this cache. I think whichever option we go with, it should be easy to trigger the cache clearing automatically and manually. We'll also want to be able to easily adjust the "minimum age to clear" to allow for experimentation with this cache age and for clearing caches after a bug fix rollout. The two good options would be a shell script and an endpoint:
Two options for automated triggers: I'm leaning towards a script that's triggered by an ECS Scheduled Task. Curious for your thoughts. Also curious if either of you would have a preference between adding this cache clearing in this PR vs a follow-up? |
@gislawill the endpoint option seems to be convenient and allows any of us to trigger it manually when necessary. We could "secure" it with a token or something simple, that should be enough to test things out. I'd prefer to have this added in a follow-up PR as it would make it a bit easier to discuss implementation and triggers. If we go with the endpoint option, we could use a simple GitHub action to run daily and call this endpoint. |
Sounds good to me @ericboucher. @wadhwamatic I've created issue #1371 for this work. This PR should be ready for review. It's blocked on rollout by the copy + translations questions here: #1328 (comment) |
This adds inundation polygons from the Google Floods API.
https://www.loom.com/share/5b430b093ee546139f5ccbe3299c60c9?sid=049273de-18ff-4b6a-8718-b1a164c82b52