-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: main
Are you sure you want to change the base?
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.13 Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughUpdated manifest server Docker base image to a SHA‑pinned Python 3.13 slim image and upgraded langchain-related dependencies in pyproject.toml; several files received only typing-suppression/comment edits or a minor client Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Add maxi297 as a reviewer too, wdyt? Pre-merge checks and finishing touches✅ Passed checks (3 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.py
has nearly identical functions (should_migrate_manifest
andshould_normalize_manifest
at 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.lock
is 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 forpytz
anddateutil.parser
are appropriate since these are external libraries without complete type stubs. The return annotation on line 93 suppresses theno-any-return
warning 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.
Summary by CodeRabbit
Chores
New Features