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

Google Floods Part 1: Creating gauge points and tooltip graphs; COUNTRY=cambodia #1328

Merged
merged 47 commits into from
Jan 15, 2025

Conversation

lowriech
Copy link
Collaborator

@lowriech lowriech commented Aug 8, 2024

Description

This fulfills step 1 and 2 of 3 for #1317 (see plan).

In this PR, we're fetching flood status by gauge from Google's Floods API for the entire country (10-100 gauges per country). We're displaying these gauges as points on Prism. They're color coded by flood status and provide past and future flooding in a tooltip when clicked.

Forecasts for each gauge were added here: #1330 (merged into this branch for review)

How to test the feature:

  • Open the Mozambique or cambodia deployment
  • Enable the Google Flood layer
  • Click on a gauge point

Checklist - did you ...

Test your changes with

  • REACT_APP_COUNTRY=rbd yarn start
  • REACT_APP_COUNTRY=cambodia yarn start
  • REACT_APP_COUNTRY=mozambique yarn start
  • Add / update necessary tests?
  • Add / update outdated documentation?

Screenshot/video of feature:

Screenshot 2024-08-12 at 2 03 31 PM 382087325-f448a602-a406-4f64-b6c5-861a8bc25c49

@lowriech lowriech marked this pull request as draft August 8, 2024 22:11
Copy link

github-actions bot commented Aug 8, 2024

Build succeeded and deployed at https://prism-1328.surge.sh
(hash aac80e7 deployed at 2025-01-08T22:11:02)

api/app/main.py Outdated
@@ -412,3 +413,10 @@ def post_raster_geotiff(raster_geotiff: RasterGeotiffModel):
return JSONResponse(
content={"download_url": presigned_download_url}, status_code=200
)


@app.get("/google-floods-gauges")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're gonna add a bunch of integrations, it will be worthwile to think about a standardized approach and common architecture. And maybe expose them through a sub endpoint of the API to keep things cleaner. wdyt @lowriech ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's a good question, @ericboucher. Do you see other APIs we're already leveraging fitting into this integration category (kobo, hdc stats, others)? Or are you thinking just for new integrations?

I'm wary of premature abstraction and trying to determine if I think now is the right time to align on this standardized approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems like low-hanging fruit to abstract once we decide on a pattern. So yes, agree we should think about it, but not sure about the velocity of adding entirely new providers. Would vote to wait until we see more pattern around this.

@gislawill gislawill changed the title Adding Google Floods Backend Adding Google Floods Backend + Creating a Gauge Points Aug 12, 2024
@gislawill
Copy link
Collaborator

@lowriech and @ericboucher, there are a couple outstanding issues (noted in the PR description) but this ready for initial review

@gislawill gislawill changed the title Adding Google Floods Backend + Creating a Gauge Points Adding Google Floods Backend + Creating a Gauge Points; COUNTRY=mozambique Aug 12, 2024
Comment on lines +172 to +177
if (
itemProps.visibility === FeatureInfoVisibility.IfDefined &&
!properties[item]
) {
return obj;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have some feature properties that may or may not be defined by the google api (for example: site name, river name). This allows us to hide the property from the pop up if it's not defined

Comment on lines 32 to 34
"river": (
data["river"] if "river" in data and len(data["river"]) > 1 else None
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

"river" can come in as a string with just 1 space (" "). This ensures there's a real value

@gislawill
Copy link
Collaborator

Note: I'm adding pytest-recording to store our google flood responses in local tests and use them in CI/CD. That will fix the failing API test in this PR

@gislawill gislawill linked an issue Aug 19, 2024 that may be closed by this pull request
@gislawill gislawill marked this pull request as ready for review August 21, 2024 01:21
@gislawill
Copy link
Collaborator

Thanks @wadhwamatic, is this good to go as far as you're concerned?

Some thoughts on potential remaining steps:

  • How does the copy look?
  • Is the layer set up optimal in each deployment (should this be in it's own layer group as currently implemented or put in an existing layer group)
  • We'll need translations once the copy and layer organization is complete

I'll note for context that the 502 errors from Google in RBD is not an issue on this branch. It seems to only crop up in #1332 (where I've added a cache-based workaround)

@wadhwamatic
Copy link
Member

@ericboucher - can you make a final pass on this PR? It's gtg from my side. Let's please remove the google flood layers from prism.json in the demo countries before merging into main though but bring it back subsequently in #1332

],
},
"properties": {
"gaugeId": data["gaugeId"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

How confident are we that all the keys will be present? Should ze use a getter with a default None to be safe?
Or make sure that we have proper error handling for missing keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We're pretty confident that they'll be present but there's no downside to the default none to be safe. I've updated to add a getter for non-essential values and error handling for missing essential values

@@ -38,3 +40,29 @@ def filter(self, record):
self.warning_count += 1

return False


def make_request_with_retries(
Copy link
Collaborator

@ericboucher ericboucher Dec 19, 2024

Choose a reason for hiding this comment

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

Do we need something specific, or could we rely on the Retry util directly?
from requests.adapters import Retry

Copy link
Collaborator

Choose a reason for hiding this comment

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

The retry utility has some useful functionality that this approach didn't have (back-off timing and status filtering for retries specifically). I've updated this function to wrap the retries utility. This make_request_with_retries function is still useful to reduce redundancy

@@ -6531,5 +6531,77 @@
}
],
"legend_text": "Joint Food Security & Nutrition Hotspot Analysis conducted by WFP, UNICEF, and partners based on SMART surveys and Cadre Harmonisé (CH) & Integrated Food Security Phase Classification (IPC) analyses"
},
"google_flood_status_at_gauges": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this layer be shared ans then just override the necessary param?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't implemented a shared layer before and I'm still trying to understand how it works. I see in the README:

Since many layers are common across multiple countries, we created shared configuration files that any deployment can access. These layers are generated by WFP globally and made available through the Humanitarian Data Cube.

Is the HDC part meaningful? Based on a quick scan, the current shared layers do appear to all be coming from HDC but I don't see why they need to (technologically speaking that is).

Also, @ericboucher, could you point me to examples of shared layers with overrides? I see examples of shared layers with duplicates in countries but I'm not seeing overrides.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ericboucher, adding this layer to our shared layer config caused it to load available dates for every deployment — regardless of whether or not the deployment used this layer. I think it's better to only add this layer to deployments that are using it until we're more selective about which layers we're loading data for

@@ -8,7 +8,7 @@ interface FetchWithTimeoutOptions extends RequestInit {

export const ANALYSIS_REQUEST_TIMEOUT = 60000;

const DEFAULT_REQUEST_TIMEOUT = 15000;
const DEFAULT_REQUEST_TIMEOUT = 30000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still needed? Is it to account for the retries?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's still needed. Not necessarily for retries but for loading all of the data needed for RBD. The server needs to make a lot of separate requests to get all of the various country data and, even with parallelization, it can take about 20 seconds.

Copy link
Member

Choose a reason for hiding this comment

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

@gislawill - I'm still seeing "Impossible to get point data dates for forecast_river_discharge_at_gauges" error on the surge deployment and locally. Any thoughts on this?

Copy link
Collaborator

@gislawill gislawill Jan 6, 2025

Choose a reason for hiding this comment

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

@wadhwamatic there are two issues right now:

  • The current deployment of our production PRISM server no longer has the functionality from this branch
    • This is resulting in 404 errors because the route to get the flood dates isn't found on the server
    • @ericboucher, any objection to me running a deployment from this branch to production? It will have the latest on main merged in
  • The google floods API seems to be down again. I'm seeing 502 "Bad gateway" errors when hitting these endpoints locally. I'll reach out to Aviel about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wadhwamatic redeploying the server fixed it
Screenshot 2025-01-06 at 9 08 46 PM

@@ -208,8 +223,21 @@ const Chart = memo(
fill: false,
}));
}
if (data.GoogleFloodConfig) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR is introducing a lot of extra complexity to this file. I'll open an issue recommending to split it by functions/services

Copy link
Collaborator

@ericboucher ericboucher left a comment

Choose a reason for hiding this comment

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

Nice work overall. Let's make sure to QA all different tooltips inc. EWS.
I left a few comments in line but this looks gtm overall !

@gislawill
Copy link
Collaborator

Nice work overall. Let's make sure to QA all different tooltips inc. EWS. I left a few comments in line but this looks gtm overall !

Confirmed EWS tooltips look good
Screenshot 2025-01-05 at 6 29 50 PM

@ericboucher
Copy link
Collaborator

@gislawill are we good to merge?

@gislawill
Copy link
Collaborator

I believe we are good to go. I'd love to have @wadhwamatic's approval before merging.

@wadhwamatic
Copy link
Member

sorry @gislawill, missed the fix for the png download. yes, gtg now. thanks!

@wadhwamatic wadhwamatic merged commit e85b377 into master Jan 15, 2025
6 checks passed
@wadhwamatic wadhwamatic deleted the feature/google-floods/1317 branch January 15, 2025 21:55
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.

[Bug]: Chart tooltip window resizes in error [Feature Request]: Google Flood Hub integration
4 participants