Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix closed CacheAdapter #847

Merged
merged 2 commits into from
Apr 26, 2024
Merged

fix closed CacheAdapter #847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9d7053
fixed closed cache
Apr 25, 2024
f53afdd
updated tests to open cache
Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions hamilton/lifecycle/default.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,11 @@ def __init__(
key=CacheAdapter.nodes_history_key, default=dict()
)
self.used_nodes_hash: Dict[str, str] = dict()
self.cache.close()

def run_before_graph_execution(self, *, graph: HamiltonGraph, **kwargs):
"""Set `cache_vars` to all nodes if received None during `__init__`"""
self.cache = shelve.open(self.cache_path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something feels wrong about this. Aren't we just opening it again? Feels like it should:

  1. Open during init
  2. Run during execution
  3. Close at the end

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, that was the previous behavior... I want to have .close() somewhere for safety, but putting it in run_after_graph_execution means you can only run it once

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it just open again? Maybe:

  1. Open/close during __init__
  2. Open during run_before_graph_execution
  3. Run
  4. Close at run_after_graph_execution
    ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's what Thierry just did here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes, my bad

if self.cache_vars == []:
self.cache_vars = [n.name for n in graph.nodes]

Expand Down
4 changes: 4 additions & 0 deletions tests/lifecycle/test_cache_adapter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ def test_set_result(hook: CacheAdapter, node_a: node.Node):
cache_key = CacheAdapter.create_key(node_hash, node_kwargs)
result = 2

hook.run_before_graph_execution(graph=graph_types.HamiltonGraph([])) # needed to open cache
hook.cache_vars = [node_a.name]
# used_nodes_hash would be set by run_to_execute() hook
hook.used_nodes_hash[node_a.name] = node_hash
Expand All @@ -79,6 +80,7 @@ def test_get_result(hook: CacheAdapter, node_a: node.Node):
result = 2
cache_key = CacheAdapter.create_key(node_hash, node_kwargs)

hook.run_before_graph_execution(graph=graph_types.HamiltonGraph([])) # needed to open cache
hook.cache_vars = [node_a.name]
# run_after_node_execution() would set cache
hook.cache[cache_key] = result
Expand All @@ -103,6 +105,7 @@ def test_append_nodes_history(
node_kwargs = dict(external_input=7)
hook.cache_vars = [node_a.name]

hook.run_before_graph_execution(graph=graph_types.HamiltonGraph([])) # needed to open cache
# node version 1
hook.used_nodes_hash[node_name] = graph_types.hash_source_code(node_a.callable, strip=True)
hook.run_to_execute_node(
Expand All @@ -129,6 +132,7 @@ def test_commit_nodes_history(hook: CacheAdapter):
"""Commit node history to cache"""
hook.nodes_history = dict(A=["hash_1", "hash_2"])

hook.run_before_graph_execution(graph=graph_types.HamiltonGraph([])) # needed to open cache
hook.run_after_graph_execution()

# need to reopen the hook cache
Expand Down