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

feat: inmemory and async cache expire configurable #2496

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

zzzming
Copy link
Collaborator

@zzzming zzzming commented Jul 2, 2024

Make in-memory and async cache expiry time configurable. This is needed for some of our deployment.

This implementation also aligns with Redis cache that allows redis cache expiry to be configurable.

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. enhancement New feature or request labels Jul 2, 2024
@zzzming zzzming changed the title feature: inmemory and async cache expire configurable feat: inmemory and async cache expire configurable Jul 2, 2024
Copy link
Contributor

@ogabrielluiz ogabrielluiz left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 2, 2024
@zzzming zzzming enabled auto-merge (squash) July 2, 2024 22:37
@@ -29,9 +29,9 @@ def create(self, settings_service: "SettingsService"):
logger.debug("Redis cache is connected")
return redis_cache
logger.warning("Redis cache is not connected, falling back to in-memory cache")
return AsyncInMemoryCache()
return AsyncInMemoryCache(expiration_time=settings_service.settings.redis_cache_expire)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return AsyncInMemoryCache(expiration_time=settings_service.settings.redis_cache_expire)
return AsyncInMemoryCache(expiration_time=settings_service.settings.cache_expire)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is Redis failure case so that inmemory cache should respect the Redis cache_expire.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you use AsyncInMemoryCache you're not using redis, therefore you should not use redis parameters.

The expire time is related to the implementation. Mixing the values can be dangerous.

In general I believe the current behaviour is wrong. If you configure to use redis but you messed up some connection config, langflow should stop instead of falling back. - but it's not related to this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Let's merge this and we'll deactivate the fallback.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove the fallback to AsyncCache behaviour since this can be confusing and make it difficult to debug if Redis cannot be connected at the initialization.

@dosubot dosubot bot removed the lgtm This PR has been approved by a maintainer label Jul 3, 2024
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jul 8, 2024
@zzzming zzzming merged commit f663655 into main Jul 8, 2024
27 checks passed
@zzzming zzzming deleted the feat/cache-expire-config branch July 8, 2024 07:19
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 8, 2024
ogabrielluiz pushed a commit to yaitec/langflow that referenced this pull request Jul 9, 2024
nicoloboschi pushed a commit to datastax/ragstack-ai-langflow that referenced this pull request Jul 10, 2024
inmemory and async cache expire configurable

(cherry picked from commit f663655)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants