Feat: Create InMemoryTarget from TaskOnKart#441
Feat: Create InMemoryTarget from TaskOnKart#441utotsubasa wants to merge 19 commits intom3dev:masterfrom
InMemoryTarget from TaskOnKart#441Conversation
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`
chore: add type hints style: remove `Protocol`
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
utotsubasa
force-pushed
the
feat/in_memory_parameter_on_task
branch
from
fa0cbd3 to
76cb255
Compare
style: add a type hint
utotsubasa
changed the title
InMemoryTarget from TargetOnKart
utotsubasa
changed the title
InMemoryTarget from TargetOnKartInMemoryTarget from TaskOnKart
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the InMemoryTarget functionality to TaskOnKart by adding a new parameter (cache_in_memory_by_default) that allows tasks to store outputs in memory rather than on file by default. It also updates tests to confirm correct target creation and behavior and refactors target creation functions for better clarity.
- Added cache_in_memory_by_default parameter and associated methods in TaskOnKart.
- Updated test files to verify that cache targets and file targets are created as expected.
- Refactored make_in_memory_target in the InMemoryTarget module to support an optional unique id.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/in_memory/test_task_cached_in_memory.py | Added tests to verify in-memory caching behavior and target paths. |
| test/in_memory/test_in_memory_target.py | Updated fixture to use data_key to create in-memory targets. |
| gokart/task.py | Introduced cache_in_memory_by_default parameter and logic for default targets. |
| gokart/in_memory/target.py | Refactored make_in_memory_target; added helper _make_data_key. |
Comments suppressed due to low confidence (1)
gokart/in_memory/target.py:43
- Typo in the TODO comment: 'migit' should be corrected to 'might'.
# TODO: this module name `_path` migit not be appropriate
|
|
||
| def output(self) -> FlattenableItems[TargetOnKart]: | ||
| return self.make_target() | ||
| return self.make_default_target() |
There was a problem hiding this comment.
I prefer to change here instead of overwriting a method.
| return self.make_default_target() | |
| return self.make_target if not self.cache_in_memory_by_default()else self.make_cache_target() |
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
In task.py, make_cache_target hardcodes redis_key='redis_key' (line ~248), which is clearly a placeholder — if locking is ever enabled on a TaskLockParams constructed this way, every task would share the same Redis key and collide. This should at minimum derive a meaningful key from the task identity (e.g. self.make_unique_id() or the task class name), consistent with how make_task_lock_params works elsewhere.
The assignment self.make_default_target = self.make_target if not self.cache_in_memory_by_default else self.make_cache_target in __init__ sets an instance attribute that shadows any subclass override of make_target or make_cache_target, since method resolution on the instance attribute bypasses the MRO. A cleaner pattern would be to keep output() as a normal method that dispatches based on self.cache_in_memory_by_default directly, which is more idiomatic and easier to override in subclasses.
In _make_data_key in target.py, using if not unique_id: treats an empty string the same as None — given the function signature accepts Optional[str], an explicit if unique_id is None: check would be more precise and defensive.
TBE