Skip to content

chore(tidy3d): FXC-4559-add-type-checking-import-moving-and-checking-to-pre-commit-and-ci #3093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marcorudolphflex wants to merge 4 commits into
base: yaugenst-flex/pydantic-v2
Choose a base branch
from
Open

chore(tidy3d): FXC-4559-add-type-checking-import-moving-and-checking-to-pre-commit-and-ci #3093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
99a1386
refactor: pydantic-v2 and mypy
mahlau-flex Dec 15, 2025
bf28388
chore(tidy3d): FXC-4545-move-type-hints-imports-behind-type-checking
marcorudolphflex Dec 15, 2025
5293ae1
chore(tidy3d): FXC-4545-move-type-hints-imports-behind-type-checking
marcorudolphflex Dec 16, 2025
d80cef2
chore(tidy3d): FXC-4559-add-type-checking-import-moving-and-checking-…
marcorudolphflex Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
40 changes: 40 additions & 0 deletions .github/workflows/tidy3d-python-client-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,39 @@ jobs:
echo "version_match_tests=$version_match_tests"
echo "extras_integration_tests=$extras_integration_tests"
echo "test_type=$test_type"


move-type-imports:
name: move-type-imports
needs: determine-test-scope
if: needs.determine-test-scope.outputs.code_quality_tests == 'true'
runs-on: ubuntu-latest
steps:
- name: checkout-branch
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.ref }}
repository: ${{ github.event.pull_request.head.repo.full_name }}
fetch-depth: 0
persist-credentials: false

- name: setup-python
uses: actions/setup-python@v5
with:
python-version: '3.11'

- name: install-dependencies
run: |
set -euo pipefail
python -m venv .venv
source .venv/bin/activate
pip install libcst

- name: verify-type-import-guards
run: |
set -euo pipefail
source .venv/bin/activate
python scripts/move_type_imports.py --mode check_on_change

lint:
needs: determine-test-scope
Expand Down Expand Up @@ -981,6 +1014,7 @@ jobs:
- determine-test-scope
- local-tests
- remote-tests
- move-type-imports
- lint
- mypy
- verify-schema-change
Expand All @@ -993,6 +1027,12 @@ jobs:
- extras-integration-tests
runs-on: ubuntu-latest
steps:
- name: move-type-imports
if: ${{ needs.determine-test-scope.outputs.code_quality_tests == 'true' && needs.move-type-imports.result != 'success' && needs.move-type-imports.result != 'skipped' }}
run: |
echo "❌ Found imports used only for typing that are not guarded by if TYPE_CHECKING."
exit 1

- name: check-linting-result
if: ${{ needs.determine-test-scope.outputs.code_quality_tests == 'true' && needs.lint.result != 'success' && needs.lint.result != 'skipped' }}
run: |
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Batch JSON files
batch*.json
*.vtu
simulation.json

# Byte-compiled / optimized / DLL files
*$py.class
Expand Down
8 changes: 8 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@ default_install_hook_types:
- pre-commit
- commit-msg
repos:
- repo: local
hooks:
- id: move-type-imports
name: move type-only imports under TYPE_CHECKING
entry: poetry run python scripts/move_type_imports.py --mode fix --only-changed
Copy link
Collaborator

@daquinteroflex daquinteroflex Dec 16, 2025

Choose a reason for hiding this comment

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

Is there a way we can do this without using poetry? Just because not everyone has it installed?

Copy link
Collaborator

@daquinteroflex daquinteroflex Dec 16, 2025

Choose a reason for hiding this comment

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

Thinking about it, should we just avoid it in the precommit but keep it in CI?

Copy link
Contributor Author

@marcorudolphflex marcorudolphflex Dec 16, 2025

Choose a reason for hiding this comment

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

Was not expecting that...
Either we might skip it or do this one?

  - repo: local
    hooks:
      - id: move-type-imports
        name: move type-only imports under TYPE_CHECKING
        entry: python scripts/move_type_imports.py --mode fix --only-changed
        language: python
        additional_dependencies:
          - "libcst==1.4.0"
        pass_filenames: false
        stages: [ pre-commit ]

Copy link
Collaborator

@daquinteroflex daquinteroflex Dec 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think this will work for people who don't even have python installed in the system? Just checking how standalone it is? Is it fully self contained?

Copy link
Contributor Author

@marcorudolphflex marcorudolphflex Dec 16, 2025

Choose a reason for hiding this comment

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

It only requires python with the libcst package.
So it's not perfectly standalone, but I would not assume that these users use the pre-commit hook? I mean there is no force to do it...

language: system
pass_filenames: false
stages: [pre-commit]
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: "v0.11.11"
hooks:
Expand Down
5 changes: 5 additions & 0 deletions docs/api/plugins/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
Plugins
=======

.. warning::

|:warning:| The 'adjoint' plugin (legacy JAX-based adjoint plugin)
was deprecated in Tidy3D 'v2.7.0' and is disabled as of 'v2.9.0'.

.. toctree::
:maxdepth: 1

Expand Down
332 changes: 229 additions & 103 deletions poetry.lock
View file
Open in desktop

Large diffs are not rendered by default.

73 changes: 12 additions & 61 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ numpy = "*"
matplotlib = "*"
shapely = "^2.0"
pandas = "*"
pydantic = "^2.0"
pydantic = ">=2.9,<3"
pydantic-settings = "^2.10.0"
PyYAML = "*"
dask = "*"
toml = "*"
Expand Down Expand Up @@ -69,6 +70,7 @@ tox = { version = "*", optional = true }
diff-cover = { version = "*", optional = true }
zizmor = { version = "*", optional = true }
mypy = { version = "1.13.0", optional = true }
libcst = { version = "*", optional = true }

# gdstk
gdstk = { version = ">=0.9.49", optional = true }
Expand Down Expand Up @@ -155,6 +157,7 @@ dev = [
'pytest-cov',
'rtree',
'ruff',
'libcst',
'sax',
'signac',
'sphinx',
Expand Down Expand Up @@ -309,7 +312,13 @@ banned-module-level-imports = ["scipy", "matplotlib"]

[tool.pytest.ini_options]
# TODO: remove --assert=plain when https://github.com/scipy/scipy/issues/22236 is resolved
addopts = "--cov=tidy3d --doctest-modules -n auto --dist worksteal --assert=plain -m 'not numerical'"
# TODO(yaugenst-flex): Revisit adjoint plugin for pydantic v2
addopts = """
--doctest-modules -n auto --dist worksteal --assert=plain -m 'not numerical' \
--ignore=tests/test_plugins/test_adjoint.py \
--ignore=tidy3d/plugins/adjoint/
--ignore=tests/test_plugins/test_adjoint.py
"""
markers = [
"numerical: marks numerical tests for adjoint gradients that require running simulations (deselect with '-m \"not numerical\"')",
]
Expand All @@ -331,82 +340,24 @@ python_files = "*.py"
[tool.mypy]
python_version = "3.10"
files = [
"tidy3d/web",
"tidy3d/config",
"tidy3d/material_library",
"tidy3d/components/geometry"
"tidy3d",
]
ignore_missing_imports = true
follow_imports = "skip"
disallow_untyped_defs = true
disable_error_code = [
"abstract",
"annotation-unchecked",
"arg-type",
"assert-type",
"assignment",
"attr-defined",
"await-not-async",
"call-arg",
"call-overload",
"comparison-overlap",
"dict-item",
"empty-body",
"exit-return",
"explicit-override",
"func-returns-value",
"has-type",
"ignore-without-code",
"import",
"import-not-found",
"import-untyped",
"index",
"list-item",
"literal-required",
"method-assign",
"misc",
"mutable-override",
"name-defined",
"name-match",
"narrowed-type-not-subtype",
"no-any-return",
"no-any-unimported",
"no-overload-impl",
"no-redef",
"no-untyped-call",
"operator",
"overload-cannot-match",
"overload-overlap",
"override",
"possibly-undefined",
"prop-decorator",
"redundant-cast",
"redundant-expr",
"redundant-self",
"return",
"return-value",
"safe-super",
"str-bytes-safe",
"str-format",
"syntax",
"top-level-await",
"truthy-bool",
"truthy-function",
"truthy-iterable",
"type-abstract",
"type-arg",
"type-var",
"typeddict-item",
"typeddict-readonly-mutated",
"typeddict-unknown-key",
"unimported-reveal",
"union-attr",
"unreachable",
"unused-awaitable",
"unused-coroutine",
"unused-ignore",
"used-before-def",
"valid-newtype",
"valid-type",
"var-annotated",
]
Expand Down
Loading