build(typing): migrate from pyrefly to ty for type checking#1054
build(typing): migrate from pyrefly to ty for type checking#1054Aaron ("AJ") Steers (aaronsteers) wants to merge 4 commits into
Conversation
Co-Authored-By: AJ Steers <aj@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1783307935-migrate-to-ty' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1783307935-migrate-to-ty'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| "faker>=21.0.0,<22.0", | ||
| "freezegun>=1.4.0,<2.0", | ||
| "pyrefly>=0.38.0,<0.39.0", | ||
| "ty>=0.0.56", |
There was a problem hiding this comment.
🔴 Type-checker lint test will always fail because the tool it invokes was removed from dependencies
The type-checker dependency is replaced ("ty>=0.0.56" at pyproject.toml:62) without updating the test that invokes the old tool (tests/lint_tests/test_mypy.py:13), so running pytest tests/lint_tests/ will always fail with a missing-command error.
Impact: The lint test suite is broken and will fail in CI or local runs.
Incomplete pyrefly-to-ty migration leaves test calling removed tool
The PR removes pyrefly from dev dependencies and replaces it with ty everywhere in the source code and CI workflow. However, tests/lint_tests/test_mypy.py:12-13 still runs:
check_result = subprocess.run(
["uv", "run", "pyrefly", "check"],
...
)Since pyrefly is no longer in the dependency group, uv run pyrefly check will fail (command not found), causing the test assertion at tests/lint_tests/test_mypy.py:19 to always fail.
The fix should update this test to call ["uv", "run", "ty", "check"] and update the error messages accordingly.
Prompt for agents
The PR replaces pyrefly with ty in pyproject.toml dev dependencies, but tests/lint_tests/test_mypy.py still invokes `uv run pyrefly check` on line 13. This test will always fail now because pyrefly is no longer installed. The file needs to be updated to call `uv run ty check` instead, and the error messages on lines 18-22 should reference ty rather than pyrefly. The test function name (test_mypy_typing) is also misleading and could be renamed to something like test_ty_typing or test_type_checking.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 417968f — updated the test to call uv run ty check and renamed the function to test_type_checking.
| [rules] | ||
| # Pre-existing issues inherited from pyrefly migration. | ||
| # Demoted to warn so CI passes; fix incrementally. | ||
| invalid-argument-type = "warn" | ||
| unknown-argument = "warn" | ||
| missing-argument = "warn" | ||
| unresolved-attribute = "warn" | ||
| invalid-assignment = "warn" | ||
| call-non-callable = "warn" | ||
| invalid-return-type = "warn" | ||
| invalid-method-override = "warn" | ||
| no-matching-overload = "warn" | ||
|
|
||
| [terminal] | ||
| error-on-warning = false |
There was a problem hiding this comment.
🔍 Type checker configured to never block CI on type errors
The ty.toml configuration at lines 19-33 demotes 9 rule categories to "warn" and sets error-on-warning = false. This means the CI step (uv run ty check) will always pass regardless of type errors in those categories. While the comment explains this is intentional for incremental migration, it effectively means the type-checking CI gate provides no enforcement for the most common type errors (invalid-argument-type, unresolved-attribute, invalid-return-type, etc.). The inline # ty: ignore[...] comments throughout the codebase are also redundant since those rules are already demoted to non-blocking warnings.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional for the migration — the inline # ty: ignore[...] comments aren't redundant because they'll be needed when these rules are promoted back to "error" level during incremental cleanup. Keeping them now avoids a second pass through the codebase later.
Co-Authored-By: AJ Steers <aj@airbyte.io>
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR switches the project’s type-checking setup from pyrefly to ty. It adds ty configuration, updates CI and project tooling to run ty, and changes inline suppression comments across source files to ty-specific codes. ChangesType checker migration (pyrefly → ty)
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ty.toml (2)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant exclude entries?
Lines 10-11 (
tests/integration_tests/fixtures/source-broken/**,tests/integration_tests/fixtures/source-test/**) are already subsumed by the blankettests/**exclude on line 13. Was this intentional (e.g., ordering/precedence reasons), or can these be dropped for clarity, wdyt?🤖 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 `@ty.toml` around lines 9 - 14, The exclude list in ty.toml has redundant entries because tests/integration_tests/fixtures/source-broken/** and tests/integration_tests/fixtures/source-test/** are already covered by tests/**. Remove the narrower fixture paths from the exclude array unless there is a specific precedence or override reason, and keep only the distinct exclusions needed for clarity in the ty config.
17-17: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueVery broad unresolved-import suppression.
allowed-unresolved-imports = ["**"]suppresses unresolved-import diagnostics for every module project-wide. This appears intended to preserve pyrefly's prior behavior per the PR objective, but it does mean ty won't catch genuinely missing/misspelled imports going forward. Worth scoping this down in a follow-up once the migration stabilizes, wdyt?🤖 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 `@ty.toml` at line 17, The ty configuration currently suppresses unresolved-import diagnostics for every import via the all-matching allowed-unresolved-imports setting. Update the ty.toml setting to scope this exception more narrowly, using a targeted pattern or list of known cases instead of the global catch-all, so ty can still report real missing or misspelled imports. Focus the change on the allowed-unresolved-imports entry in the ty configuration.
🤖 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 `@ty.toml`:
- Around line 9-14: The exclude list in ty.toml has redundant entries because
tests/integration_tests/fixtures/source-broken/** and
tests/integration_tests/fixtures/source-test/** are already covered by tests/**.
Remove the narrower fixture paths from the exclude array unless there is a
specific precedence or override reason, and keep only the distinct exclusions
needed for clarity in the ty config.
- Line 17: The ty configuration currently suppresses unresolved-import
diagnostics for every import via the all-matching allowed-unresolved-imports
setting. Update the ty.toml setting to scope this exception more narrowly, using
a targeted pattern or list of known cases instead of the global catch-all, so ty
can still report real missing or misspelled imports. Focus the change on the
allowed-unresolved-imports entry in the ty configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 495e3cc8-8234-42f6-8bdc-146050dd1069
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.github/workflows/python_lint.ymlairbyte/_connector_base.pyairbyte/_message_iterators.pyairbyte/_processors/sql/postgres.pyairbyte/_processors/sql/snowflake.pyairbyte/_util/api_util.pyairbyte/_util/meta.pyairbyte/_util/name_normalizers.pyairbyte/caches/_utils/_cache_to_dest.pyairbyte/caches/base.pyairbyte/caches/motherduck.pyairbyte/cli/pyab.pyairbyte/cloud/sync_results.pyairbyte/datasets/_sql.pyairbyte/mcp/local.pyairbyte/progress.pyairbyte/secrets/google_gsm.pyairbyte/shared/catalog_providers.pyairbyte/shared/sql_processor.pyairbyte/shared/state_providers.pyairbyte/sources/base.pyairbyte/types.pypyproject.tomlpyrefly.tomlty.toml
💤 Files with no reviewable changes (1)
- pyrefly.toml
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in the branch is 67%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in the branch is 67%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in the branch is 72%. The coverage in the branch is 71%. Show a code coverage summary of the most impacted files.
Updated |
…omments Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
|
|
||
| if output_file: | ||
| output_file.write_text(content) # pyrefly: ignore[unbound-name] | ||
| output_file.write_text(content) |
| return | ||
|
|
||
| syntax_highlighted = Syntax(content, format) # pyrefly: ignore[unbound-name] | ||
| syntax_highlighted = Syntax(content, format) |
2 participants
Summary
Migrates the type checker from pyrefly to ty (Astral's renamed/successor project).
Config:
pyrefly.toml→ty.tomlwith equivalent settings mapped to ty's schema:project-includes→[src] includeproject-excludes→[src] excludeignore-missing-imports = ["*"]→[analysis] allowed-unresolved-imports = ["**"]Ignore comments: All 42
# pyrefly: ignore[rule]comments converted to# ty: ignore[rule]with mapped rule names (e.g.bad-return→invalid-return-type,missing-attribute→unresolved-attribute,unexpected-keyword→unknown-argument).Pre-existing diagnostics: ty finds ~209 warnings that pyrefly didn't flag. These are demoted to
warnseverity inty.tomlwitherror-on-warning = falseso CI passes. They can be fixed incrementally.Dev dep:
pyrefly>=0.38.0,<0.39.0→ty>=0.0.56CI: Workflow step renamed from "Run Pyrefly Check" → "Run ty Check" (
uv run ty check).Poe task:
checktask updated frompyrefly check→ty check.Link to Devin session: https://app.devin.ai/sessions/7243ddf054a44c07b34b69d1cc20da45
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit