Skip to content

feat: Add filesystem caching - #93

Closed
PaleNeutron wants to merge 4 commits into
pawamoy:mainfrom
PaleNeutron:feat/filesystem-caching
Closed

PaleNeutron wants to merge 4 commits into
pawamoy:mainfrom
PaleNeutron:feat/filesystem-caching

Conversation

@PaleNeutron

@PaleNeutron PaleNeutron commented Dec 4, 2025 •

Copy link
Copy Markdown

Summary

This PR implements a filesystem-based caching system for code execution results, providing cross-build persistence and faster documentation builds.

Key Features

🚀 Filesystem Caching

  • All cache stored in .markdown-exec-cache/ directory
  • SHA-256 hash-based caching with automatic invalidation when code or execution options change

🔄 Cache Refresh

  • Global refresh: MARKDOWN_EXEC_CACHE_REFRESH=1 environment variable forces re-execution of all cached blocks
  • Useful for CI/CD pipelines and when external dependencies change

🧹 Automatic Stale Cleanup

  • MkDocs builds: stale cache files (from removed or changed code blocks) are deleted at the end of each build (on_post_build)
  • Standalone usage: cleanup runs automatically when the Python process exits

🧪 Test Improvements

  • 15 comprehensive cache-specific tests
  • Test isolation with temporary cache directories

📚 Documentation

  • Complete caching guide at docs/usage/caching.md
  • Best practices and troubleshooting guide
  • Updated README with caching quickstart

Changes

New Files

  • src/markdown_exec/_internal/cache.py - CacheManager implementation
  • tests/test_cache.py - Comprehensive test suite
  • docs/usage/caching.md - User documentation

Modified Files

  • src/markdown_exec/_internal/formatters/base.py - Integrated caching
  • src/markdown_exec/_internal/main.py - Added cache option
  • tests/conftest.py - Test isolation fixture
  • src/markdown_exec/__init__.py - Exposed CacheManager in public API
  • .gitignore - Added .markdown-exec-cache/
  • README.md - Added caching section
  • mkdocs.yml - Added caching docs to navigation

Usage Examples

Hash-Based Caching (Auto-Invalidation)

```python exec="yes" cache="yes"
import time
time.sleep(5)
print("done")
```

Global Refresh (All Caches)

MARKDOWN_EXEC_CACHE_REFRESH=1 mkdocs build

Clear Cache Manually

rm -rf .markdown-exec-cache/

Breaking Changes

None — this is a new feature. Existing code blocks work unchanged.

- Implement CacheManager class for code execution result caching
- Support hash-based caching (automatic invalidation on code changes)
- Support custom cache IDs for cross-build persistence
- Add MARKDOWN_EXEC_CACHE_REFRESH environment variable for global refresh
- Add refresh option to force cache updates per code block
- Store cache files in .markdown-exec-cache/ directory
- Add comprehensive test suite with 14 cache-specific tests
- Add caching documentation with usage examples
- Update README with caching quickstart
- Isolate test cache directories to prevent pollution

This improves documentation build performance by caching expensive
operations like plot generation, API calls, and computations.
@PaleNeutron PaleNeutron mentioned this pull request Dec 4, 2025
@PaleNeutron PaleNeutron changed the title feat: Add filesystem-only caching with global refresh support feat: Add filesystem caching with global refresh support Dec 4, 2025
@pawamoy

pawamoy commented Dec 4, 2025

Copy link
Copy Markdown
Owner

It looks like you've used AI. Did you thoroughly review every code change and docs change?

@PaleNeutron

Copy link
Copy Markdown
Author

It looks like you've used AI. Did you thoroughly review every code change and docs change?

Yes, I have reviewed it.

@pawamoy

pawamoy commented Dec 4, 2025

Copy link
Copy Markdown
Owner

Thanks. What is your motivation here, are you a user of markdown-exec?

@pawamoy pawamoy left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

While the PR looks good, as often with code models, it doesn't really address what was said in #4. Specifically, the intent to actually track cached files (for use in CI) is not reflected in this PR. As a consequence, the PR also does nothing for removing stale cache entries.

Comment thread README.md
```
````

Use custom cache IDs for persistence across builds:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Force some reason the AI advertises custom IDs as providing persistence across builds throughout the change, while this is of course provided by the caching feature globally. There will be some rewording to do.

Comment thread README.md
```
````

Force cache refresh with `refresh="yes"`:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not convinced by the utility of this refresh feature. Having to update the code block, build docs, then modify the code block back seems tedious. I'd prefer a solution where an environment variable is used, as for global refreshes. This would require IDs of course, so, to be discussed.

Generally speaking, this feature doesn't seem to come from the requirements identified in #4. It doesn't address what was said there.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have provide MARKDOWN_EXEC_CACHE_REFRESH=1 for global control and I think refresh="yes" is necessary for user who are modifying code and just affect a little piece of document. He can quickly modify project source code and just run one code block to see if everything is fine.

If user try to remove cache="true" to let block always run, when he add it back, the cache system will use previous cache result which will lead to mistake.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Maybe we could check every file under cache dir if it is currently mentioned in build system and and remove all legacy cache files. Thus we can remove the refresh="yes" option. It affect performance a little, but it sounds worth it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@pawamoy which approach do you prefer? Use refresh="yes" or manage all cache files in cache dir?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The stale cache entries must be automatically deleted for sure 🙂

Comment thread docs/usage/caching.md
@@ -0,0 +1,245 @@
# Caching

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The page generally looks good, except for some parts which don't make a lot of sense. It's also a bit too verbose. This would have to be reworked (I will provide more details).

Comment on lines +195 to +206
cache_manager = get_cache_manager()
cache_id = cache if isinstance(cache, str) else None
cache_options = {
"language": language,
"html": html,
"result": result,
"returncode": returncode,
"workdir": workdir,
"width": width,
"extra": extra,
}
cache_manager.set(cache_id, source_input, output, **cache_options)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It seems wasteful to recompute everything. We can reuse the previously computed cache id and cache options, at least.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right, I'll change this.

@pawamoy

pawamoy commented Dec 4, 2025 •

Copy link
Copy Markdown
Owner

I am also a bit annoyed by the fact that you (or the agent you used) didn't respect the PR template we provide. The original issue (#4) wasn't even linked properly. And the markup is broken, which makes me doubt you actually thoroughly reviewed this.

@PaleNeutron

Copy link
Copy Markdown
Author

I am also a bit annoyed by the fact that you (or the agent you used) didn't respect the PR template we provide. The original issue (#4) wasn't even linked properly. And the markup is broken, which makes me doubt you actually thoroughly reviewed this.

What do you mean about "the markup is broken"?

@pawamoy

pawamoy commented Dec 4, 2025 •

Copy link
Copy Markdown
Owner

The Usage Examples in the PR body are ill-formatted (it failed to use quadruple-backtick fences).

@PaleNeutron
PaleNeutron force-pushed the feat/filesystem-caching branch from ab0abd5 to 91d8cab Compare December 5, 2025 03:28
@PaleNeutron

Copy link
Copy Markdown
Author

The Usage Examples in the PR body are ill-formatted (it failed to use quadruple-backtick fences).

Sorry, I am not an expert of markdown and this is the first time I know about quadruple-backtick fences. I have fixed them in the body.

@pawamoy

pawamoy commented Dec 5, 2025

Copy link
Copy Markdown
Owner

No worries, thanks for updating the PR body.

@pawamoy

pawamoy commented Mar 5, 2026

Copy link
Copy Markdown
Owner

Closing for lack of update, @PaleNeutron feel free to comment whether you'd like to continue work on this and I'll reopen 🙂

@pawamoy pawamoy closed this Mar 5, 2026
@PaleNeutron

Copy link
Copy Markdown
Author

@pawamoy , sorry for completely forget this PR. Please reopen this and I'll continue working on this.

@PaleNeutron PaleNeutron changed the title feat: Add filesystem caching with global refresh support feat: Add filesystem caching Mar 6, 2026
@pawamoy pawamoy reopened this Mar 6, 2026
@PaleNeutron

Copy link
Copy Markdown
Author

@pawamoy , can you review this PR? I removed refresh parameter and custom cache id feature. Now cache="true" is the only option and cache manager will clean stale files after build finished.

@pawamoy

pawamoy commented Mar 11, 2026

Copy link
Copy Markdown
Owner

I have re-read #4 and the changes here, and I'm not comfortable with the changes here. This PR does too much at once.

Ideally, we would work incrementally, as suggested in #4. First, very basic caching based on code block ID. No ID, no caching. A mechanism to refresh cache based on a regular expression matching IDs. Then provide best-effort automatic cache invalidation, etc. (I won't repeat what's in #4).

Furthermore, markdown-exec was initially made to be used within MkDocs projects. Nowadays, I'm focusing on Zensical instead, which natively provides caching mechanisms, possibly rendering dedicated caching in markdown-exec useless. Therefore I'd like that we go back to discussing the feature in #4. I'll copy this last paragraph there.

For now, I'll be closing this PR. I hope you can understand. Thank you again for contributing to this project 🙂

@pawamoy pawamoy closed this Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants