Conversation
feat: last_modification_time feature in `InMemoryTarget` style: add some type hints fix: fix typo in `InMemoryCacheRepository` test: add some tests for `InMemoryTarget` and `InMemoryCacheRepository`
style: update variable name from `id` to `key`
feat: add the new parameter `cache_in_memory_by_default` to switch default Target style: update the variable name from `target_key` to `data_key` for code consistency test: add tests for `TaskOnKart`s with the `cache_in_memory` parameter
feat: add cacheable flag to `make_target` test: add test fo Cacheable Targets chore: move `InMemoryTarget` to `gokart.target` to avoid circular dependencies
test: add tests for `CacheableModelTarget`
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
In gokart/in_memory/data.py, create_data is decorated with @classmethod but uses self as the first parameter instead of cls — this is a bug that will work accidentally but is semantically incorrect and will confuse readers. In cacheable.py, load is annotated as returning bool (def load(self) -> bool) when it clearly returns the cached data object (Any), which will mislead type checkers and callers. The class-level _cache: dict[str, InMemoryData] = {} on InMemoryCacheRepository is shared across all instances, and since InMemoryCacheRepository is a singleton, clearing the cache between test runs requires explicit teardown — there should be a clear() method exposed on the repository, and tests should verify isolation. Finally, the InstantScheduler comment # TODO: ambiguous class name acknowledges a real problem: the name implies no-op scheduling, but it actually deletes entries on get_hook, behaving like a "use once" or "consume" cache — OneShotScheduler or ConsumeOnReadScheduler would be much clearer.
2 participants
No description provided.