-
Notifications
You must be signed in to change notification settings - Fork 32
chore: use python 3.13 in connector builder base image #801
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Updates the manifest server Docker base image to Python 3.13.
- Bumps Python version from 3.12 to 3.13.
- Pins the image to a specific sha256 digest for reproducibility.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@dbgold17/update-manifest-server-to-python-3.13#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch dbgold17/update-manifest-server-to-python-3.13Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughThe changes primarily focus on updating type-checking annotations and dependencies. The Dockerfile base image is upgraded to Python 3.13.9-slim-bookworm, langchain dependency declarations are restructured in pyproject.toml, and type: ignore annotations are added throughout the codebase to suppress typing warnings without modifying functional behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes These are largely consistent pattern changes—adding type-ignore annotations, explicit bool() wrapping, and a straightforward dependency restructuring. The modifications preserve existing behavior while improving type-checker compatibility. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
Comment |
PyTest Results (Full)3 805 tests 3 793 ✅ 10m 52s ⏱️ Results for commit 82b423b. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/connector_builder/connector_builder_handler.py (1)
51-51: Consider explicit bool() casting for consistency with similar code?I noticed that
airbyte_cdk/manifest_server/command_processor/utils.pyhas nearly identical functions (should_migrate_manifestandshould_normalize_manifestat lines 40-57) that use explicitbool()casting instead of type-ignore comments:return bool(manifest.get(SHOULD_MIGRATE_KEY, False))The explicit casting approach is a bit more self-documenting and ensures type safety without suppressing warnings. Would you consider aligning these implementations for consistency across the codebase? wdyt?
Apply this diff to use explicit bool casting:
- return config.get("__should_migrate", False) # type: ignore[no-any-return] + return bool(config.get("__should_migrate", False))- return config.get("__should_normalize", False) # type: ignore[no-any-return] + return bool(config.get("__should_normalize", False))Also applies to: 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
airbyte_cdk/connector_builder/connector_builder_handler.py(2 hunks)airbyte_cdk/manifest_server/command_processor/utils.py(3 hunks)airbyte_cdk/sources/declarative/interpolation/macros.py(2 hunks)airbyte_cdk/sources/file_based/stream/default_file_based_stream.py(1 hunks)airbyte_cdk/sources/streams/checkpoint/checkpoint_reader.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/sources/streams/checkpoint/checkpoint_reader.py
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte_cdk/sources/file_based/stream/default_file_based_stream.py (2)
airbyte_cdk/sources/streams/concurrent/adapters.py (1)
cursor(196-197)airbyte_cdk/sources/streams/concurrent/default_stream.py (1)
cursor(92-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-google-drive
- GitHub Check: Check: destination-motherduck
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/interpolation/macros.py (1)
14-15: Type-ignore annotations look reasonable for untyped dependencies.The
type: ignore[import-untyped]annotations forpytzanddateutil.parserare appropriate since these are external libraries without complete type stubs. The return annotation on line 93 suppresses theno-any-returnwarning fromastimezone(), which is reasonable given the complexity of the datetime handling.Also applies to: 93-93
airbyte_cdk/manifest_server/command_processor/utils.py (1)
47-47: Nice! Explicit bool() casting is a solid approach.The explicit
bool()casting ensures these functions always return proper booleans, even if the config contains truthy/falsy non-bool values. This is clearer and safer than type-ignore comments, as it documents the intention and provides runtime guarantees.Also applies to: 57-57
airbyte_cdk/sources/file_based/stream/default_file_based_stream.py (1)
95-95: Good refinement of the type-ignore annotation.The updated
type: ignore[override]is more specific than the previous generic comment, making it clearer what mypy warning is being suppressed. This helps future maintainers understand the intention.
82b423b to
32a7191
Compare
32a7191 to
c0e506f
Compare
Summary by CodeRabbit
Chores
Internal