You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@hamilton.apache.org by "Shoryamishra61 (via GitHub)" <gi...@apache.org> on 2026/09/19 20:22:07 UTC
[PR] fix(caching): avoid storing metadata and results for CachingBehavior.IGNORE nodes [hamilton]
Shoryamishra61 opened a new pull request, #1716:
URL: https://github.com/apache/hamilton/pull/1716
<!-- SPDX-License-Identifier: Apache-2.0 -->
Fixes #1704
## Description
`CachingBehavior.IGNORE` is documented as "Results are never stored nor versioned" (`adapter.py`), but was previously included in the metadata storage branch in `do_node_execute` and the result storage branch in `post_node_execute`. Because `pre_node_execute` returns early for `IGNORE` nodes without creating a `cache_key`, storing metadata resulted in entries with `cache_key = NULL`. In SQLite, `INSERT OR IGNORE` does not treat `NULL` as duplicate, leading to orphaned metadata entries accumulating on every run for ignored nodes.
## Changes
- In `hamilton/caching/adapter.py`:
- `do_node_execute`: removed `CachingBehavior.IGNORE` from the metadata persistence block, restricting it to `CachingBehavior.RECOMPUTE`.
- `post_node_execute`: removed `CachingBehavior.IGNORE` from the result store persistence block, keeping only `DEFAULT` and `RECOMPUTE`.
- Kept node execution of `IGNORE` nodes and the `"<ignore>"` dependency version marker used by downstream nodes intact.
- In `tests/caching/test_integration.py`:
- Updated `test_ignore_behavior` assertions to confirm that repeated executions with an ignored node do not increase metadata store size beyond the new `<ignore>` key created for downstream nodes.
- Added `test_ignore_behavior_from_start` verifying that when a node is ignored from run 1, it writes 0 metadata entries and 0 result store entries, while downstream nodes can still cache under the `<ignore>` dependency key.
## How I tested this
- Ran focused regression tests:
`pytest tests/caching/test_integration.py -k "test_ignore_behavior" -vv`
- Ran full caching integration test suite:
`pytest tests/caching/test_integration.py -vv`
- Ran full caching test suite:
`pytest tests/caching/ -vv`
- Ran pre-commit hooks (`ruff check`, `ruff format`, license headers, etc.):
`pre-commit run --files hamilton/caching/adapter.py tests/caching/test_integration.py`
## Checklist
- [x] PR has an informative and human-readable title (this will be pulled into the release notes)
- [x] Changes are limited to a single goal (no scope creep)
- [x] Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
- [x] Any _change_ in functionality is tested
- [x] New functions are documented (with a description, list of inputs, and expected output)
- [x] Placeholder code is flagged / future TODOs are captured in comments
- [x] Project documentation has been updated if adding/changing functionality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(caching): avoid storing metadata and results for CachingBehavior.IGNORE nodes [hamilton]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1716:
URL: https://github.com/apache/hamilton/pull/1716#discussion_r4081889075
##########
hamilton/caching/adapter.py:
##########
@@ -1278,10 +1278,7 @@ def do_node_execute(
node_kwargs=node_kwargs,
task_id=task_id,
)
- if self.behaviors[run_id][node_name] in (
- CachingBehavior.RECOMPUTE,
- CachingBehavior.IGNORE,
- ):
+ if self.behaviors[run_id][node_name] == CachingBehavior.RECOMPUTE:
cache_key = self.get_cache_key(run_id=run_id, node_name=node_name, task_id=task_id)
Review Comment:
This change makes the metadata/result-normalization logic in this block run only for `RECOMPUTE`. In `do_node_execute`, DataLoader nodes can return `(result, metadata)` tuples; previously, `IGNORE` also flowed through the block that strips metadata (e.g., `result = result[0]`) before returning. After this change, an `IGNORE` DataLoader may now return the raw tuple to downstream nodes, potentially breaking type expectations and changing runtime behavior. Consider keeping the tuple-normalization step outside the `RECOMPUTE`-only block (or explicitly applying it for `IGNORE` as well) while still skipping metadata/result persistence.
##########
tests/caching/test_integration.py:
##########
@@ -495,19 +495,41 @@ def test_ignore_behavior(tmp_path):
cache._ignore = ["A"]
# execution 3: a new key that ignores A will be recomputed for B
- # A doesn't produce any metadata or result
+ # A doesn't produce any metadata or result, only B adds a metadata entry for its <ignore> key
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A", "B"])
- check_metadata_store_size(cache=cache, size=4)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
# execution 4: B can be retrieved using its new key that ignores A
+ # A executes but does not produce any metadata, so size remains 3
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A"], did_not=["B"])
- check_metadata_store_size(cache=cache, size=5)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
+def test_ignore_behavior_from_start(tmp_path):
+ cache = HamiltonCacheAdapter(path=tmp_path)
+ module = ad_hoc_utils.create_temporary_module(node_A(), node_B_depends_on_A())
+ final_vars = ["B"]
+ cache._ignore = ["A"]
+
+ # execution 1: A executes without caching metadata or result; B executes and is cached with <ignore> key
+ execute_dataflow(module=module, cache=cache, final_vars=final_vars)
+ check_execution(cache=cache, did=["A", "B"])
+ check_metadata_store_size(cache=cache, size=1)
+ check_results_exist_in_store(cache, ["B"])
+ assert len([p for p in cache.result_store.path.iterdir() if p.name != "metadata_store.db"]) == 1
+
+ # execution 2: A re-executes, B hits cache; metadata store size stays 1
+ execute_dataflow(module=module, cache=cache, final_vars=final_vars)
+ check_execution(cache=cache, did=["A"], did_not=["B"])
+ check_metadata_store_size(cache=cache, size=1)
+ check_results_exist_in_store(cache, ["B"])
+ assert len([p for p in cache.result_store.path.iterdir() if p.name != "metadata_store.db"]) == 1
Review Comment:
These assertions couple the test to `FileResultStore`’s on-disk layout and assume the cache directory contains only result artifacts plus `metadata_store.db`. This is brittle if the result store implementation adds subdirectories/auxiliary files (or if other features create additional files in the same directory). Prefer asserting via store APIs (e.g., that only B’s `data_version` exists) or by deriving expected stored `data_version`s from `cache.metadata_store.get_run(...)` and checking `cache.result_store.exists(...)` for exactly those, without making assumptions about directory contents.
##########
tests/caching/test_integration.py:
##########
@@ -495,19 +495,41 @@ def test_ignore_behavior(tmp_path):
cache._ignore = ["A"]
# execution 3: a new key that ignores A will be recomputed for B
- # A doesn't produce any metadata or result
+ # A doesn't produce any metadata or result, only B adds a metadata entry for its <ignore> key
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A", "B"])
- check_metadata_store_size(cache=cache, size=4)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
# execution 4: B can be retrieved using its new key that ignores A
+ # A executes but does not produce any metadata, so size remains 3
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A"], did_not=["B"])
- check_metadata_store_size(cache=cache, size=5)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
+def test_ignore_behavior_from_start(tmp_path):
+ cache = HamiltonCacheAdapter(path=tmp_path)
+ module = ad_hoc_utils.create_temporary_module(node_A(), node_B_depends_on_A())
+ final_vars = ["B"]
+ cache._ignore = ["A"]
+
+ # execution 1: A executes without caching metadata or result; B executes and is cached with <ignore> key
+ execute_dataflow(module=module, cache=cache, final_vars=final_vars)
+ check_execution(cache=cache, did=["A", "B"])
+ check_metadata_store_size(cache=cache, size=1)
+ check_results_exist_in_store(cache, ["B"])
+ assert len([p for p in cache.result_store.path.iterdir() if p.name != "metadata_store.db"]) == 1
+
+ # execution 2: A re-executes, B hits cache; metadata store size stays 1
+ execute_dataflow(module=module, cache=cache, final_vars=final_vars)
+ check_execution(cache=cache, did=["A"], did_not=["B"])
+ check_metadata_store_size(cache=cache, size=1)
+ check_results_exist_in_store(cache, ["B"])
+ assert len([p for p in cache.result_store.path.iterdir() if p.name != "metadata_store.db"]) == 1
Review Comment:
These assertions couple the test to `FileResultStore`’s on-disk layout and assume the cache directory contains only result artifacts plus `metadata_store.db`. This is brittle if the result store implementation adds subdirectories/auxiliary files (or if other features create additional files in the same directory). Prefer asserting via store APIs (e.g., that only B’s `data_version` exists) or by deriving expected stored `data_version`s from `cache.metadata_store.get_run(...)` and checking `cache.result_store.exists(...)` for exactly those, without making assumptions about directory contents.
##########
tests/caching/test_integration.py:
##########
@@ -495,19 +495,41 @@ def test_ignore_behavior(tmp_path):
cache._ignore = ["A"]
# execution 3: a new key that ignores A will be recomputed for B
- # A doesn't produce any metadata or result
+ # A doesn't produce any metadata or result, only B adds a metadata entry for its <ignore> key
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A", "B"])
- check_metadata_store_size(cache=cache, size=4)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
# execution 4: B can be retrieved using its new key that ignores A
+ # A executes but does not produce any metadata, so size remains 3
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A"], did_not=["B"])
- check_metadata_store_size(cache=cache, size=5)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
+def test_ignore_behavior_from_start(tmp_path):
+ cache = HamiltonCacheAdapter(path=tmp_path)
+ module = ad_hoc_utils.create_temporary_module(node_A(), node_B_depends_on_A())
+ final_vars = ["B"]
+ cache._ignore = ["A"]
+
+ # execution 1: A executes without caching metadata or result; B executes and is cached with <ignore> key
+ execute_dataflow(module=module, cache=cache, final_vars=final_vars)
+ check_execution(cache=cache, did=["A", "B"])
+ check_metadata_store_size(cache=cache, size=1)
+ check_results_exist_in_store(cache, ["B"])
Review Comment:
The PR description says `test_ignore_behavior_from_start` verifies that an ignored-from-run-1 node writes 0 metadata entries and 0 result entries, but the test asserts `metadata_store.size == 1` (and expects one cached result for downstream node B). This looks like a wording mismatch: it’s accurate that A writes 0 entries, but the overall run still writes entries for B. Please update the PR description (or the test expectation) so they match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(caching): avoid storing metadata and results for CachingBehavior.IGNORE nodes [hamilton]
Posted by "Shoryamishra61 (via GitHub)" <gi...@apache.org>.
Shoryamishra61 commented on PR #1716:
URL: https://github.com/apache/hamilton/pull/1716#issuecomment-5789359341
Thanks for flagging this -- I missed the repo-wide `prek` formatting check on the last push. I am running the `uv` workflow now and will update the branch with the formatting fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(caching): avoid storing metadata and results for CachingBehavior.IGNORE nodes [hamilton]
Posted by "Shoryamishra61 (via GitHub)" <gi...@apache.org>.
Shoryamishra61 commented on code in PR #1716:
URL: https://github.com/apache/hamilton/pull/1716#discussion_r4054599785
##########
tests/caching/test_integration.py:
##########
@@ -495,19 +495,39 @@ def test_ignore_behavior(tmp_path):
cache._ignore = ["A"]
# execution 3: a new key that ignores A will be recomputed for B
- # A doesn't produce any metadata or result
+ # A doesn't produce any metadata or result, only B adds a metadata entry for its <ignore> key
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A", "B"])
- check_metadata_store_size(cache=cache, size=4)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
# execution 4: B can be retrieved using its new key that ignores A
+ # A executes but does not produce any metadata, so size remains 3
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A"], did_not=["B"])
- check_metadata_store_size(cache=cache, size=5)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
+def test_ignore_behavior_from_start(tmp_path):
+ cache = HamiltonCacheAdapter(path=tmp_path)
+ module = ad_hoc_utils.create_temporary_module(node_A(), node_B_depends_on_A())
+ final_vars = ["B"]
+ cache._ignore = ["A"]
+
+ # execution 1: A executes without caching metadata or result; B executes and is cached with <ignore> key
+ execute_dataflow(module=module, cache=cache, final_vars=final_vars)
+ check_execution(cache=cache, did=["A", "B"])
+ check_metadata_store_size(cache=cache, size=1)
+ check_results_exist_in_store(cache, ["B"])
Review Comment:
Updated! Added the explicit assertion verifying the result store contains only the single entry for B.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(caching): avoid storing metadata and results for CachingBehavior.IGNORE nodes [hamilton]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1716:
URL: https://github.com/apache/hamilton/pull/1716#discussion_r4054595760
##########
tests/caching/test_integration.py:
##########
@@ -495,19 +495,39 @@ def test_ignore_behavior(tmp_path):
cache._ignore = ["A"]
# execution 3: a new key that ignores A will be recomputed for B
- # A doesn't produce any metadata or result
+ # A doesn't produce any metadata or result, only B adds a metadata entry for its <ignore> key
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A", "B"])
- check_metadata_store_size(cache=cache, size=4)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
# execution 4: B can be retrieved using its new key that ignores A
+ # A executes but does not produce any metadata, so size remains 3
execute_dataflow(module=module, cache=cache, final_vars=final_vars)
check_execution(cache=cache, did=["A"], did_not=["B"])
- check_metadata_store_size(cache=cache, size=5)
+ check_metadata_store_size(cache=cache, size=3)
check_results_exist_in_store(cache, ["A", "B"])
+def test_ignore_behavior_from_start(tmp_path):
+ cache = HamiltonCacheAdapter(path=tmp_path)
+ module = ad_hoc_utils.create_temporary_module(node_A(), node_B_depends_on_A())
+ final_vars = ["B"]
+ cache._ignore = ["A"]
+
+ # execution 1: A executes without caching metadata or result; B executes and is cached with <ignore> key
+ execute_dataflow(module=module, cache=cache, final_vars=final_vars)
+ check_execution(cache=cache, did=["A", "B"])
+ check_metadata_store_size(cache=cache, size=1)
+ check_results_exist_in_store(cache, ["B"])
Review Comment:
This helper only iterates over the current run's metadata (and ignores its `expected_nodes` argument), so this assertion would still pass if `post_node_execute` wrote an ignored node's result without metadata. Since this PR changes the result-store branch too, assert that the fresh result store contains exactly B's single entry here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(caching): avoid storing metadata and results for CachingBehavior.IGNORE nodes [hamilton]
Posted by "skrawcz (via GitHub)" <gi...@apache.org>.
skrawcz commented on PR #1716:
URL: https://github.com/apache/hamilton/pull/1716#issuecomment-5787918715
@Shoryamishra61 looks good -- but can you fix the pre-commit check please? you should be able to run uv to fix things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(caching): avoid storing metadata and results for CachingBehavior.IGNORE nodes [hamilton]
Posted by "skrawcz (via GitHub)" <gi...@apache.org>.
skrawcz merged PR #1716:
URL: https://github.com/apache/hamilton/pull/1716
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org