Skip to content

Conversation

@dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Oct 29, 2025

Greptile Overview

Updated On: 2025-10-29 01:51:19 UTC

Greptile Summary

Replaces the brittle is_running_pytest() function (which checked for pytest in sys.modules) with a more explicit environment variable-based approach. The RF license warning is now suppressed during tests via TIDY3D_MICROWAVE__SUPPRESS_RF_LICENSE_WARNING=true in pytest config.

Key changes:

  • Deleted tidy3d/utils.py entirely (contained only the is_running_pytest() helper)
  • Updated MicrowaveBaseModel to use config.microwave.suppress_rf_license_warning instead of the deprecated flat config path
  • Added TIDY3D_MICROWAVE__SUPPRESS_RF_LICENSE_WARNING=true to pytest environment variables in pyproject.toml
  • Fixed typo: default_contructeddefault_constructed

Benefits:

  • More explicit control over warning suppression
  • Works consistently across different test environments
  • Leverages the existing config system's environment variable parsing

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-structured, properly tested, and improve the robustness of test warning suppression. The typo fix is a bonus improvement. All changes are backwards compatible through the legacy config wrapper.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
CHANGELOG.md 5/5 Added changelog entry for more robust RF license warning suppression
pyproject.toml 5/5 Added environment variable to suppress RF license warnings during pytest runs
tidy3d/components/microwave/base.py 5/5 Removed is_running_pytest() check, fixed typo (contructed -> constructed), updated to use config.microwave.suppress_rf_license_warning
tidy3d/utils.py 5/5 File deleted - contained only the now-unused is_running_pytest() function

Sequence Diagram

sequenceDiagram
    participant Pytest
    participant EnvLoader as ConfigLoader.load_environment_overrides()
    participant Config as config.microwave
    participant MWBase as MicrowaveBaseModel
    participant Logger as log.warning()

    Note over Pytest: pytest starts with env var:<br/>TIDY3D_MICROWAVE__SUPPRESS_RF_LICENSE_WARNING=true
    
    Pytest->>EnvLoader: Parse TIDY3D_* environment variables
    EnvLoader->>Config: Set suppress_rf_license_warning = True
    
    Note over MWBase: User instantiates MicrowaveBaseModel subclass
    MWBase->>MWBase: _warn_rf_license() validator runs
    MWBase->>Config: Check config.microwave.suppress_rf_license_warning
    
    alt suppress_rf_license_warning == True
        Config-->>MWBase: True (suppressed)
        Note over MWBase: Skip warning
    else suppress_rf_license_warning == False
        Config-->>MWBase: False
        MWBase->>Logger: Emit RF license warning
    end
    
    Note over MWBase: Alternative: _default_without_license_warning()
    MWBase->>Config: Temporarily set suppress_rf_license_warning = True
    MWBase->>MWBase: Instantiate cls()
    MWBase->>Config: Restore suppress_rf_license_warning = False
Loading

@dmarek-flex dmarek-flex self-assigned this Oct 29, 2025
@dmarek-flex dmarek-flex changed the title fix(rf): more robust suppression of RF license warning fix(rf): FXC-3877 more robust suppression of RF license warning Oct 29, 2025
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, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2025

Diff Coverage

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

  • tidy3d/components/microwave/base.py (100%)

Summary

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

@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-3877-fix-suppress-rf-license-warning branch from a920ba2 to c5e7ad3 Compare October 29, 2025 02:49
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.

Thanks @dmarek-flex LGTM!

@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 29, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 29, 2025
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-3877-fix-suppress-rf-license-warning branch from c5e7ad3 to 56aff17 Compare October 29, 2025 16:43
@dmarek-flex dmarek-flex force-pushed the dmarek/FXC-3877-fix-suppress-rf-license-warning branch from 97300f7 to a2bfe21 Compare October 30, 2025 17:46
@dmarek-flex dmarek-flex enabled auto-merge October 30, 2025 17:46
@dmarek-flex dmarek-flex added this pull request to the merge queue Oct 30, 2025
Merged via the queue into develop with commit 3f24f99 Oct 30, 2025
26 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/FXC-3877-fix-suppress-rf-license-warning branch October 30, 2025 19:31
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.

4 participants