fix: caching SQLiteMetadataStore.get_run_ids()

[zilto]

This PR follows issue #1204

It includes a few changes to the MetadataStore base class and sets a clearer contract for .get_run() and .get_run_ids().

• .get_run_ids() should return a list of runs sorted by their start time, i.e., when .initialize() is called. This order may differ from the order of the first recorded node execution. An empty list will be returned if no run were initialized.

• .get_run() should return a list of dictionaries, where each dict is associated with a particular node execution. Each dict must minimally return the data_version and the cache_key (which can be decoded to retrieve the node_name, code_version, and dependencies_data_versions), but may include more information. An IndexError is raised if run_id doesn't exist. An empty list is returned if the run was initialized, but recorded no node execution.

Tests were added to catch the bug reported in the original issue.

[elijahbenizzy]

Looks good, unclear about a few of the other API changes (**kwargs), but I think we should ship it! Also docs

[skrawcz]

this could be backwards incompatible. But if we're the only ones using this, then all good.

[zilto]

It's not because the HamiltonCacheAdapter is the only object passing values here. As shown in the updated logic of SQLiteMetadataStore.set() it will resolve the values of node_name and code_version from the cache_key so these values will never be None (for instance, the SQL table still enforce NOT NULL constraints)

For instance, the MetadataStore base class doesn't expose node_name and code_version as arguments because other implementations might no provide them directly. This information is redundant with the content of cache_key.

The SQLiteMetadataStore implementation uses node_name and code_version explicitly to allow indexing and cache management (e.g., delete all instance of this node). These values could be derived by decoding cache_key but that would be a wasteful operation

[skrawcz]

it's more that if people were not using keyword args it would be break due to the change in argument order; fix is easy though as you just update all callers to the new order.

[skrawcz]

(I didn't see any update to callers of this function hence why I asked)

[ellipsis-dev]

👍 Looks good to me! Incremental review on e6958eb in 14 seconds

More details

• Looked at 38 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. hamilton/caching/stores/sqlite.py:210

• Draft comment:
  Consider implementing a caching mechanism for get_run_ids() to improve performance, especially if this method is called frequently. This could involve storing the result in memory and invalidating the cache when new runs are initialized.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. hamilton/caching/stores/sqlite.py:210

• Draft comment:
  This function is complex and would benefit from a comment explaining its purpose and main steps.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_YCg2stvuI4USVCy2

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.