Skip to content

Conversation

@momchil-flex
Copy link
Collaborator

@momchil-flex momchil-flex commented Nov 24, 2025

web.monitor would error when a simulation diverges. Previously we did not want this to happen because a diverged simulation still contains results if the user e.g. wants to examine where the divergence happens, etc.

Moving the diverge(d) statuses to completed state restores previous behavior. The user still sees a warning when the SimulationData is loaded.

image

Greptile Overview

Greptile Summary

Moved diverge and diverged states from ERROR_STATES to COMPLETED_STATES to restore previous behavior where web.monitor doesn't error on diverged simulations.

  • Key change: Diverged simulations now complete successfully instead of erroring, allowing users to examine results at the point of divergence
  • Issue found: STATE_PROGRESS_PERCENTAGE dictionary still marks diverge/diverged as 0% instead of COMPLETED_PERCENT (100%), creating inconsistency with their new classification as completed states

Confidence Score: 3/5

  • This PR is mostly safe but has an inconsistency that should be fixed
  • The core logic change is sound and restores expected behavior, but there's an inconsistency in STATE_PROGRESS_PERCENTAGE where diverge/diverged states still show 0% instead of 100%, which could cause confusion or incorrect progress reporting
  • tidy3d/web/api/states.py requires attention to fix the STATE_PROGRESS_PERCENTAGE inconsistency

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/web/api/states.py 3/5 Moved diverge and diverged from ERROR_STATES to COMPLETED_STATES, but STATE_PROGRESS_PERCENTAGE still shows them as 0% instead of 100%

Sequence Diagram

sequenceDiagram
    participant User
    participant web.monitor
    participant get_status
    participant states
    
    User->>web.monitor: monitor(task_id)
    web.monitor->>get_status: get_status(task_id)
    get_status->>states: check status in END_STATES
    
    alt Status is "diverged" (BEFORE this PR)
        states-->>get_status: status in ERROR_STATES
        get_status-->>web.monitor: raise WebError
        web.monitor-->>User: Error thrown
    else Status is "diverged" (AFTER this PR)
        states-->>get_status: status in COMPLETED_STATES
        get_status-->>web.monitor: status = "diverged"
        web.monitor-->>User: Complete successfully
        Note over User: User can load SimulationData<br/>and see warning about divergence
    end
Loading

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR moves the diverge and diverged simulation states from ERROR_STATES to COMPLETED_STATES to restore previous behavior where diverged simulations don't cause web.monitor to error, allowing users to examine results even when divergence occurs.

  • Removes "diverge" and "diverged" from ERROR_STATES
  • Adds "diverge" and "diverged" to COMPLETED_STATES
  • Formats COMPLETED_STATES as a multi-line set for better readability

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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/states.py, line 101-102 (link)

    logic: diverge and diverged moved to COMPLETED_STATES but still showing 0% progress. Should these be 100% like other completed states?

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@momchil-flex momchil-flex force-pushed the momchil/diverged_state branch from 796d96d to bbea48a Compare November 24, 2025 13:23
@momchil-flex
Copy link
Collaborator Author

Close in favor of #3022

@momchil-flex momchil-flex deleted the momchil/diverged_state branch November 24, 2025 13:43
@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/web/api/states.py (100%)

Summary

  • Total: 1 line
  • Missing: 0 lines
  • Coverage: 100%

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