Skip to content

Conversation

@marcorudolphflex
Copy link
Contributor

@marcorudolphflex marcorudolphflex commented Nov 4, 2025

Implements cache CLI helpers:

  • list: Shows cache entries
  • info: Shows local cache config, no. of entries and total storage size
  • clear: clears cache

Sidefix: Move cache directory instead of deleting it on change.

Note: Branch was originally coming from this one (#2945) as the cache stats from there are used, this is why greptile comments are partially misleading.

Greptile Overview

Updated On: 2025-11-04 15:12:18 UTC

Greptile Summary

This PR implements CLI helpers for the local cache system, along with a major refactoring to improve performance by introducing a lightweight stats tracking mechanism.

Key Changes:

  • Added three new CLI commands: tidy3d cache info (shows config and usage stats), tidy3d cache list (displays all cache entries), and tidy3d cache clear (removes all cached data)
  • Introduced CacheStats and CacheEntryMetadata Pydantic models to replace loose dictionaries for better type safety
  • Implemented stats.json file to track cache metadata (last_used, total_size, total_entries) without needing to iterate through all cache entries
  • Replaced threading.Lock with threading.RLock and added _with_lock() context manager for cleaner lock management
  • Optimized __len__() method to read from stats file instead of iterating all entries
  • Added sync_stats() method to rebuild stats file from actual cache entries when inconsistencies are detected
  • Improved eviction logic to work with stats dictionary instead of iterating entries, with proper handling of incoming entries during limit enforcement

Issues Found:

  • One critical bug in error handling path (line 800 in tidy3d/web/cache.py) where old_root._root should be old_root

Confidence Score: 4/5

  • This PR is safe to merge after fixing the critical syntax error on line 800
  • The implementation is well-designed with comprehensive tests covering all new functionality. The stats tracking system is a smart optimization that avoids expensive directory iteration. However, there's one critical bug in the error handling path (line 800) where old_root._root should be old_root, which would cause an AttributeError if that code path is executed. The rest of the code is solid with proper locking, atomic operations, and thorough test coverage.
  • Pay close attention to tidy3d/web/cache.py line 800 - the syntax error must be fixed before merging

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/web/cli/cache.py 5/5 New CLI commands for cache management (list, info, clear) - clean implementation with proper error handling
tidy3d/web/cli/app.py 5/5 Added import and registration for cache_group CLI command - minimal change
tidy3d/web/cache.py 4/5 Major refactoring: added CacheStats and CacheEntryMetadata models, stats tracking system, and performance optimizations. One potential issue with error reference in line 800.
tests/test_web/test_local_cache.py 5/5 Added comprehensive tests for stats tracking, sync, CLI commands, and performance optimizations - thorough coverage

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as CLI Commands
    participant Cache as LocalCache
    participant Stats as CacheStats
    participant FS as File System

    Note over User,FS: Cache Info Command
    User->>CLI: tidy3d cache info
    CLI->>Cache: sync_stats()
    Cache->>FS: Read stats.json
    FS-->>Cache: Return stats data
    Cache->>Cache: _iter_entries() to rebuild if needed
    Cache->>FS: Write updated stats.json
    FS-->>Cache: Stats written
    Cache-->>CLI: Return CacheStats
    CLI-->>User: Display info (enabled, entries, size, limits)

    Note over User,FS: Cache List Command
    User->>CLI: tidy3d cache list
    CLI->>Cache: list()
    Cache->>Cache: _with_lock()
    Cache->>Cache: _iter_entries()
    Cache->>FS: Read metadata from each entry
    FS-->>Cache: Entry metadata
    Cache-->>CLI: Return list of entries
    CLI-->>User: Display formatted entries

    Note over User,FS: Store Result with Stats Tracking
    User->>Cache: store_result(data, task_id, path, type)
    Cache->>Cache: _with_lock()
    Cache->>Cache: _ensure_limits()
    Cache->>Stats: _load_stats()
    Stats-->>Cache: Current stats
    Cache->>Cache: Check if eviction needed
    alt Eviction Required
        Cache->>Cache: _evict() or _evict_by_size()
        Cache->>FS: Remove old entries
        Cache->>Stats: _record_remove_stats()
    end
    Cache->>FS: Copy artifact to temp location
    Cache->>Cache: Calculate checksum
    Cache->>FS: Write metadata.json
    Cache->>FS: Atomic rename to final location
    Cache->>Stats: _record_store_stats()
    Stats->>FS: Write updated stats.json
    Cache-->>User: Entry stored

    Note over User,FS: Fetch with Stats Update
    User->>Cache: try_fetch(simulation)
    Cache->>Cache: _with_lock()
    Cache->>Cache: build_cache_key()
    Cache->>Cache: _fetch(key)
    Cache->>FS: Load metadata
    Cache->>Cache: verify() checksum
    Cache->>Cache: _touch(entry)
    Cache->>Stats: _record_touch_stats()
    Stats->>FS: Update stats.json with new last_used
    Cache-->>User: Return CacheEntry

    Note over User,FS: Clear Cache
    User->>CLI: tidy3d cache clear
    CLI->>Cache: clear()
    Cache->>FS: Remove cache directory
    Cache->>FS: Write empty stats.json
    CLI-->>User: "Local cache cleared."
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@marcorudolphflex marcorudolphflex force-pushed the FXC-3903-tidy-3-d-cli-cache-helpers branch 2 times, most recently from 2415c8c to 0c721f8 Compare November 4, 2025 15:25
@marcorudolphflex marcorudolphflex force-pushed the FXC-3903-tidy-3-d-cli-cache-helpers branch from 0c721f8 to 21657d5 Compare November 5, 2025 12:57
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/web/cache.py (70.0%): Missing lines 852-854
  • tidy3d/web/cli/app.py (100%)
  • tidy3d/web/cli/cache.py (96.7%): Missing lines 20,30

Summary

  • Total: 72 lines
  • Missing: 5 lines
  • Coverage: 93%

tidy3d/web/cache.py

  848         try:
  849             new_root.parent.mkdir(parents=True, exist_ok=True)
  850             if old_root.exists():
  851                 shutil.move(old_root, new_root)
! 852         except Exception as e:
! 853             log.warning(f"Failed to move cache directory: {e}. Delete old cache.")
! 854             shutil.rmtree(old_root)
  855 
  856     _CACHE = LocalCache(
  857         directory=config.local_cache.directory,
  858         max_entries=config.local_cache.max_entries,

tidy3d/web/cli/cache.py

  16     for unit in ["B", "KB", "MB", "GB", "TB"]:
  17         if num_bytes < 1024.0 or unit == "TB":
  18             return f"{num_bytes:.2f} {unit}"
  19         num_bytes /= 1024.0
! 20     return f"{num_bytes:.2f} B"  # fallback, though unreachable
  21 
  22 
  23 def _get_cache(ensure: bool = True) -> Optional[LocalCache]:
  24     """Resolve the local cache object, surfacing errors as ClickExceptions."""

  26         cache = resolve_local_cache(use_cache=True)
  27     except Exception as exc:  # pragma: no cover - defensive guard
  28         raise click.ClickException(f"Failed to access local cache: {exc}") from exc
  29     if cache is None and ensure:
! 30         raise click.ClickException("Local cache is disabled in the current configuration.")
  31     return cache
  32 
  33 
  34 @click.group(name="cache")

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Nice!

@yaugenst-flex yaugenst-flex added this pull request to the merge queue Nov 5, 2025
Merged via the queue into develop with commit d39a059 Nov 5, 2025
55 of 59 checks passed
@yaugenst-flex yaugenst-flex deleted the FXC-3903-tidy-3-d-cli-cache-helpers branch November 5, 2025 16:11
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.

3 participants