Skip to content

Conversation

@marcorudolphflex
Copy link
Contributor

@marcorudolphflex marcorudolphflex commented Nov 3, 2025

Previous: Caching does not work with autograd and non-local gradients because some data access via web is tried to made with a dummy task id for cached simulations.

Solution: Get original task id from originally cached simulation to get the trace-relevant data.

Greptile Overview

Updated On: 2025-11-04 12:14:42 UTC

Greptile Summary

Fixes caching for autograd with non-local gradients by retrieving and using the original task_id from cached simulations.

Key changes:

  • Modified restore_simulation_if_cached to return both the restored path and the cached task_id
  • Updated autograd forward pass to check cache and use cached task_id when available for backward pass parent_tasks dependency
  • Changed stash path generation to use uuid.uuid4() instead of _cached_task_id to avoid None values
  • Added early exit in Batch.monitor for cached jobs

Issues found:

  • Critical bug in autograd.py:583-589: load() is called with path=None instead of using restored_path, which will cause the load to fail
  • Performance issue in container.py:1128: load_if_cached is a @cached_property but is accessed inside the monitor loop, causing repeated cache checks
  • Missing docstring update in webapi.py for the new tuple return value

Confidence Score: 2/5

  • This PR has critical bugs that will break the caching functionality for autograd
  • Score reflects a critical bug where load() is called with path=None instead of restored_path, which will cause autograd cache loading to fail. Additionally, there's a performance issue with repeated cached property access in the monitor loop.
  • Critical attention needed for tidy3d/web/api/autograd/autograd.py (incorrect path parameter). Performance issue in tidy3d/web/api/container.py (inefficient cached property access).

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/web/api/webapi.py 3/5 Modified restore_simulation_if_cached to return tuple of (path, task_id); has incorrect return type annotation and outdated docstring
tidy3d/web/api/container.py 3/5 Updated cache handling with _cached_task_id attribute and uuid-based stash paths; inefficient cached property access in monitor loop
tidy3d/web/api/autograd/autograd.py 2/5 Added cache restoration for non-local gradients; critical bug passing None as path to load() instead of using restored_path

Sequence Diagram

sequenceDiagram
    participant AG as Autograd
    participant Cache as restore_simulation_if_cached
    participant Load as load()
    participant Web as WebAPI
    
    Note over AG: Non-local gradient case
    AG->>Cache: restore_simulation_if_cached(sim_original)
    Cache-->>AG: (restored_path, cached_task_id)
    
    alt Cache Hit
        Note over AG: BUG: passes path=None instead of restored_path
        AG->>Load: load(task_id=None, path=None)
        Note over Load: Will fail - cannot load from None path
        Load--xAG: Error
    else Cache Miss
        AG->>Web: _run_tidy3d(sim_original)
        Web-->>AG: (sim_data_orig, task_id_fwd)
    end
    
    Note over AG: aux_data[FWD_TASK_ID] = task_id_fwd
    Note over AG: Later: Backward pass uses cached task_id
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.

Additional Comments (1)

  1. tidy3d/web/api/webapi.py, line 377-378 (link)

    syntax: docstring doesn't match the new return type - should document both the path and task_id return values

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@marcorudolphflex marcorudolphflex force-pushed the FXC-3948-fix-caching-for-autograd branch 3 times, most recently from 21d3662 to 4c25be6 Compare November 3, 2025 13:52
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

Diff Coverage

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

  • tidy3d/web/api/autograd/autograd.py (100%)
  • tidy3d/web/api/autograd/io_utils.py (100%)
  • tidy3d/web/api/container.py (100%)
  • tidy3d/web/api/webapi.py (100%)
  • tidy3d/web/cache.py (100%)

Summary

  • Total: 33 lines
  • Missing: 0 lines
  • Coverage: 100%

@marcorudolphflex marcorudolphflex force-pushed the FXC-3948-fix-caching-for-autograd branch 2 times, most recently from 60fd0f0 to 4b4365d Compare November 4, 2025 10:14
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.

@marcorudolphflex marcorudolphflex force-pushed the FXC-3948-fix-caching-for-autograd branch from 4b4365d to 976046a Compare November 4, 2025 11:59
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.

Additional Comments (1)

  1. tidy3d/web/api/webapi.py, line 375-378 (link)

    style: docstring not updated to reflect new return value - should document that it returns a tuple of (path, task_id)

7 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@marcorudolphflex marcorudolphflex force-pushed the FXC-3948-fix-caching-for-autograd branch from 976046a to ddd622d Compare November 4, 2025 12:58
@marcorudolphflex marcorudolphflex added this pull request to the merge queue Nov 4, 2025
Merged via the queue into develop with commit 88a8d75 Nov 4, 2025
26 checks passed
@marcorudolphflex marcorudolphflex deleted the FXC-3948-fix-caching-for-autograd branch November 4, 2025 14:07
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