Skip to content

Conversation

@marcorudolphflex
Copy link
Contributor

@marcorudolphflex marcorudolphflex commented Oct 24, 2025

…processing

currently, we observe duplicate log messages on warnings (and possibly multiple log downloads) on warnings when lazy is set to false, as we trigger it in “from_file” via the on_load and then again in Tidy3dStubData.postprocess

on_load(obj)

cls._check_convergence_and_warnings(stub_data)

I just removed the second call in postprocess.

As a little side-fix, I trigger a refresh on the upload progress bar in batches which was displayed as incomplete sometimes.

Greptile Overview

Updated On: 2025-10-24 11:08:10 UTC

Greptile Summary

Fixes duplicate convergence check warnings when lazy=False and improves progress bar refresh behavior for batch uploads.

Main Changes:

  • Removed redundant _check_convergence_and_warnings() call in Tidy3dStubData.postprocess() that was causing duplicate log messages when lazy=False. The convergence check is already triggered via the on_load callback in from_file() (line 407 in base.py), so the second call was unnecessary.
  • Renamed constant from BATCH_MONITOR_PROGRESS_REFRESH_TIME to BATCH_PROGRESS_REFRESH_TIME for consistency
  • Added progress.refresh() and sleep after upload completion in batch processing

Issues Found:

  • The progress bar refresh fix in container.py has a logical issue: the refresh and sleep are placed outside the loop (lines 842-843), executing only once after all uploads complete, which won't resolve incomplete progress displays during uploads. They should be inside the loop.

Confidence Score: 3/5

  • This PR is safe to merge with one logical issue that needs correction
  • Score reflects correct fix for duplicate convergence checks but a logical error in the progress bar refresh placement that prevents it from achieving its stated goal
  • Pay close attention to tidy3d/web/api/container.py lines 842-843 - the refresh logic is placed outside the loop

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/web/api/tidy3d_stub.py 5/5 Removed duplicate convergence check when lazy=False to prevent duplicate log messages
tidy3d/web/api/container.py 4/5 Renamed constant and added progress bar refresh after upload completion, potential issue with refresh placement

Sequence Diagram

sequenceDiagram
    participant Client
    participant Tidy3dStubData
    participant BaseModel
    participant LazyProxy
    participant ConvergenceCheck

    Note over Client,ConvergenceCheck: Postprocess with lazy=False (OLD BEHAVIOR)
    Client->>Tidy3dStubData: postprocess(file_path, lazy=False)
    Tidy3dStubData->>BaseModel: from_file(lazy=False, on_load=_check_convergence)
    BaseModel->>BaseModel: dict_from_file()
    BaseModel->>BaseModel: parse_obj()
    BaseModel->>ConvergenceCheck: on_load(_check_convergence) ✓ First call
    BaseModel-->>Tidy3dStubData: stub_data
    Note over Tidy3dStubData,ConvergenceCheck: OLD: Duplicate call here
    Tidy3dStubData->>ConvergenceCheck: _check_convergence(stub_data) ✗ REMOVED
    Tidy3dStubData-->>Client: stub_data

    Note over Client,ConvergenceCheck: Postprocess with lazy=True
    Client->>Tidy3dStubData: postprocess(file_path, lazy=True)
    Tidy3dStubData->>BaseModel: from_file(lazy=True, on_load=_check_convergence)
    BaseModel->>LazyProxy: create LazyProxy with on_load callback
    BaseModel-->>Tidy3dStubData: proxy
    Tidy3dStubData-->>Client: proxy (not yet loaded)
    Note over Client,LazyProxy: Later access triggers loading
    Client->>LazyProxy: access attribute
    LazyProxy->>LazyProxy: materialize data
    LazyProxy->>ConvergenceCheck: on_load(_check_convergence) ✓ Called on access
    LazyProxy-->>Client: data
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.

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@marcorudolphflex marcorudolphflex requested review from daquinteroflex and removed request for yaugenst-flex October 24, 2025 11:09
@github-actions
Copy link
Contributor

Diff Coverage

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

  • tidy3d/web/api/container.py (75.0%): Missing lines 1076

Summary

  • Total: 4 lines
  • Missing: 1 line
  • Coverage: 75%

tidy3d/web/api/container.py

Lines 1072-1080

  1072                             desc = pbar_description(task_name, display_status, max_name_length, 0)
  1073                             progress.update(pbar, description=desc, completed=pct)
  1074 
  1075                         progress.refresh()
! 1076                         time.sleep(BATCH_PROGRESS_REFRESH_TIME)
  1077 
  1078                     # final render to terminal state for all bars
  1079                     for task_name, job in self.jobs.items():
  1080                         schedule_download(job)

Copy link
Collaborator

@daquinteroflex daquinteroflex 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 added this pull request to the merge queue Oct 24, 2025
Merged via the queue into develop with commit 4552eaf Oct 24, 2025
52 of 54 checks passed
@marcorudolphflex marcorudolphflex deleted the FXC-3829-duplicate-convergence-checks-for-non-lazy-postprocessing branch October 24, 2025 13:14
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