feat(lint): Upgrade ty - #543
Conversation
7d134ed to
55d16ef
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates Changesty Upgrade and CI Linting
SDK Resilience and Tests
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The ty upgrade from 0.0.17 to 0.0.56 catches additional pre-existing type violations. Add these new error categories to the CI ignore list with counts, matching the existing pattern for incremental cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
ty 0.0.56 exits non-zero on warnings (unsupported-base, unused-type-ignore-comment, redundant-cast). Add --exit-zero-on-warning so only errors fail CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/lint/lint-python-types.sh (1)
38-38: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
--exit-zero-on-warningsilences all warnings, not just the three cited.Per ty docs, this flag makes ty "Use exit code 0 if there are no error-level diagnostics" — i.e. it suppresses CI failure for any warning-level rule, not only
unsupported-base,unused-type-ignore-comment,redundant-castmentioned in the commit message. Future warning-level regressions (in rules not inci_ignored_rules) will silently pass CI too. Consider--ignorefor just those three rules (or--warndemotion) instead of blanket--exit-zero-on-warning, to keep other warnings failing the build.[reliability_and_availability]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/lint/lint-python-types.sh` at line 38, The ty invocation in lint-python-types.sh is using a blanket warning suppression flag that lets every warning-level diagnostic pass CI, not just the three intended rules. Update the lint command around uv run and ty check to scope suppression only to unsupported-base, unused-type-ignore-comment, and redundant-cast, using ty’s per-rule ignore or warn handling instead of --exit-zero-on-warning, so other warning-level regressions still fail the build.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tools/lint/lint-python-types.sh`:
- Line 38: The ty invocation in lint-python-types.sh is using a blanket warning
suppression flag that lets every warning-level diagnostic pass CI, not just the
three intended rules. Update the lint command around uv run and ty check to
scope suppression only to unsupported-base, unused-type-ignore-comment, and
redundant-cast, using ty’s per-rule ignore or warn handling instead of
--exit-zero-on-warning, so other warning-level regressions still fail the build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 476ff565-c22e-4f0b-b895-9fd6b9a6fe60
📒 Files selected for processing (1)
tools/lint/lint-python-types.sh
Instead of adding blanket suppressions for new error categories, fix each violation individually: - Fix with_logging decorator in anonymizer/data-designer to preserve class types through TypeVar (invalid-type-form) - Fix StudioService.dependencies to use ClassVar (invalid-attribute-override) - Add inline ty: ignore comments for false positives and intentional test misuse (invalid-paramspec, call-non-callable, unknown-argument, missing-argument, empty-body, too-many-positional-arguments, invalid-yield, unresolved-import, invalid-type-form) No new blanket suppressions added to lint-python-types.sh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Replace `# type: ignore[rule] # ty: ignore[rule]` with blanket `# type: ignore` on lines that need both mypy and ty suppression. Blanket `# type: ignore` is respected by both checkers, eliminating the need for separate directives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
This reverts commit cd7b6f6. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
This repo uses ty, not mypy. Replace redundant dual comments (# type: ignore[rule] # ty: ignore[rule]) with just # ty: ignore[rule] on the 14 lines we touched for the ty upgrade. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Run make vendor and make update-cli to sync the evaluator SDK vendored files with the ty: ignore comment changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
* squash Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(lint): suppress new ty 0.0.56 error categories in CI The ty upgrade from 0.0.17 to 0.0.56 catches additional pre-existing type violations. Add these new error categories to the CI ignore list with counts, matching the existing pattern for incremental cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(lint): add --exit-zero-on-warning for ty 0.0.56 ty 0.0.56 exits non-zero on warnings (unsupported-base, unused-type-ignore-comment, redundant-cast). Add --exit-zero-on-warning so only errors fail CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(lint): resolve all new ty 0.0.56 errors at source Instead of adding blanket suppressions for new error categories, fix each violation individually: - Fix with_logging decorator in anonymizer/data-designer to preserve class types through TypeVar (invalid-type-form) - Fix StudioService.dependencies to use ClassVar (invalid-attribute-override) - Add inline ty: ignore comments for false positives and intentional test misuse (invalid-paramspec, call-non-callable, unknown-argument, missing-argument, empty-body, too-many-positional-arguments, invalid-yield, unresolved-import, invalid-type-form) No new blanket suppressions added to lint-python-types.sh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(lint): consolidate duplicate type ignore comments Replace `# type: ignore[rule] # ty: ignore[rule]` with blanket `# type: ignore` on lines that need both mypy and ty suppression. Blanket `# type: ignore` is respected by both checkers, eliminating the need for separate directives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * Revert "fix(lint): consolidate duplicate type ignore comments" This reverts commit cd7b6f6. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(lint): use ty: ignore instead of type: ignore This repo uses ty, not mypy. Replace redundant dual comments (# type: ignore[rule] # ty: ignore[rule]) with just # ty: ignore[rule] on the 14 lines we touched for the ty upgrade. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * chore: sync vendored SDK and CLI after ty ignore cleanup Run make vendor and make update-cli to sync the evaluator SDK vendored files with the ty: ignore comment changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> --------- Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Matthew Grossman mgrossman@nvidia.com
This includes astral-sh/ruff#26479, which helps my nemoclient efforts greatly!
Summary by CodeRabbit
Summary by CodeRabbit