Fix DeclarativeExecutor passing down empty configs #869
Conversation
👋 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@main' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@main'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughThreads a new optional Changes
Sequence DiagramsequenceDiagram
participant Source as get_source()
participant Executor as get_connector_executor()
participant Declarative as DeclarativeExecutor.__init__()
Source->>Executor: call with config
Note right of Executor: signature now accepts config
Executor->>Declarative: instantiate with config=config
Note right of Declarative: config_dict ← config\nthen inject components/checksums
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte/_executors/declarative.py(2 hunks)airbyte/_executors/util.py(3 hunks)airbyte/sources/util.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run Linters
airbyte/_executors/util.py
[error] 169-169: Ruff would reformat: add trailing comma to 'config' default value in function signature. Run 'ruff format' to automatically fix formatting.
[warning] 1-1: The top-level linter settings are deprecated in favour of their counterparts in the 'lint' section. Please update the following options in .ruff.toml: 'select' -> 'lint.select'.
airbyte/sources/util.py
[error] 128-128: Ruff would reformat: add trailing comma to 'config' argument in function call. Run 'ruff format' to automatically fix formatting.
airbyte/_executors/declarative.py
[error] 48-48: B006 Do not use mutable data structures for argument defaults. Replace with None; initialize within function.
⏰ 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). (2)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (2)
airbyte/_executors/util.py (1)
338-338: LGTM! Config propagation looks good.The config parameter is correctly passed to
DeclarativeExecutorin both code paths (Path/dict and URL/bool cases). This ensures the executor receives the configuration in all scenarios.Also applies to: 354-354
airbyte/_executors/declarative.py (1)
70-70: Good fix! Consider whether to copy the config dict.This change correctly initializes
config_dictfrom the passedconfigparameter instead of always starting from an empty dict, which fixes the issue. However, since lines 78-81 mutateconfig_dictby adding__injected_components_pykeys, the caller's original config dict will be modified if they pass one in.Is this mutation intentional, or should we use
config.copy()to avoid side effects on the caller's dict? Wdyt?
| self, | ||
| name: str, | ||
| manifest: dict | Path, | ||
| config: dict[str, Any] = {}, |
There was a problem hiding this comment.
Critical: Replace mutable default argument with None.
The linter is correctly flagging B006 here. Using {} as a default is particularly problematic in this case because config_dict is mutated on lines 78-81 when components_py is provided. This means the default dict would accumulate injected components across multiple calls, causing unpredictable behavior. Wdyt about switching to None?
- config: dict[str, Any] = {},
+ config: dict[str, Any] | None = None,Then update line 70:
- config_dict: dict[str, Any] = config
+ config_dict: dict[str, Any] = config if config is not None else {}Or alternatively at line 70:
- config_dict: dict[str, Any] = config
+ config_dict: dict[str, Any] = config.copy() if config is not None else {}(Using .copy() would be safer to avoid mutating the caller's dict, though it depends on whether mutation is intended.)
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Run Linters
[error] 48-48: B006 Do not use mutable data structures for argument defaults. Replace with None; initialize within function.
🤖 Prompt for AI Agents
In airbyte/_executors/declarative.py around lines 48 to 48, replace the mutable
default dict in the function signature with None (use config: Optional[dict[str,
Any]] = None and import typing if needed), then at the top of the function
(before lines where config_dict is mutated around 78-81) initialize a fresh dict
and avoid mutating the caller: if config is None: config = {} else: config =
config.copy(); this prevents shared-state bugs and preserves caller data while
allowing local mutations when components_py is provided.
| install_root: Path | None = None, | ||
| use_python: bool | Path | str | None = None, | ||
| no_executor: bool = False, | ||
| config: dict[str, Any] = {} |
There was a problem hiding this comment.
Replace mutable default argument with None.
Using a mutable default argument ({}) is a common Python pitfall - the same dict instance is shared across all calls that don't provide config, which can lead to unintended state sharing if modified. The linter (B006) is correctly flagging this. Wdyt about using None as the default instead?
- config: dict[str, Any] = {}
+ config: dict[str, Any] | None = None,Then at the beginning of the function body (after the docstring), add:
if config is None:
config = {}🤖 Prompt for AI Agents
In airbyte/_executors/util.py around line 172, the function parameter uses a
mutable default config: dict[str, Any] = {}, which causes shared state across
calls; change the default to None and at the top of the function body (after the
docstring) add a guard that sets config = {} when config is None to avoid shared
mutable defaults.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Fixing proposal for: #868
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.