Skip to content

ENH: Extending node caching - #127

Draft
Carwyn Pelley (cpelley) wants to merge 6 commits into
mainfrom
controllable_pickle
Draft

Carwyn Pelley (cpelley) wants to merge 6 commits into
mainfrom
controllable_pickle

Conversation

@cpelley

@cpelley Carwyn Pelley (cpelley) commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Utilises existing DAGrunner cache support to provide:

  • Per-node cache control, extending the previously supported global on/off behaviour.
  • Atomic cache writes to improve robustness and prevent partial cache files.
  • Cache filenames now incorporate a tokenised extract value to guarantee uniqueness and avoid collisions.
  • Additional unit test coverage.
  • Dependency updates, including correcting an overly restrictive Python version upper bound that inadvertently limited support to Python 3.9 Needed updating now so as not to unnecessarily handle differences in tokenization with dask.

This change may be relevant to EPP, based on my current understanding of the caching-related issue they are investigating.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Tests pass as required.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 High severity · 1 Medium severity

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_result to plugin_executor to enable caching on a per-node basis (in addition to the existing global cache toggle).
  • Updated _PickleCache pickle 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.

Comment thread dagrunner/execute_graph.py Resolved
Comment thread dagrunner/utils/_cache.py Resolved
Comment thread .github/workflows/tests.yml Resolved
Comment thread dagrunner/utils/_cache.py Resolved
Comment thread dagrunner/utils/_cache.py Resolved
Comment thread dagrunner/execute_graph.py Resolved
@cpelley

Copy link
Copy Markdown
Collaborator Author

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.

@cpelley
Carwyn Pelley (cpelley) marked this pull request as draft October 20, 2026 09:25
@cpelley Carwyn Pelley (cpelley) changed the title ENH: Support for specific node caching ENH: Extending node caching Oct 29, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants