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

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

Merged
merged 4 commits into from
Feb 24, 2023

Conversation

linabutler
Copy link
Member

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

@linabutler linabutler requested a review from a team February 16, 2023 21:14
@linabutler 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
@ncloudioj
Copy link
Contributor

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
Copy link
Member Author

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.

Copy link
Contributor

@Trinaa Trinaa left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
merino/providers/weather/provider.py Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@ncloudioj
Copy link
Contributor

@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 linabutler force-pushed the disco-2028-cache-accuweather-resp-take-2 branch from 3129b56 to 0d312ad Compare February 22, 2023 18:17
@linabutler
Copy link
Member Author

@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 Outdated Show resolved Hide resolved
@ncloudioj
Copy link
Contributor

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 linabutler force-pushed the disco-2028-cache-accuweather-resp-take-2 branch from 0d312ad to 88a0b1d Compare February 23, 2023 08:08
@linabutler
Copy link
Member Author

@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 Outdated Show resolved Hide resolved
Copy link
Contributor

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits for your consideration.

Thanks! :shipit:

@linabutler linabutler force-pushed the disco-2028-cache-accuweather-resp-take-2 branch from 759b636 to 2d86ac0 Compare February 24, 2023 17:09
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 linabutler force-pushed the disco-2028-cache-accuweather-resp-take-2 branch from 2d86ac0 to d62ff4c Compare February 24, 2023 17:31
@linabutler linabutler merged commit 9b00131 into main Feb 24, 2023
@linabutler linabutler deleted the disco-2028-cache-accuweather-resp-take-2 branch February 24, 2023 19:53
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.

5 participants