fix: DeclarativeExecutor passing down empty configs#918
fix: DeclarativeExecutor passing down empty configs#918aaronsteers wants to merge 5 commits intomainfrom
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Change config parameter default from {} to None in DeclarativeExecutor.__init__
- Change config parameter default from {} to None in get_connector_executor
- Use config.copy() to avoid mutating caller's dict
- Add missing Any import to util.py
This fixes the B006 lint error (mutable default argument) which could cause
shared state bugs across multiple calls.
Co-Authored-By: AJ Steers <aj@airbyte.io>
Original prompt from AJ Steers |
🤖 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. Testing 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/fix-pr-869-mutable-defaults' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/fix-pr-869-mutable-defaults'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughThe changes propagate a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ 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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte/_executors/declarative.py (1)
44-51: Config parameter correctly added to signature.The new
configparameter with typedict[str, Any] | None = Nonefollows Python best practices by usingNoneas the default instead of a mutable default argument. This allows the executor to receive user-provided configuration at initialization time, resolving issue #868.One minor observation: the docstring (lines 52-59) doesn't explicitly document the new
configparameter—wdyt about adding a brief note?If you'd like to document it, consider:
"""Initialize a declarative executor. + Args: + name: Connector name. + manifest: Declarative manifest (dict or Path to YAML file). + config: Optional user-provided configuration dict. + components_py: Optional custom components.py content or path. + components_py_checksum: Optional checksum for components_py validation. + - If `manifest` is a path, it will be read as a json file.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte/_executors/declarative.py(2 hunks)airbyte/_executors/util.py(4 hunks)airbyte/sources/util.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 415
File: airbyte/cli.py:0-0
Timestamp: 2024-10-09T21:11:11.706Z
Learning: In `_resolve_source_job`, returning `None` when `config` is falsy distinguishes between empty config and not-yet-provided config, allowing `_resolve_config()` to handle non-null inputs effectively.
⏰ 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). (6)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (6)
airbyte/_executors/declarative.py (1)
70-70: Excellent use of config.copy() to prevent mutation.Creating a copy of the provided config ensures that subsequent modifications (like injecting components at lines 78-81) don't mutate the caller's dictionary. Using a shallow copy is appropriate here since only top-level keys are added to
config_dict.airbyte/_executors/util.py (4)
8-8: Import addition looks good.The
Anyimport is necessary for theconfig: dict[str, Any] | Nonetype hint and is correctly placed in alphabetical order.
172-172: Config parameter correctly added to factory function.The new parameter properly extends
get_connector_executor()to accept configuration, with the correct type hint and safe default value. The parameter is positioned at the end of the signature, which is a reasonable choice given the function already has many parameters.
335-340: Config correctly passed to DeclarativeExecutor (first instantiation site).When
source_manifestis a dict or Path, the config is now properly threaded to the DeclarativeExecutor, ensuring user-provided configuration is available to the declarative source.
351-357: Config correctly passed to DeclarativeExecutor (second instantiation site).When the manifest is fetched from a URL, the config is properly propagated to the DeclarativeExecutor. Both instantiation sites (lines 335-340 and 351-357) now consistently receive the config parameter, ensuring declarative sources always have access to user-provided configuration.
airbyte/sources/util.py (1)
131-131: Config parameter correctly threaded to executor factory.The config is now properly passed from
get_source()toget_connector_executor(), ensuring all DeclarativeExecutor instantiation sites receive the user-provided configuration. Both manifest-based execution paths (Path and URL-based) pass config through correctly.
|
is this going to be merged soon? i have the exact same issue |
3 participants
Summary
Fixes #868 - Declarative YAML connectors were not receiving user config, causing template expressions like
{{ config['hello'] }}to resolve to empty values.This PR threads the
configparameter fromget_source()throughget_connector_executor()toDeclarativeExecutor, so declarative manifests can access user-provided configuration.Key changes:
configparameter toDeclarativeExecutor.__init__()configparameter toget_connector_executor()factoryconfigfromget_source()through the call chainconfig.copy() if config is not None else {}to avoid mutable default argument issues (B006 lint error)This supersedes PR #869 from @adileo, incorporating their fix plus addressing the mutable default argument issues flagged by CodeRabbit.
Review & Testing Checklist for Human
source_manifest=Trueor a custom manifest dict, pass a config with values, and verify the connector receives the config (e.g., template{{ config['api_key'] }}resolves correctly).copy()call should prevent the executor from mutating the original config dict passed by the callerutil.py(lines 335 and 351) are the only places that need updatingNotes
_config_dictwas initialized as empty{}and never received the user's configSummary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.