Support grouping by keys in LatestValueCache
#424
Conversation
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for key-based grouping in the LatestValueCache, enabling the cache to store and retrieve the latest value for each key.
- Added integration tests to verify key-based caching and overall cache behavior.
- Modified the LatestValueCache to accept an optional key function and maintain separate caches per key.
- Updated RELEASE_NOTES.md to document the new feature.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_latest_value_cache_integration.py | Added integration tests for key-based caching functionality. |
| src/frequenz/channels/_latest_value_cache.py | Updated the cache implementation to support grouping by keys via a key function. |
| RELEASE_NOTES.md | Updated release notes to reflect the new key function feature. |
| receiver: Receiver[T_co], | ||
| *, | ||
| unique_id: str | None = None, | ||
| key: typing.Callable[[T_co], typing.Any] | None = None, |
There was a problem hiding this comment.
The implementation signature uses a generic return type for 'key' compared to the overloads which expect a HashableT. Consider updating the annotation to 'typing.Callable[[T_co], HashableT] | None' for consistency.
| key: typing.Callable[[T_co], typing.Any] | None = None, | |
| key: typing.Callable[[T_co], HashableT] | None = None, |
There was a problem hiding this comment.
No, it has to be a superset of the two overloads HashableT and None.
| @typing.overload | ||
| def __init__( | ||
| self: "LatestValueCache[T_co, None]", | ||
| receiver: Receiver[T_co], | ||
| *, | ||
| unique_id: str | None = None, | ||
| key: None = None, | ||
| ) -> None: | ||
| """Create a new cache that does not use keys. | ||
| Args: | ||
| receiver: The receiver to cache. | ||
| unique_id: A string to help uniquely identify this instance. If not | ||
| provided, a unique identifier will be generated from the object's | ||
| [`id()`][id]. It is used mostly for debugging purposes. | ||
| key: This parameter is ignored when set to `None`. | ||
| """ | ||
|
|
||
| @typing.overload | ||
| def __init__( | ||
| self: "LatestValueCache[T_co, HashableT]", | ||
| receiver: Receiver[T_co], | ||
| *, | ||
| unique_id: str | None = None, | ||
| key: typing.Callable[[T_co], HashableT], | ||
| ) -> None: |
There was a problem hiding this comment.
Damn, is this overload over self really necessary? Doesn't self just works magically if you don't annotate it at all? Have you tried with Self?
Also, you should from __future__ import annotations and remove the quotes from the types if you still need it.
There was a problem hiding this comment.
This is how to avoid having to explicitly specify HashableT and default to None when no key is specified.
I will remove the quotes.
There was a problem hiding this comment.
I wonder if using a (non-hashable) sentinel in the type could help with this too, if the issue is that None is Hashable.
`LatestValueCache` now takes an optional `key` function. When specified, it is used to get the key for each incoming message, and the latest value for each key is cached and can be retrieved separately. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
shsms
force-pushed
the
🔑💸
branch
2 times, most recently
from
3d6fd84 to
3ad6744
Compare
| receiver: Receiver[T_co], | ||
| *, | ||
| unique_id: str | None = None, | ||
| key: Sentinel = NO_KEY_FUNCTION, |
There was a problem hiding this comment.
Can you use Literal[NO_KEY_FUNCTION] here instead of Sentinel? Otherwise this will also accept key=NO_VALUE_RECEIVER for example. I guess it is still OK (the alternative is to create one type per sentinel), but if Literal works it could be a very nice solution.
There was a problem hiding this comment.
No, Literal is only available for fundamental types. I think it is fine, the sentinels are interchangeable and their meaning comes from context, not from their names. Also, they are not exposed, so users can't use them without accessing private symbols.
There was a problem hiding this comment.
And Enums, that could be an alternative, but it is probably not worth it, if the values are not exposed anyway, then it doesn't add much.
This makes the code easier to read and the documentation look better. Signed-off-by: Sahas Subramanian <[email protected]>
…s#424)" This reverts commit 3edcb49, reversing changes made to f7fb341. Signed-off-by: Sahas Subramanian <[email protected]>
2 participants
LatestValueCachenow takes an optionalkeyfunction. Whenspecified, it is used to get the key for each incoming message, and
the latest value for each key is cached and can be retrieved
separately.