Skip to content

ci: make ty type checking blocking#223

Open
binaryaaron wants to merge 6 commits into
mainfrom
binaryaaron/chore/blocking-ty
Open

ci: make ty type checking blocking#223
binaryaaron wants to merge 6 commits into
mainfrom
binaryaaron/chore/blocking-ty

Conversation

@binaryaaron

@binaryaaron binaryaaron commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Related Issue

No linked issue required: maintainer-owned CI, type-safety, and contributor-workflow maintenance.

Plan Document

No plan required: this is a bounded enforcement and boundary-hardening change developed and validated through review.

Summary

  • Pin ty to 0.0.58 and make warnings and errors blocking across source, tests, docs, scripts, and tools.
  • Run the same four Make-backed checks locally, in pre-commit, and as discrete CI stages.
  • Resolve resulting diagnostics with narrow runtime-preserving fixes and preserve NumPy-wrapped entity handling.
  • Replace the distributed detection wrapper with Data Designer's native serializable plugin configuration, including fresh-process round-trip coverage.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Refactoring
  • CI, release, or contributor workflow update

Contributor Checklist

  • PR title follows Conventional Commits.
  • Related issue is linked, or a maintainer-owned no-issue reason is documented above.
  • A plan is linked, or the no-plan reason is documented above.
  • Public API and bundled skill impact checked.
  • No real PII or secrets added.

Validation

  • make check
  • make test — 1085 passed on the prior implementation head
  • Focused native-config and interface tests — 60 passed on the final reviewed branch
  • GitHub CI — all 12 checks passed on the prior pushed head; final-head checks will rerun after push
  • No open inline review threads

E2E model-service tests were not run because the changes do not alter model-provider or external-service execution.

Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
@binaryaaron
binaryaaron marked this pull request as ready for review July 15, 2026 18:06
@binaryaaron
binaryaaron requested review from a team as code owners July 15, 2026 18:06
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR promotes ty type checking from advisory to blocking across the full repository — source, tests, docs, scripts, and tools — by pinning ty to 0.0.58, adding --error-on-warning, wiring the same four Make targets into both CI and pre-commit, and resolving all resulting diagnostics with narrow, runtime-preserving fixes. It also replaces the deleted distributed.py worker-factory module with Data Designer's native serializable plugin configuration, backed by round-trip and fresh-subprocess tests.

  • CI and pre-commit hardening: the old advisory typecheck job (continue-on-error: true, || true) is collapsed into a unified check job; pre-commit now runs make check on every commit so the same gates apply locally.
  • Type-safety fixes across ~30 files: hasattr duck-type guards replaced with isinstance(…, BaseModel) checks, cast() and structural Protocols introduced where structural typing was previously suppressed with # type: ignore, and NumPy-array entity-list handling made explicit in _unwrap_entities and _entity_label (with a new log-level regression test).
  • Distributed module replaced: anonymizer.distributed.build_detection_builder deleted; callers should now serialise via builder.get_builder_config().to_json() and reconstruct with DataDesignerConfigBuilder.from_config(), with the worker discovering detection plugins through Data Designer's plugin registry.

Confidence Score: 4/5

Safe to merge; the fixes are narrow and runtime-preserving with good test coverage for the new serialization path.

All changes are mechanical type-correctness fixes or CI wiring. The three findings are minor: an assert that can be stripped under -O, a duplicate private protocol across five files, and resolve_model_aliases returning the original list by reference rather than a copy. None affect correctness in normal execution.

src/anonymizer/interface/anonymizer.py (assert-based narrowing and duplicated protocol); src/anonymizer/engine/ndd/model_loader.py (list returned by reference).

Important Files Changed

Filename Overview
.github/workflows/ci.yml Merged advisory typecheck job into the main check job; typecheck is now blocking with no continue-on-error.
.pre-commit-config.yaml Replaced commented-out advisory typecheck hook with a blocking make check pre-commit hook that runs on every commit.
Makefile Removed leading - from typecheck target (no longer advisory) and added --group docs to include docs-group deps for type checking.
pyproject.toml Pins ty to exact version 0.0.58; expands [tool.ty.src] include list to cover tests, docs, scripts, and tools; adds root to the environment section.
src/anonymizer/distributed.py File deleted; its distributed-worker factory is replaced by Data Designer's native serializable plugin config, tested in the new serialization test module.
src/anonymizer/interface/anonymizer.py Multiple type-safety fixes: _unwrap_entities/_entity_label robustified for NumPy arrays; AnonymizerEvent construction switched to model_validate; assert used for type narrowing of privacy_goal (risky under -O); _ListConvertible protocol duplicated here.
src/anonymizer/engine/ndd/model_loader.py resolve_model_aliases raises TypeError instead of silently accepting non-string aliases; list branch now returns the original list by reference (no defensive copy).
src/anonymizer/engine/detection/postprocess.py _coerce_int and _coerce_float guard with isinstance checks against SupportsInt/SupportsFloat/SupportsIndex before calling int()/float(), removing the type: ignore suppressions.
src/anonymizer/engine/evaluation/judge_base.py Introduces _JudgeAdapter and _JudgeRunResult structural protocols; decouples _BaseJudgeWorkflow from concrete NddAdapter type at the constructor level.
tests/engine/test_detection_config_serialization.py New test file: round-trip and fresh-subprocess tests for native Data Designer config serialization, replacing the deleted distributed.py factory path.
tools/codestyle/typecheck.sh Adds --error-on-warning flag to all ty check invocations, making warnings blocking.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph before["Before (CI)"]
        B1[test job] --> B2[lint job
format-check
copyright-check
lock-check]
        B3[typecheck job
continue-on-error: true
ty check OR true]
    end

    subgraph after["After (CI)"]
        A1[test job] --> A2[check job
format-check
ty check --error-on-warning
lock-check
copyright-check]
    end

    subgraph precommit["Pre-commit hooks (new)"]
        P1[ruff format/lint staged .py] --> P2[copyright-fix SPDX headers]
        P2 --> P3[uv-lock verify lock freshness]
        P3 --> P4[make check
format-check + typecheck + lock-check + copyright-check]
        P4 --> P5[DCO Signed-off-by check]
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    subgraph before["Before (CI)"]
        B1[test job] --> B2[lint job
format-check
copyright-check
lock-check]
        B3[typecheck job
continue-on-error: true
ty check OR true]
    end

    subgraph after["After (CI)"]
        A1[test job] --> A2[check job
format-check
ty check --error-on-warning
lock-check
copyright-check]
    end

    subgraph precommit["Pre-commit hooks (new)"]
        P1[ruff format/lint staged .py] --> P2[copyright-fix SPDX headers]
        P2 --> P3[uv-lock verify lock freshness]
        P3 --> P4[make check
format-check + typecheck + lock-check + copyright-check]
        P4 --> P5[DCO Signed-off-by check]
    end
Loading

Comments Outside Diff (2)

  1. src/anonymizer/interface/anonymizer.py, line 1126-1131 (link)

    P2 _ListConvertible protocol duplicated across five modules

    The identical _ListConvertible protocol and _is_list_convertible guard are copy-pasted into engine/schemas/shared.py, interface/anonymizer.py, interface/display.py, tools/measurement/analyze_benchmark_output.py, and tools/measurement/analyze_detection_artifacts.py. Centralising them in the existing shared.py (or a small _compat.py) would make future adjustments a single change.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. src/anonymizer/engine/ndd/model_loader.py, line 199-214 (link)

    P2 resolve_model_aliases now returns the original list by reference

    Previously the list branch returned list(value) — a defensive copy. The new branch returns value directly, so callers who append to the returned list would mutate the underlying ModelSelection field. The existing callers all build new lists rather than extending the return value, so no concrete breakage today, but a copy (return list(value)) is a cheap safeguard against future mutations.

Reviews (1): Last reviewed commit: "revert: move DCO app migration to a sepa..." | Re-trigger Greptile

Comment on lines +599 to 601
privacy_goal = config.rewrite.privacy_goal
assert privacy_goal is not None # populated by Rewrite's model validator
rewrite_result = self._rewrite_runner.run(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 assert for type narrowing can be stripped at runtime

assert privacy_goal is not None is valid as a type-checker hint, but Python's -O flag removes assert statements, so if the process is ever launched with optimizations the assertion is silently skipped and privacy_goal=None flows into rewrite_runner.run(). Prefer an explicit early-return or a short if ... raise guard that survives optimization mode.

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