Repository navigation
ENH: Extending node caching - #127
Carwyn Pelley (cpelley) wants to merge 6 commits into
Conversation
Rob Purvis (mo-robert-purvis)
left a comment
There was a problem hiding this comment.
Looks good. Tests pass as required.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are correctness/security issues in the cache file path construction and the cache “atomic” write behavior, plus a dry-run caching bug and a test isolation issue that should be fixed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
This PR enhances DAGrunner’s caching capabilities by allowing per-node opt-in caching, improving cache file naming uniqueness, and adding/adjusting tests around cache utilization.
Changes:
- Added
cache_resulttoplugin_executorto enable caching on a per-node basis (in addition to the existing global cache toggle). - Updated
_PickleCachepickle filename scheme to include a tokenized component and changed the write path intended to be more atomic. - Added a unit test for single-node caching and updated existing cache-related test naming/docs.
| File | Description |
|---|---|
dagrunner/utils/_cache.py |
Adjusts cache file naming and cache write behavior. |
dagrunner/execute_graph.py |
Adds per-node caching control via cache_result and documents it. |
dagrunner/tests/execute_graph/test_plugin_executor.py |
Renames global cache test and adds a single-node cache test + fixture. |
dagrunner/tests/execute_graph/test_integration.py |
Updates cache utilization test docstring to reflect graph filtering behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6a4bc92 to
ea94cf7
Compare
|
Returning this PR to draft. I'm not yet confident that tokenising cache arguments is the right approach without understanding how it fits into failure recovery and the wider caching problem. The outcome of that investigation may significantly influence this implementation, so I'd rather revisit this once I have a clearer picture. As far as I'm aware, nobody is currently waiting on this change. |
98a6c8d to
57ff0cf
Compare


Utilises existing DAGrunner cache support to provide:
This change may be relevant to EPP, based on my current understanding of the caching-related issue they are investigating.