hikari icon indicating copy to clipboard operation
hikari copied to clipboard

Easy way to declare custom Cache

Open circuitsacul opened this issue 3 years ago • 11 comments

Summary

It would be nice if I could declare a cache class in CacheSettings. Currently you have to patch multiple things, including Bot._cache and Bot._event_manager._cache.

Why is this needed?

To add to or modify the default cache.

Ideal implementation

An extra setting in CacheSettings for cache class; it should expect a subclass of CacheImpl. Then it can set self._cache = cache_settings.cache_class(...), or something of the sort.

Checklist

  • [X] I have searched the issue tracker and have made sure it's not a duplicate. If it is a follow up of another issue, I have specified it.

circuitsacul avatar Jan 21 '22 23:01 circuitsacul

The CacheImpl class itself takes a CacheSettings object in the constructor, so that would be impractical to declare a Cache class in the settings. The GatewayBot class has a property called _cache which is the cache implementation that the bot uses. https://github.com/hikari-py/hikari/blob/965d6e8a4c7685a7b6d9fd1b53cec3609f0821e3/hikari/impl/bot.py#L342-L344

You can probably subclass it and override it to use your own cache implementation. If you want to implement your own cache, consider implementing this abstract class https://github.com/hikari-py/hikari/blob/965d6e8a4c7685a7b6d9fd1b53cec3609f0821e3/hikari/api/cache.py#L75 From what I see, there is no way to add a Cache class to cache settings since the Cache object itself takes in the settings when it's being constructed. All I can tell you is to subclass GatewayBot, set the _cache property to your own cache implementation, then use it.

But I think it would be better for the GatewayBot class to take in a cache implementation as a constructor arg, instead of taking the cache impl in the settings. I will leave that decision to Dav and others since I don't maintain this project :)

ahnaf-zamil avatar Jan 24 '22 02:01 ahnaf-zamil

I see. The problem is you have to patch two places; Bot._cache and Bot._event_manager._cache. Maybe an extra option on Bot that accepts a cache class would be best.

circuitsacul avatar Jan 24 '22 02:01 circuitsacul

Yeah that's what I said in the last part of my reply. The GatewayBot could take in a cache impl as a constructor arg, and then use that. But at the moment, it's hardcoded to use hikari's default CacheImpl, and the only way to change it is to override _cache and _event_manager._cache.

ahnaf-zamil avatar Jan 24 '22 02:01 ahnaf-zamil

I just realized, you don't need to override _event_manager._cache since the event manager takes in GatewayBot._cache. The RESTClient does so as well. So all that needs to be done is to add a feature to set custom cache class, if I'm not being dumb rn

ahnaf-zamil avatar Jan 24 '22 03:01 ahnaf-zamil

Why is this needed? To add to or modify the default cache.

What's the actual use case for this though

FasterSpeeding avatar Jan 24 '22 03:01 FasterSpeeding

Why is this needed? To add to or modify the default cache.

What's the actual use case for this though

Maybe someone might want to use their own cache implementation. For example, I personally had thought of the idea to use Redis for cache. Even though not everyone will use their custom cache impl, having a pluggable cache seems like a nice feature

ahnaf-zamil avatar Jan 24 '22 03:01 ahnaf-zamil

Why is this needed? To add to or modify the default cache.

What's the actual use case for this though

Maybe someone might want to use their own cache implementation. For example, I personally had thought of the idea to use Redis for cache. Even though not everyone will use their custom cache impl, having a pluggable cache seems like a nice feature

This isn't a viable way to add an asynchronous cache, you'd have to have that as a separate thing

FasterSpeeding avatar Jan 24 '22 03:01 FasterSpeeding

Why is this needed? To add to or modify the default cache.

What's the actual use case for this though

Maybe someone might want to use their own cache implementation. For example, I personally had thought of the idea to use Redis for cache. Even though not everyone will use their custom cache impl, having a pluggable cache seems like a nice feature

This isn't a viable way to add an asynchronous cache, you'd have to have that as a separate thing

Separate thing hmmm... Like what exactly?

ahnaf-zamil avatar Jan 24 '22 03:01 ahnaf-zamil

Why is this needed? To add to or modify the default cache.

What's the actual use case for this though

Adding/modifying an existing cache. For example, the current cache only stores existing values. What if I wanted to store None for deleted messages, so that I don't fetch non-existing messages repeatedly?

An async cache would have to be seperate, but here's a custom cache I use right now: https://github.com/TrigonDev/Starboard/blob/main/starboard/cache.py

circuitsacul avatar Jan 24 '22 14:01 circuitsacul

For example, the current cache only stores existing values. What if I wanted to store None for deleted messages, so that I don't fetch non-existing messages repeatedly?

BTW this example would likely fall under #880 (although it'd never quite apply to messages since that cache store is always incomplete)

FasterSpeeding avatar Jan 24 '22 15:01 FasterSpeeding

This isn't a viable way to add an asynchronous cache, you'd have to have that as a separate thing

I think there is with a library like greenback. Obviously it's a performance drop but I would take that because it will allow me to use speedups from libraries that can skip API calls if they can interact with the cache. (Both crescent and flare have this.)

Lunarmagpie avatar Jan 02 '23 17:01 Lunarmagpie