Skip to content

Conversation

@yaugenst-flex
Copy link
Collaborator

@yaugenst-flex yaugenst-flex commented Aug 1, 2025

  • Introduced a structured config manager that loads profiles from the XDG base directory by default (~/.config/tidy3d/config.toml on macOS/Linux, %APPDATA%\tidy3d\config.toml on Windows) while still honoring the legacy ~/.tidy3d/config.json when present.
    • Only a small set of options is exposed and persisted currently, with per-field flags controlling whether values are written back to disk. We can add more later, but we'll have to consider schema stability.
    • Added a migration helper (tidy3d config migrate) that copies legacy files into the new location and format, plus tidy3d config reset to clear the profile; loader warnings now point users to these commands.
    • Legacy APIs remain available: existing code can still read tidy3d.config objects, the wrapper keeps API parity, and we continue to fall back to in-memory defaults if no profile is found.

Greptile Overview

Updated On: 2025-10-20 12:44:02 UTC

Greptile Summary

This PR introduces a comprehensive configuration system overhaul that migrates from a legacy flat JSON-based config (~/.tidy3d/config.json) to a structured, profile-based TOML system following XDG Base Directory standards. The new architecture uses ConfigManager to orchestrate configuration loading from multiple sources (builtin profiles, base config, user profiles, environment variables) with clear precedence, while maintaining backward compatibility through LegacyConfigWrapper. Configuration constants previously scattered across the codebase—especially autograd-related parameters like MAX_NUM_ADJOINT_PER_FWD, GRADIENT_DTYPE_FLOAT, and cylinder discretization settings—are now centralized under config.adjoint.* and accessible at runtime. The system includes CLI helpers (tidy3d config migrate, tidy3d config reset), plugin registration infrastructure, per-field persistence flags, and extensive test coverage. All existing APIs remain functional through wrapper classes that emit deprecation warnings, allowing gradual migration while providing users immediate access to configuration flexibility.

Important Files Changed

Changed Files
Filename Score Overview
tidy3d/config/manager.py 4/5 Core config orchestrator with profile switching, section updates with rollback, persistence filtering, and multi-source merging
tidy3d/config/loader.py 0/5 Not analyzed - likely handles XDG directory resolution and TOML file I/O
tidy3d/config/legacy.py 4/5 Compatibility shim providing deprecated attribute-level access to new manager, including environment variable backup/restore that could lose data
tidy3d/config/init.py 3/5 Public API creation with singleton manager, legacy wrapper, and reload helper; potential race condition in global state management
tidy3d/config/registry.py 5/5 Plugin-style registration system for config sections and handlers using decorators and module-level globals
tidy3d/config/serializer.py 3.5/5 TOML comment-preserving serializer; potential TypeError when calling issubclass on non-class annotations
tidy3d/config/sections.py 0/5 Not analyzed - likely defines config section schemas
tidy3d/config/profiles.py 5/5 Built-in profile definitions for prod/dev/uat/pre/nexus environments with endpoints and S3 regions
tidy3d/web/cli/app.py 1/5 CLI refactor with critical bugs: duplicate command name decorators and double registration of config-reset
tidy3d/web/cli/migrate.py 4/5 API key migration now uses config manager; potential data loss if save() fails before renaming auth.json
tidy3d/web/cli/constants.py 4/5 Consolidated config directory resolution using XDG helper; filename extension change from 'config' to 'config.toml'
tidy3d/web/core/environment.py 4.5/5 Now a re-export shim for backward compatibility after moving environment logic to tidy3d.config
tidy3d/web/core/http_util.py 4/5 API key retrieval now uses config manager; error message still references obsolete file paths
tidy3d/web/api/autograd/autograd.py 4/5 Autograd limits now config-driven; traced field serialization moved before primitive call; getattr hook for backward compatibility
tidy3d/web/api/autograd/backward.py 5/5 Replaced hardcoded ADJOINT_FREQ_CHUNK_SIZE with config.adjoint.solver_freq_chunk_size and added robust chunking logic
tidy3d/web/api/autograd/ops_backward.py 4/5 New file extracting adjoint ops with frequency chunking controlled by config; likely refactored from another module
tidy3d/web/api/run.py 5/5 max_num_adjoint_per_fwd now Optional[int] defaulting to config.adjoint.max_adjoint_per_fwd at runtime
tidy3d/components/autograd/constants.py 2/5 File emptied - all constants removed; high risk if imports not updated across codebase
tidy3d/components/autograd/derivative_utils.py 4/5 Migrated gradient dtypes and wavelength fractions to config; redundant import inside method; potential performance overhead
tidy3d/components/geometry/base.py 4/5 Major autograd refactor: surface integral approach replaces point-sampling; xarray-based integration with PEC singularity handling
tidy3d/components/geometry/polyslab.py 4/5 ~40 call sites replaced with config.adjoint.* access; changed 2D z-slice logic removes clamping
tidy3d/components/geometry/primitives.py 0/5 Not analyzed
tidy3d/components/structure.py 0/5 Not analyzed
tidy3d/packaging.py 3/5 Decoupled from config using module-level global; disable_local_subpixel lost try-finally restoration on exception
tidy3d/plugins/smatrix/component_modelers/base.py 4/5 max_num_adjoint_per_fwd parameter now Optional with config fallback when None
tidy3d/config.py 4/5 Legacy file removed and replaced with empty file; compatibility layer in config/init.py
CHANGELOG.md 5/5 Clear user-facing entry documenting profile-based TOML config with XDG directories
pyproject.toml 5/5 Added tomlkit ^0.13.2 dependency for formatting-preserving TOML write operations
docs/api/configuration.rst 5/5 New API documentation for config manager, legacy wrapper, and registration utilities
docs/api/constants.rst 4/5 Reference changed from Tidy3dConfig class to config instance
docs/api/index.rst 4.5/5 Added configuration entry to API toctree
docs/configuration/index.rst 4.5/5 Comprehensive user guide for new config system with examples and migration path
docs/configuration/migration.rst 4/5 Migration guide with critical error: references legacy file as .toml instead of .json
docs/index.rst 3/5 Updated installation instructions but line 79 still references old $HOME/.tidy3d/config path
docs/install.rst 5/5 Removed hardcoded legacy config instructions; now delegates to configuration/index
tests/config/conftest.py 4/5 Test fixtures for env cleanup and temp config dir; potential base_dir/TIDY3D_BASE_DIR mismatch
tests/config/test_loader.py 4.5/5 Tests comment preservation, plugin registration, and CLI reset; cleanup uses private _SECTIONS
tests/config/test_manager.py 5/5 Comprehensive manager tests covering defaults, overlays, profiles, env var precedence, and adjoint config
tests/config/test_legacy.py 4/5 Validates backward compatibility of legacy API attribute access and Env enum
tests/config/test_profiles.py 4/5 Basic profile save tests; lacks validation of persisted-only fields and profile isolation
tests/config/test_plugins.py 4/5 Plugin registration and persistence tests; doesn't verify persist flag enforcement
tests/test_package/test_config.py 4.5/5 Migrated from pydantic v1 to v2; _test_frozen disabled pending new frozen implementation
tests/test_package/test_log.py 5/5 Import updated from pydantic.v1.ValidationError to pydantic.ValidationError
tests/test_components/autograd/test_autograd.py 4.5/5 Three autograd constants replaced with config.adjoint.* attribute access
["f6a669d8-0060-4f11-9cac-10ac7ee749ea", "809457d4-6b33-46ed-b49b-37057d02807e"]

@yaugenst-flex yaugenst-flex self-assigned this Aug 1, 2025
@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config branch 4 times, most recently from 9198880 to 918eaa9 Compare September 29, 2025 12:39
@yaugenst-flex yaugenst-flex changed the title wip: config overhaul Config overhaul Sep 29, 2025
@yaugenst-flex yaugenst-flex added the rc2 2nd pre-release label Sep 29, 2025
@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config branch 3 times, most recently from b3b2a4d to e68bea1 Compare October 1, 2025 19:44
@yaugenst-flex yaugenst-flex changed the title Config overhaul (FXC-3298) Config overhaul Oct 16, 2025
@yaugenst-flex yaugenst-flex changed the title (FXC-3298) Config overhaul FXC-3298 Config overhaul Oct 16, 2025
@yaugenst-flex yaugenst-flex removed the rc2 2nd pre-release label Oct 20, 2025
@yaugenst-flex
Copy link
Collaborator Author

@lucas-flexcompute tagging you as I think this will also be relevant for PF. In a follow-up, we'll also want to integrate e.g. the local caching here. We can consider making the cache path configurable.

@yaugenst-flex yaugenst-flex marked this pull request as ready for review October 20, 2025 12:42
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 88-93 (link)

    style: Update the error message to reflect the new config file location and format (XDG base directory, .toml format) instead of referring to the legacy ~/.tidy3d/config file.

44 files reviewed, 32 comments

Edit Code Review Agent Settings | Greptile

@flexcompute flexcompute deleted a comment from github-actions bot Oct 20, 2025
@yaugenst-flex yaugenst-flex added the ignore_diff_coverage Ignores CI comments on diff coverage label Oct 20, 2025
@flexcompute flexcompute deleted a comment from github-actions bot Oct 20, 2025
@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/config branch 7 times, most recently from 3ad69ef to dd49d7d Compare October 22, 2025 07:35
Copy link
Collaborator

@lucas-flexcompute lucas-flexcompute left a comment

Choose a reason for hiding this comment

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

Looks nice! Is it possible to add config sections dynamically? Meaning, during photonforge initialization (after tidy3d) I @register_section to retrieve photonforge-specific configuration?

@yaugenst-flex
Copy link
Collaborator Author

Is it possible to add config sections dynamically? Meaning, during photonforge initialization (after tidy3d) I @register_section to retrieve photonforge-specific configuration?

Yes! That's exactly the intended usage. @register_section(photonforge) will do the right thing and you'll be able to access that config via tidy3d.config.photonforge.key. If you config.save() afterward, the photonforge section will also be persisted to disk. On load, unregistered sections will still be loaded (into _raw_tree) and once @register_section runs, the default models will be hydrated based on those values. The hierarchical loading and import timing are probably the biggest sources of complexity in this PR, bit of a pain 😆

Copy link
Collaborator

@momchil-flex momchil-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. Reading through the docs, it sounds good. We'll need to update the backend a bit to avoid deprecation warnings in some places but this can be done after this is merged. Full disclosure I did not really go carefully through the all the code. It does seem like there might be some things that still need some cleanup though.

Another thought I had while reading through this: you've moved the various constants for the adjoint plugin to the config setting, making them "feel" like they are configurable by the user. However, that would only be the case if they use local gradients. For remote gradients (default) the default config will always be used.

@yaugenst-flex
Copy link
Collaborator Author

@momchil-flex Another thought I had while reading through this: you've moved the various constants for the adjoint plugin to the config setting, making them "feel" like they are configurable by the user. However, that would only be the case if they use local gradients. For remote gradients (default) the default config will always be used.

Yeah good point I forgot about that. I tried to be conservative with what we expose, especially with what is persisted, but I might have to iterate on that a bit more. My hope is that once this is in, we can start moving some of our globals (not only autograd) into the config. Eventually we probably also would need to transmit the config and other runtime settings if they should have an effect.

@momchil-flex
Copy link
Collaborator

@momchil-flex Another thought I had while reading through this: you've moved the various constants for the adjoint plugin to the config setting, making them "feel" like they are configurable by the user. However, that would only be the case if they use local gradients. For remote gradients (default) the default config will always be used.

Yeah good point I forgot about that. I tried to be conservative with what we expose, especially with what is persisted, but I might have to iterate on that a bit more. My hope is that once this is in, we can start moving some of our globals (not only autograd) into the config. Eventually we probably also would need to transmit the config and other runtime settings if they should have an effect.

Well, kind of, but we have to be conservative too, because the purpose of a lot of the restrictions is to avoid indefinite vjp processing on our servers.

@yaugenst-flex
Copy link
Collaborator Author

@momchil-flex I've done the following now:

  • Add local_gradient as a config setting, and persist it to file
  • Move all other adjoint-related settings to memory-only, none are persisted to the config file by default
  • Whenever you modify one of the adjoint config settings, you get a warning if local_gradient=False telling you to use local gradients otherwise the settings will have no effect

I think for now that should work?

@momchil-flex
Copy link
Collaborator

Nice, sounds good!

@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 24, 2025
Merged via the queue into develop with commit 8bdf76e Oct 24, 2025
25 checks passed
@yaugenst-flex yaugenst-flex deleted the yaugenst-flex/config branch October 24, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.10 ignore_diff_coverage Ignores CI comments on diff coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants