[DISCO-2028] Cache weather reports in Redis #202

linabutler · 2023-02-16T21:13:26Z

This PR adds logic to cache WeatherReport responses from AccuWeather in Redis, expiring after 15 minutes by default.

merino/configs/development.toml

merino/providers/weather/backends/accuweather.py

merino/providers/weather/provider.py

ncloudioj · 2023-02-17T16:29:51Z

Looks good. \o/

A couple of follow-up questions:

This adds Redis as a hard dependency, we will have to run a Redis server even for local development. Shall we make it optional? For example, we can disable this cache layer via configuration, or use an in-memory cache as the alternative backend
@Trinaa poke poke 😁, would love to hear your thoughts about testing ergonomics. For simplicity, the cache logic is baked directly inside the weather provider, and we mock Redis calls for testing.

linabutler · 2023-02-17T18:29:31Z

Thanks for the review, @ncloudioj! 🚀

Shall we make it optional? For example, we can disable this cache layer via configuration, or use an in-memory cache as the alternative backend.

That sounds good to me! We can have a CacheBackend protocol with a Redis implementation and a fake implementation.

merino/providers/weather/provider.py

Trinaa

So sorry for my tardiness in my comments. Similarly I will be OOF the next couple days and so will trust you to take or leave anything I've said.

Thank you for this work, especially the thoughtful tests!

merino/providers/weather/provider.py

merino/providers/weather/backends/accuweather.py

merino/providers/weather/provider.py

Trinaa · 2023-02-21T18:05:52Z

tests/integration/api/v1/suggest/test_suggest_weather.py

@@ -90,6 +99,7 @@ def test_suggest_with_weather_report(client: TestClient, backend_mock: Any) -> N
            forecast=weather_report.forecast,
        )
    ]
+    backend_mock.cache_inputs_for_weather_report.return_value = None


This return_value assignment is consistent across tests, an option could be to assign it once in the fixture_backend_mock.

Alternatively, were you thinking we could have an integration test where we simulate the cache workflow?

I went ahead and assigned it in the fixture_backend_mock!

Hmm, we'd still need to mock out Redis in our integration tests, right? My gut feeling is that simulating the cache workflow wouldn't tell us more than the unit tests we already have, or the contract tests with a real Redis container that we can add later—WDYT?

In short -- I agree

An integration test wouldn't tell us more than the unit, it would serve as the redundancy to the unit test. However, this particular case will have a contract test as you pointed out, that I think we will produce in short order 🤞🏻, which can also serve that purpose.

tests/unit/providers/weather/test_provider.py

ncloudioj · 2023-02-22T14:57:31Z

@linabutler Doh, sorry, the PR I merged yesterday caused lots of conflicts with yours, some rebasing is needed :/

This looks good to me. Did you want to add the CacheBackend and a simple in-memory cache to this PR as well? If you wanted to do that for a followup, that's also fine, but we will need to wire the provisioned Redis server with Merino in the cloudops-infra repo.

linabutler · 2023-02-22T18:20:18Z

@ncloudioj Yep, I just pushed a sketch of a CacheAdapter protocol, with Redis and in-memory implementations. WDYT? I think I'm happy with the general shape of it; if you are, too, I'll go ahead and take care of the remaining nits, TODOs, and tests.

merino/cache/no.py

ncloudioj · 2023-02-22T18:39:01Z

Yep, I just pushed a sketch of a CacheAdapter protocol, with Redis and in-memory implementations. WDYT? I think I'm happy with the general shape of it; if you are, too, I'll go ahead and take care of the remaining nits, TODOs, and tests.

LGTM 👍

linabutler · 2023-02-23T08:34:06Z

@ncloudioj Ready for re-review! I'll fold the fixup! commits into the earlier commits before merging, but I kept them separate for now to make it easier to see the interdiff.

merino/cache/protocol.py

tests/unit/providers/weather/backends/test_accuweather.py

tests/unit/providers/weather/test_provider.py

ncloudioj

LGTM, just a few nits for your consideration.

Thanks!

This commit changes the weather provider to: * Serialize and store weather reports in a volatile Redis key. The new `providers.accuweather.suggestions_ttl_sec` config setting controls the expiration time of the report. * Check for cached reports before requesting them from the backend when fetching weather suggestions.

linabutler requested a review from a team February 16, 2023 21:14

linabutler force-pushed the disco-2028-cache-accuweather-resp-take-2 branch 3 times, most recently from cdfbe7c to b3a31a8 Compare February 16, 2023 23:48

tiftran reviewed Feb 17, 2023

View reviewed changes

merino/configs/development.toml Show resolved Hide resolved