Skip to content

Conversation

caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Oct 10, 2025

Some minor improvements to packaging:

  • disable_local_subpixel should restore the config even if the function it wraps errors
  • check if the module exists before trying to import it, to discriminate between different types of import errors
  • a bit better error messages

Greptile Overview

Updated On: 2025-10-10 17:41:16 UTC

Greptile Summary

This PR strengthens the packaging system for the optional tidy3d-extras dependency by improving error handling and module detection logic. The changes refactor the supports_local_subpixel decorator in tidy3d/packaging.py to use find_spec() for more robust module existence checking before attempting imports, providing clearer error differentiation between "module not found" vs "module failed to initialize". The disable_local_subpixel decorator is also enhanced with proper try/finally blocks to ensure configuration state is always restored even when exceptions occur.

Additionally, a new test function test_tidy3d_extras() is added to tests/test_components/test_packaging.py that validates the packaging behavior for both scenarios when tidy3d-extras is available and when it's not. This test ensures that the @supports_local_subpixel decorator correctly detects the module, loads features, and sets appropriate flags. The changes improve the reliability of Tidy3D's optional dependency handling, which is critical for a library that extends core functionality through optional packages.

Important Files Changed

Changed Files
Filename Score Overview
tidy3d/packaging.py 4/5 Refactored module detection logic using find_spec() and improved error handling with try/finally blocks
tests/test_components/test_packaging.py 4/5 Added comprehensive test for tidy3d-extras packaging functionality and module detection

Confidence score: 4/5

  • This PR improves robustness of optional dependency handling with minimal risk of breaking existing functionality
  • Score reflects solid improvements to error handling and testing coverage, but complexity in module loading logic requires careful review
  • Pay close attention to the refactored supports_local_subpixel decorator logic in tidy3d/packaging.py

Sequence Diagram

sequenceDiagram
    participant User
    participant test_tidy3d_extras
    participant importlib
    participant supports_local_subpixel
    participant get_eps
    participant tidy3d_extras_global
    participant config
    participant tidy3d_extras_mod

    User->>test_tidy3d_extras: "Run test_tidy3d_extras()"
    test_tidy3d_extras->>importlib: "find_spec('tidy3d_extras')"
    importlib-->>test_tidy3d_extras: "spec or None"
    test_tidy3d_extras->>test_tidy3d_extras: "has_tidy3d_extras = spec is not None"
    test_tidy3d_extras->>get_eps: "@supports_local_subpixel decorated call"
    
    supports_local_subpixel->>config: "Check config.use_local_subpixel"
    
    alt config.use_local_subpixel is False
        supports_local_subpixel->>tidy3d_extras_global: "Set use_local_subpixel = False"
        supports_local_subpixel->>tidy3d_extras_global: "Set mod = None"
    else config.use_local_subpixel is not False and mod is None
        supports_local_subpixel->>tidy3d_extras_global: "Set use_local_subpixel = False"
        supports_local_subpixel->>importlib: "find_spec('tidy3d_extras')"
        importlib-->>supports_local_subpixel: "spec or None"
        
        alt module exists
            supports_local_subpixel->>tidy3d_extras_mod: "import tidy3d_extras"
            tidy3d_extras_mod-->>supports_local_subpixel: "tidy3d_extras_mod"
            supports_local_subpixel->>tidy3d_extras_mod: "Check __version__"
            supports_local_subpixel->>tidy3d_extras_mod: "extension._features()"
            tidy3d_extras_mod-->>supports_local_subpixel: "features list"
            supports_local_subpixel->>tidy3d_extras_global: "Set mod = tidy3d_extras_mod"
            supports_local_subpixel->>tidy3d_extras_global: "Set use_local_subpixel = 'local_subpixel' in features"
        end
    end
    
    supports_local_subpixel->>get_eps: "Call original function"
    
    alt has_tidy3d_extras is True
        get_eps->>tidy3d_extras_global: "Assert mod is not None"
        get_eps->>tidy3d_extras_global: "Get mod.extension._features()"
        get_eps->>get_eps: "Assert use_local_subpixel == ('local_subpixel' in features)"
    else has_tidy3d_extras is False
        get_eps->>tidy3d_extras_global: "Assert use_local_subpixel is False"
        get_eps->>tidy3d_extras_global: "Assert mod is None"
    end
    
    get_eps-->>test_tidy3d_extras: "Function execution complete"
    test_tidy3d_extras-->>User: "Test complete"
Loading

@caseyflex caseyflex force-pushed the chore/casey/extrastest branch from 6c1a07b to b263531 Compare October 10, 2025 17:41
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 (2)

  1. tests/test_components/test_packaging.py, line 79 (link)

    style: Consider removing this print statement as it's debug output that shouldn't be in production tests

    Context Used: Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)

  2. tests/test_components/test_packaging.py, line 5 (link)

    style: The config import is unused in this file - consider removing it if not needed

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@caseyflex caseyflex force-pushed the chore/casey/extrastest branch from b263531 to c47d63f Compare October 10, 2025 17:43
Copy link
Contributor

Diff Coverage

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

  • tidy3d/packaging.py (66.7%): Missing lines 201,209-210,212

Summary

  • Total: 12 lines
  • Missing: 4 lines
  • Coverage: 66%

tidy3d/packaging.py

Lines 197-205

  197             if tidy3d_extras["mod"] is None:
  198                 tidy3d_extras["use_local_subpixel"] = False
  199                 module_exists = find_spec("tidy3d_extras") is not None
  200                 if config.use_local_subpixel is True and not module_exists:
! 201                     raise Tidy3dImportError(
  202                         "The package 'tidy3d-extras' is required for this "
  203                         "operation when 'config.use_local_subpixel' is 'True'. "
  204                         "Please install the 'tidy3d-extras' package using, for "
  205                         "example, 'pip install tidy3d[extras]'."

Lines 205-216

  205                         "example, 'pip install tidy3d[extras]'."
  206                     )
  207 
  208                 if module_exists:
! 209                     try:
! 210                         import tidy3d_extras as tidy3d_extras_mod
  211 
! 212                     except ImportError as exc:
  213                         # this should not happen; if the API key is invalid,
  214                         # the package should still import but the version will be None
  215                         raise Tidy3dImportError(
  216                             "The package 'tidy3d-extras' did not initialize correctly. "

@caseyflex caseyflex force-pushed the chore/casey/extrastest branch from c47d63f to 74694a9 Compare October 10, 2025 22: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.

1 participant