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

fix: Cache the Config #933

Merged
merged 10 commits into from
May 21, 2024
Merged

fix: Cache the Config #933

merged 10 commits into from
May 21, 2024

Conversation

ross-p-smith
Copy link
Collaborator

@ross-p-smith ross-p-smith commented May 16, 2024

Purpose

Fixes #647

We use the higher-order functions to decorate the get_active_config_or_default method with a cache. We also invalidate the cache when we save the config.

However, because there are 2 applications, we display some text to tell the user that they will need to restart the Chat App to invalidate the cache

Copy link

github-actions bot commented May 16, 2024

Coverage

Coverage Report •
FileStmtsMissCoverMissing
code/backend/batch/utilities/helpers/config
   config_helper.py1380100% 
code/backend/pages
   04_Configuration.py1351350%1–9, 11–12, 14, 21, 28, 30, 35–44, 47–48, 51–62, 64–65, 67–69, 73–74, 86–90, 93–94, 98–100, 103–104, 107–108, 111–112, 135, 137–138, 140–144, 146–149, 152–156, 163–164, 174–176, 178, 198–199, 201, 203, 209, 217, 225, 232–233, 240, 242–243, 247, 255, 261, 268, 286–290, 296–297, 316–317, 321, 323–324, 347, 381–382, 386–387, 390–391, 394–397, 399–400, 402–404, 406–409, 411–412
TOTAL243167272% 

Tests Skipped Failures Errors Time
206 0 💤 0 ❌ 0 🔥 12.338s ⏱️

@ross-p-smith ross-p-smith marked this pull request as draft May 16, 2024 22:22
@ross-p-smith ross-p-smith marked this pull request as ready for review May 17, 2024 13:41
adamdougal
adamdougal previously approved these changes May 17, 2024
Copy link
Collaborator

@adamdougal adamdougal left a comment

Choose a reason for hiding this comment

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

Looks great! Just wondering, how much effort do you think it would be to add an endpoint to the chat app to reset/clear the config that we call after saving? The caveat being that if their are multiple instances of the chat app it won't work. But not sure if that's something we allow to be configured. WDYT?

lgtm

@ross-p-smith
Copy link
Collaborator Author

how much effort do you think it would be to add an endpoint to the chat app

I was actually thinking about this. However, I think the problem would be the dependency graph of the deployment. We would need the chat app deployed to be able to get the url (unless we did it by convention), but the backend is what is deployed last; and it would be the admin app calling the backend. But not saying no, let me give it a weekend thought!

@adamdougal
Copy link
Collaborator

how much effort do you think it would be to add an endpoint to the chat app

I was actually thinking about this. However, I think the problem would be the dependency graph of the deployment. We would need the chat app deployed to be able to get the url (unless we did it by convention), but the backend is what is deployed last; and it would be the admin app calling the backend. But not saying no, let me give it a weekend thought!

Ah good point! I guess another option could be to have a ttl on the cache, is that possible with func tools?

cecheta
cecheta previously approved these changes May 17, 2024
Copy link
Collaborator

@cecheta cecheta left a comment

Choose a reason for hiding this comment

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

Nice

@ross-p-smith ross-p-smith enabled auto-merge May 17, 2024 23:17
@ross-p-smith ross-p-smith dismissed stale reviews from cecheta and adamdougal via 956121f May 20, 2024 19:00
Copy link
Collaborator

@adamdougal adamdougal left a comment

Choose a reason for hiding this comment

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

LGTM!

@ross-p-smith ross-p-smith added this pull request to the merge queue May 21, 2024
Merged via the queue into main with commit b4e7dcc May 21, 2024
9 checks passed
@ross-p-smith ross-p-smith deleted the ross/647 branch May 21, 2024 07:46
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.

Cache config in application
3 participants