Skip to content

Conversation

@marcorudolphflex
Copy link
Contributor

@marcorudolphflex marcorudolphflex commented Dec 5, 2025

We need to remove the heavy ssl import for the WASM build.
As this is urgent and moving all typing imports behind TYPE_CHECKING is not straightforward with automatic fixes due to pydantic models and the original ssl problem is not solvable by that in general, I decided to only remove this import/type in the config package for now.

Open question: Do we want to extend the except block for all exceptions to catch ssl.SSLError?

Greptile Overview

Greptile Summary

This PR successfully removes the ssl module import from the config package (tidy3d/config/) by changing the ssl_version field from ssl.TLSVersion enum to string-based storage. The implementation includes comprehensive validation that normalizes various TLS version input formats (e.g., "TLS 1.2", "tls1.2", "1.2") into canonical forms (e.g., "TLSv1_2"), and converts back to the enum at the point of use in http_util.py.

  • Removed ssl import from tidy3d/config/legacy.py and tidy3d/config/sections.py
  • Added robust TLS version validation with regex-based normalization supporting multiple input formats
  • Updated type hints from Optional[ssl.TLSVersion] to Optional[str] throughout config modules
  • Maintained backward compatibility by accepting ssl.TLSVersion enum objects as input (via .name attribute)
  • Added comprehensive test coverage for normalization and validation logic
  • Updated existing tests to expect string outputs instead of enum values

Note: Two other files (tidy3d/web/cli/app.py and tidy3d/plugins/dispersion/web.py) still import and catch ssl.SSLError, which relates to the PR author's open question about extending exception handling. These files were not modified in this PR and may need future attention for WASM compatibility.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk, as it maintains backward compatibility and includes comprehensive tests
  • Score of 4 reflects a well-implemented refactor with thorough testing. The string-based TLS version storage successfully removes ssl imports from the config package while maintaining full backward compatibility. The validation logic is robust and handles edge cases well. Minor deduction because: (1) there's a theoretical edge case where an invalid string could bypass validation and cause a KeyError in http_util.py (though unlikely with current validation), and (2) the PR doesn't address ssl.SSLError handling in other modules (web/cli/app.py, plugins/dispersion/web.py), though this appears intentional as noted in the PR description.
  • No files require special attention - the implementation is solid across all changed files

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/config/sections.py 4/5 Added TLS version normalization with string-based storage instead of ssl.TLSVersion enum; comprehensive validation logic handles multiple input formats
tidy3d/web/core/http_util.py 5/5 Converts string TLS version back to ssl.TLSVersion enum when creating SSL context; maintains backward compatibility
tidy3d/config/legacy.py 5/5 Updated type hints from ssl.TLSVersion to str; removed ssl import from legacy config module

Sequence Diagram

sequenceDiagram
    participant User
    participant WebConfig
    participant Validator
    participant HttpUtil
    participant SSLContext

    User->>WebConfig: Set ssl_version="TLS 1.2"
    WebConfig->>Validator: _normalize_ssl_version_name(value)
    Validator->>Validator: Strip spaces & normalize
    Validator->>Validator: Regex match TLS version
    Validator->>Validator: Convert to canonical "TLSv1_2"
    Validator->>Validator: Validate against TLS_VERSION_CHOICES
    Validator-->>WebConfig: Return "TLSv1_2" (string)
    WebConfig-->>User: Store as string

    User->>HttpUtil: Make HTTPS request
    HttpUtil->>WebConfig: Get ssl_version (string)
    WebConfig-->>HttpUtil: "TLSv1_2"
    HttpUtil->>HttpUtil: Convert ssl.TLSVersion["TLSv1_2"]
    HttpUtil->>SSLContext: create_urllib3_context(ssl_version=enum)
    SSLContext-->>HttpUtil: SSL context with TLS 1.2
    HttpUtil-->>User: Execute request with SSL context
Loading

@marcorudolphflex
Copy link
Contributor Author

@greptile

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/core/http_util.py, line 203 (link)

    logic: create_urllib3_context expects ssl_version to be an ssl.TLSVersion enum or None, not a string. Since config.web.ssl_version is now a string (e.g., "TLSv1_2"), this will cause a TypeError at runtime when TLS version is configured.

    You need to convert the string back to ssl.TLSVersion enum before passing it to create_urllib3_context:

    ssl_version_enum = None
    if config.web.ssl_version:
        ssl_version_enum = getattr(ssl.TLSVersion, config.web.ssl_version, None)
    context = create_urllib3_context(ssl_version=ssl_version_enum)

6 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@marcorudolphflex marcorudolphflex force-pushed the FXC-4436-move-typing-only-imports-behind-if-type-checking branch 2 times, most recently from aa70ca7 to 72dcafe Compare December 5, 2025 15:31
@marcorudolphflex marcorudolphflex marked this pull request as ready for review December 5, 2025 15:32
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.

6 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@marcorudolphflex marcorudolphflex changed the title refactor(tidy3d): FXC-4436-move-typing-only-imports-behind-if-type-checking refactor(tidy3d): FXC-4436-disable-ssl-type-in-config Dec 5, 2025
@marcorudolphflex marcorudolphflex changed the title refactor(tidy3d): FXC-4436-disable-ssl-type-in-config refactor(tidy3d): FXC-4436-avoid-ssl-import-for-wasm-build Dec 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2025

Diff Coverage

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

  • tidy3d/config/legacy.py (100%)
  • tidy3d/config/sections.py (100%)
  • tidy3d/web/core/http_util.py (25.0%): Missing lines 205-206,211-214

Summary

  • Total: 24 lines
  • Missing: 6 lines
  • Coverage: 75%

tidy3d/web/core/http_util.py

  201 
  202 
  203 class TLSAdapter(HTTPAdapter):
  204     def init_poolmanager(self, *args: Any, **kwargs: Any) -> None:
! 205         try:
! 206             ssl_version = (
  207                 ssl.TLSVersion[config.web.ssl_version]
  208                 if config.web.ssl_version is not None
  209                 else None
  210             )
! 211         except KeyError:
! 212             log.warning(f"Invalid SSL/TLS version '{config.web.ssl_version}', using default")
! 213             ssl_version = None
! 214         context = create_urllib3_context(ssl_version=ssl_version)
  215         kwargs["ssl_context"] = context
  216         return super().init_poolmanager(*args, **kwargs)
  217 

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 @marcorudolphflex. Only comment is let's keep this as minimal as possible.

@marcorudolphflex marcorudolphflex force-pushed the FXC-4436-move-typing-only-imports-behind-if-type-checking branch from 72dcafe to db09ac4 Compare December 10, 2025 12:15
@marcorudolphflex marcorudolphflex force-pushed the FXC-4436-move-typing-only-imports-behind-if-type-checking branch from db09ac4 to 7235a7e Compare December 10, 2025 12:56
@yaugenst-flex yaugenst-flex added this pull request to the merge queue Dec 10, 2025
Merged via the queue into develop with commit 8581468 Dec 10, 2025
33 checks passed
@yaugenst-flex yaugenst-flex deleted the FXC-4436-move-typing-only-imports-behind-if-type-checking branch December 10, 2025 13:44
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