Skip to content
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

Feat: Adds config_change_callback to Destinations and Sources #440

Merged
merged 11 commits into from
Nov 8, 2024

Conversation

tcboles
Copy link
Contributor

@tcboles tcboles commented Nov 4, 2024

Summary by CodeRabbit

  • New Features

    • Introduced an optional config_change_callback parameter in the ConnectorBase, Destination, and Source classes, enabling dynamic handling of configuration changes.
    • Enhanced the get_destination and get_source functions with the config_change_callback parameter to support configuration changes during setup.
    • Deprecated the get_connector function, encouraging users to transition to get_source.
  • Bug Fixes

    • Improved error handling related to configuration change messages without affecting existing log and trace message handling.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

vercel bot commented Nov 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
pyairbyte-api-docs ❌ Failed (Inspect) Nov 5, 2024 5:56pm

@tcboles tcboles marked this pull request as draft November 4, 2024 19:37
Copy link

coderabbitai bot commented Nov 4, 2024

Warning

Rate limit exceeded

@tcboles has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 957f7eb and 5567cd2.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes introduce a new optional parameter, config_change_callback, to the __init__ methods of the ConnectorBase, Destination, and Source classes in the Airbyte codebase. This callback allows for dynamic handling of configuration changes by invoking a specified callable when configuration change messages are received. The modifications include adjustments to the _peek_airbyte_message method to process these messages, while existing error handling remains unchanged. Overall, the control flow is enhanced to accommodate the new callback mechanism.

Changes

File Change Summary
airbyte/_connector_base.py Added config_change_callback parameter to ConnectorBase.__init__. Modified _peek_airbyte_message to invoke the callback on configuration change messages.
airbyte/destinations/base.py Added config_change_callback parameter to Destination.__init__, passing it to the superclass constructor.
airbyte/sources/base.py Added config_change_callback parameter to Source.__init__, also passing it to the superclass constructor.
airbyte/destinations/util.py Added config_change_callback parameter to get_destination, passed to Destination constructor.
airbyte/sources/util.py Added config_change_callback to get_source, deprecated get_connector, which now redirects to get_source.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (4)
airbyte/_connector_base.py (2)

59-67: Would you mind updating the docstring to include the new parameter? 🤔

The __init__ docstring should document the config_change_callback parameter. Here's a suggestion, wdyt?

     def __init__(
         self,
         executor: Executor,
         name: str,
         config: dict[str, Any] | None = None,
         config_change_callback: Callable[[dict[str, Any], int], None] | None = None,
         *,
         validate: bool = False,
     ) -> None:
         """Initialize the source.
 
         If config is provided, it will be validated against the spec if validate is True.
+
+        Args:
+            config_change_callback: Optional callback that will be invoked when configuration
+                change messages are received. The callback receives the new configuration
+                and the timestamp of the message.
         """
🧰 Tools
🪛 Ruff

59-59: Undefined name Callable

(F821)


Line range hint 6-6: Shall we add the missing Callable import? 🔍

The Callable type is used but not imported. Since we have a TYPE_CHECKING block, let's add it there:

 if TYPE_CHECKING:
     import logging
-    from collections.abc import Generator
+    from collections.abc import Callable, Generator
     from typing import IO
🧰 Tools
🪛 Ruff

59-59: Undefined name Callable

(F821)

airbyte/sources/base.py (2)

61-61: Consider enhancing the docstring with config_change_callback details?

The new parameter could benefit from documentation explaining its purpose and usage. Here's a suggestion, wdyt?

     def __init__(
         self,
         executor: Executor,
         name: str,
         config: dict[str, Any] | None = None,
         config_change_callback: Callable[[dict[str, Any]], None] | None = None,
         streams: str | list[str] | None = None,
         *,
         validate: bool = False,
     ) -> None:
         """Initialize the source.
 
         If config is provided, it will be validated against the spec if validate is True.
+
+        Args:
+            executor: The executor to use for running the connector.
+            name: The name of the connector.
+            config: Optional configuration dictionary.
+            config_change_callback: Optional callback function that will be called when
+                configuration changes are received. The callback takes a dictionary of
+                the new configuration as its argument.
+            streams: Optional stream names to select.
+            validate: If True, validate the config against the spec.
+
+        Example:
+            ```python
+            def on_config_change(new_config: dict[str, Any]) -> None:
+                print(f"Config updated: {new_config}")
+
+            source = Source(
+                executor=executor,
+                name="my-source",
+                config_change_callback=on_config_change
+            )
+            ```
         """

Also applies to: 77-77

🧰 Tools
🪛 Ruff

61-61: Undefined name Callable

(F821)


Line range hint 4-4: Add missing import for Callable?

The static analysis tool caught that Callable is undefined. Should we add it to the typing imports, wdyt?

 from __future__ import annotations
 
 import json
 import warnings
 from pathlib import Path
-from typing import TYPE_CHECKING, Any, Literal
+from typing import TYPE_CHECKING, Any, Callable, Literal
🧰 Tools
🪛 Ruff

61-61: Undefined name Callable

(F821)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7c703ac and abe154e.

📒 Files selected for processing (3)
  • airbyte/_connector_base.py (3 hunks)
  • airbyte/destinations/base.py (2 hunks)
  • airbyte/sources/base.py (2 hunks)
🧰 Additional context used
🪛 Ruff
airbyte/_connector_base.py

59-59: Undefined name Callable

(F821)

airbyte/destinations/base.py

51-51: Undefined name Callable

(F821)

airbyte/sources/base.py

61-61: Undefined name Callable

(F821)

🔇 Additional comments (4)
airbyte/destinations/base.py (3)

51-51: LGTM: Well-typed callback parameter addition

The addition of the optional config_change_callback parameter with proper type hints looks good! This maintains backward compatibility while enabling dynamic config change handling.

🧰 Tools
🪛 Ruff

51-51: Undefined name Callable

(F821)


63-63: LGTM: Proper callback propagation to base class

The config_change_callback is correctly passed to the ConnectorBase constructor, maintaining the expected inheritance chain.


Line range hint 51-63: Verify integration with existing configuration handlers

Hey! The changes look solid, but would you mind if we double-check how this integrates with existing config handlers? Let's verify there are no unexpected interactions, wdyt?

✅ Verification successful

Let me analyze the implementation details to ensure consistent config handling patterns across these files.


Config handling implementation looks consistent across the codebase

The changes align well with the existing config handling patterns! I can see that:

  • The base connector class in _connector_base.py defines the core config change callback functionality
  • Both source and destination implementations consistently inherit and pass through the config change callback
  • The callback signature in destinations matches the parent implementation

The integration looks solid and maintains the expected behavior. No issues to worry about! 🎯

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing config handling patterns
echo "Checking for existing config handling patterns..."
rg -l "config.*change" --type py

# Look for similar callback patterns
echo "Checking for similar callback patterns..."
rg -l "callback.*config" --type py

Length of output: 433


Script:

#!/bin/bash
# Let's examine the implementation details of config handling in these files
echo "Checking config handling implementation in _connector_base.py..."
rg "config.*change" airbyte/_connector_base.py -B 2 -A 2

echo "\nChecking config handling in sources/base.py..."
rg "config.*change" airbyte/sources/base.py -B 2 -A 2

echo "\nChecking config handling in destinations/base.py..."
rg "config.*change" airbyte/destinations/base.py -B 2 -A 2

# Let's also check for any inheritance patterns
echo "\nChecking class inheritance patterns..."
ast-grep --pattern 'class $_ extends $_:
    $$$'

Length of output: 2443

🧰 Tools
🪛 Ruff

51-51: Undefined name Callable

(F821)

airbyte/_connector_base.py (1)

386-394: LGTM! Clean implementation of the config change callback handling 👍

The implementation follows the existing pattern and properly handles the config change messages. The null check ensures safety when no callback is provided.

airbyte/destinations/base.py Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (3)
airbyte/destinations/util.py (1)

Line range hint 33-61: Consider updating the docstring

The docstring is missing documentation for the new config_change_callback parameter. Would you like me to suggest the documentation? Here's what I'm thinking:

    Args:
        name: connector name
        config: connector config - if not provided, you need to set it later via the set_config
            method.
+       config_change_callback: Optional callback function that will be invoked when configuration
+           changes are received. The callback takes a dictionary containing the new configuration
+           and an integer parameter, and returns None.
        streams: list of stream names to select for reading. If set to "*", all streams will be

wdyt? 🤔

airbyte/sources/util.py (1)

Line range hint 47-119: Update docstring to document config_change_callback parameter

The implementation looks solid, but we should add documentation for the new parameter. Would you mind adding it to the Args section? Something like:

    Args:
        name: connector name
        config: connector config - if not provided, you need to set it later via the set_config
            method.
+       config_change_callback: Optional callback function that will be invoked when configuration
+           changes are received. The callback takes a dictionary containing the new configuration
+           and an integer parameter.
        streams: list of stream names to select for reading. If set to "*", all streams will be
airbyte/destinations/base.py (1)

Line range hint 51-65: Add documentation for the new parameter

The new config_change_callback parameter needs to be documented in the docstring. Would you like me to suggest a docstring update that explains its purpose and usage? Something like:

     def __init__(
         self,
         executor: Executor,
         name: str,
         config: dict[str, Any] | None = None,
         config_change_callback: Callable[[dict[str, Any], int], None] | None = None,
         *,
         validate: bool = False,
     ) -> None:
-        """Initialize the source.
+        """Initialize the source.
+
+        Args:
+            executor: The executor to use for running the connector.
+            name: The name of the connector.
+            config: The configuration for the connector.
+            config_change_callback: Optional callback function that will be called when
+                configuration changes are received. The callback receives the updated
+                config dict and a version number.
+            validate: Whether to validate the config against the spec.
+        """

         If config is provided, it will be validated against the spec if validate is True.
         """

Wdyt? 🤔

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between abe154e and 5650728.

📒 Files selected for processing (4)
  • airbyte/destinations/base.py (2 hunks)
  • airbyte/destinations/util.py (2 hunks)
  • airbyte/sources/base.py (2 hunks)
  • airbyte/sources/util.py (2 hunks)
🧰 Additional context used
🪛 Ruff
airbyte/destinations/base.py

51-51: Undefined name Callable

(F821)

airbyte/destinations/util.py

22-22: Undefined name Callable

(F821)

airbyte/sources/base.py

61-61: Undefined name Callable

(F821)

airbyte/sources/util.py

48-48: Undefined name Callable

(F821)

🔇 Additional comments (7)
airbyte/destinations/util.py (2)

22-22: LGTM: Clean signature addition

The new config_change_callback parameter is well-structured with proper type hints and follows Python best practices. Nice work! 👍

🧰 Tools
🪛 Ruff

22-22: Undefined name Callable

(F821)


62-62: Verify consistent callback usage

Let's make sure the callback parameter is being used consistently across other connector implementations.

✅ Verification successful

Callback parameter is used consistently across the codebase

The config_change_callback parameter is consistently implemented:

  • It's defined in the base ConnectorBase class in _connector_base.py
  • Both Source and Destination classes properly inherit and pass through this parameter
  • The callback is uniformly typed as Callable[[dict[str, Any], int], None] | None
  • It's consistently used with CONTROL messages for config updates
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other implementations to verify consistent callback usage
echo "Searching for Destination constructor calls:"
rg "Destination\(" -A 3

echo -e "\nSearching for config_change_callback parameter usage:"
rg "config_change_callback" -B 2 -A 2

Length of output: 4867

airbyte/sources/util.py (2)

Line range hint 19-42: LGTM: Well-implemented deprecation

The deprecation of get_connector is implemented cleanly with a proper warning message and seamless forwarding to get_source. Nice work! 👍

🧰 Tools
🪛 Ruff

45-45: Missing argument description in the docstring for get_source: config_change_callback

(D417)


48-48: Undefined name Callable

(F821)


107-107: Verify alignment with ConnectorBase implementation

The callback forwarding looks good! Just to be thorough, shall we verify that this matches the implementation in ConnectorBase?

✅ Verification successful

Perfect alignment with ConnectorBase confirmed! 🎯

The implementation in airbyte/sources/util.py perfectly matches the config_change_callback pattern used in ConnectorBase and across the codebase. The callback:

  • Has the same type signature Callable[[dict[str, Any], int], None] | None
  • Is properly forwarded through the class hierarchy
  • Is consistently used in both source and destination implementations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check ConnectorBase implementation of config_change_callback
echo "Searching for config_change_callback usage in ConnectorBase:"
ast-grep --pattern 'class ConnectorBase {
  $$$
  def __init__($$$, config_change_callback: $_) {
    $$$
  }
  $$$
}'

Length of output: 251


Script:

#!/bin/bash
# Let's try a broader search for config_change_callback in Python files
echo "Searching for config_change_callback in Python files:"
rg "config_change_callback" -t python -A 2

# Also search for ConnectorBase class definition
echo -e "\nSearching for ConnectorBase class definition:"
rg "class ConnectorBase" -t python -A 5

Length of output: 370


Script:

#!/bin/bash
# Let's try searching without file type restriction
echo "Searching for config_change_callback usage:"
rg "config_change_callback" -A 2

echo -e "\nSearching for ConnectorBase class definition:"
rg "class ConnectorBase" -A 5

# Also search for files that might contain these
echo -e "\nFinding relevant Python files:"
fd -e py

Length of output: 8280

airbyte/destinations/base.py (2)

51-51: Add missing import for Callable

The existing comment about adding the Callable import is still valid.

🧰 Tools
🪛 Ruff

51-51: Undefined name Callable

(F821)


63-63: Verify integration with ConnectorBase

The callback is passed to ConnectorBase.__init__, but let's verify the integration.

✅ Verification successful

Let me analyze how the callback is implemented and used across these files to ensure proper integration.


Integration looks good! The callback is properly wired up across the codebase 🎯

I've verified that:

  • The callback is correctly passed through from destinations/base.py to ConnectorBase
  • ConnectorBase properly stores and uses the callback in _peek_airbyte_message to handle configuration changes
  • The implementation is consistent across both sources and destinations
  • Type hints are properly maintained throughout the chain

The integration follows the same pattern in both source and destination implementations, which is a nice touch for consistency!

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check how ConnectorBase handles the config_change_callback

# Look for the implementation in ConnectorBase
echo "Checking ConnectorBase implementation:"
ast-grep --pattern 'class ConnectorBase:
  $$$
  def _peek_airbyte_message($$$):
    $$$
'

# Look for any other potential callback invocations
echo "Checking for other callback usages:"
rg -l "config_change_callback" --type py

Length of output: 434


Script:

#!/bin/bash
# Let's check the implementation details in these files

echo "Checking _connector_base.py implementation:"
rg -A 10 "config_change_callback" airbyte/_connector_base.py

echo "\nChecking source and destination implementations:"
rg -A 5 "config_change_callback" airbyte/sources/base.py airbyte/destinations/base.py

echo "\nChecking utility implementations:"
rg -A 5 "config_change_callback" airbyte/sources/util.py airbyte/destinations/util.py

Length of output: 4767

airbyte/sources/base.py (1)

61-61: LGTM! Clean implementation of the config change callback

The implementation looks solid! I like how you've:

  • Made it optional with a None default
  • Added proper type hints
  • Correctly passed it to the superclass

Also applies to: 77-77

🧰 Tools
🪛 Ruff

61-61: Undefined name Callable

(F821)

airbyte/destinations/util.py Outdated Show resolved Hide resolved
airbyte/sources/util.py Outdated Show resolved Hide resolved
airbyte/sources/base.py Outdated Show resolved Hide resolved
@aaronsteers
Copy link
Contributor

aaronsteers commented Nov 5, 2024

/fix-pr

Auto-Fix Job Info

This job attempts to auto-fix any linting or formating issues. If any fixes are made,
those changes will be automatically committed and pushed back to the PR.
(This job requires that the PR author has "Allow edits from maintainers" enabled.)

PR auto-fix job started... Check job output.

✅ Changes applied successfully.

@aaronsteers
Copy link
Contributor

aaronsteers commented Nov 5, 2024

@tcboles - This looks great! I like the approach you took here - it seems nicely extensible and also simple enough to easily understand and explain to new developers.

I'm running a quick auto-fix here in CI for any simple linting or formatting issues. The slash command will only work for maintainers but you can run something similar using Ruff or Poe commands locally.

For both "get_source()" and "get_destination()", the linter will require adding a docstring entry for the new arg. These descriptions will then feed into our auto-generated API documentation.

I'll try to provide more detailed feedback inline but for now just wanted to drop in and say "thanks!" while giving my first observations/thoughts on this implementation. Feel free to ping me here if you have other questions/blockers.

Copy link

@coderabbitai coderabbitai bot left a 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

🧹 Outside diff range and nitpick comments (4)
airbyte/destinations/util.py (2)

Line range hint 20-31: Consider grouping related parameters into a configuration object?

The function has quite a few parameters now. Would you consider grouping the configuration-related parameters (config, config_change_callback) into a dedicated Config class? This could make the function signature cleaner and more maintainable, wdyt? 🤔

Example approach:

@dataclass
class DestinationConfig:
    config: dict[str, Any] | None = None
    config_change_callback: Callable[[dict[str, Any], int], None] | None = None

def get_destination(
    name: str,
    config: DestinationConfig | None = None,
    *,
    version: str | None = None,
    # ... other parameters
)
🧰 Tools
🪛 Ruff

20-20: Too many arguments in function definition (9 > 8)

(PLR0913)


20-20: Missing argument description in the docstring for get_destination: config_change_callback

(D417)


Line range hint 32-54: Add documentation for the config_change_callback parameter?

The docstring is missing information about the new config_change_callback parameter. Could we add a description explaining its purpose and usage? Something like:

    Args:
        name: connector name
        config: connector config - if not provided, you need to set it later via the set_config
            method.
+       config_change_callback: Optional callback function that will be invoked when configuration
+           changes occur. The callback receives the updated config dict and a version number.
        streams: list of stream names to select for reading. If set to "*", all streams will be
🧰 Tools
🪛 Ruff

20-20: Too many arguments in function definition (9 > 8)

(PLR0913)


20-20: Missing argument description in the docstring for get_destination: config_change_callback

(D417)

airbyte/_connector_base.py (2)

59-59: Consider enhancing the docstring for the config_change_callback parameter?

The new parameter looks good, but would it be helpful to add some docstring details about when the callback is invoked and what the timestamp parameter represents? Something like:

     def __init__(
         self,
         executor: Executor,
         name: str,
         config: dict[str, Any] | None = None,
         config_change_callback: Callable[[dict[str, Any], int], None] | None = None,
         *,
         validate: bool = False,
     ) -> None:
-        """Initialize the source.
+        """Initialize the source.
 
         If config is provided, it will be validated against the spec if validate is True.
+
+        Args:
+            config_change_callback: Optional callback that will be invoked when a configuration
+                change message is received. The callback receives the new configuration dict
+                and the Unix timestamp when the change was emitted.
         """

wdyt? 🤔

Also applies to: 67-67


386-393: Would you consider adding some defensive programming here?

The config change handling looks good, but what if the callback raises an exception? Maybe we could wrap it in a try-except to prevent callback errors from breaking the message processing pipeline? Something like:

         if (
             message.type == "CONTROL"
             and message.control.type == "CONNECTOR_CONFIG"
             and self.config_change_callback is not None
         ):
-            self.config_change_callback(message.control.config, message.control.emitted_at)
+            try:
+                self.config_change_callback(message.control.config, message.control.emitted_at)
+            except Exception as e:
+                self._print_error_message(f"Config change callback failed: {str(e)}")
             return

Also, should we validate that message.control.config is a dict and emitted_at is an int? wdyt? 🤔

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 5650728 and 41afdcb.

📒 Files selected for processing (5)
  • airbyte/_connector_base.py (4 hunks)
  • airbyte/destinations/base.py (3 hunks)
  • airbyte/destinations/util.py (2 hunks)
  • airbyte/sources/base.py (3 hunks)
  • airbyte/sources/util.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • airbyte/destinations/base.py
  • airbyte/sources/base.py
  • airbyte/sources/util.py
🧰 Additional context used
🪛 Ruff
airbyte/destinations/util.py

20-20: Too many arguments in function definition (9 > 8)

(PLR0913)


20-20: Missing argument description in the docstring for get_destination: config_change_callback

(D417)

🔇 Additional comments (2)
airbyte/destinations/util.py (1)

62-64: LGTM! Clean implementation of the callback forwarding.

The callback is correctly passed to the Destination constructor. 👍

airbyte/_connector_base.py (1)

366-367: The docstring update looks good!

Nice job updating the docstring to document the config change handling. 👍

Copy link

@coderabbitai coderabbitai bot left a 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

🧹 Outside diff range and nitpick comments (4)
airbyte/destinations/util.py (2)

Line range hint 20-31: Consider grouping related parameters into a dataclass or config object, wdyt? 🤔

The function has quite a few parameters now (9 total). To improve maintainability, we could group related parameters. For example:

@dataclass
class ConnectorConfig:
    version: str | None = None
    pip_url: str | None = None
    local_executable: Path | str | None = None
    docker_image: str | bool | None = None
    use_host_network: bool = False
    install_if_missing: bool = True

This would simplify the signature to:

def get_destination(
    name: str,
    config: dict[str, Any] | None = None,
    config_change_callback: Callable[[dict[str, Any]], None] | None = None,
    *,
    connector_config: ConnectorConfig = ConnectorConfig(),
) -> Destination:
🧰 Tools
🪛 Ruff

20-20: Too many arguments in function definition (9 > 8)

(PLR0913)


38-38: How about enhancing the config_change_callback documentation? 📝

The current description could be more detailed to help users understand:

  • When exactly the callback is invoked
  • What data structure to expect in the dictionary parameter
  • An example usage

Here's a suggestion:

"""
config_change_callback: Optional callback function that will be invoked when the connector
    receives a configuration change message. The callback receives a dictionary containing
    the new configuration. Example:
    ```python
    def on_config_change(new_config: dict[str, Any]) -> None:
        print(f"Configuration updated to: {new_config}")
    ```
"""
airbyte/_connector_base.py (2)

59-67: Add docstring for the new parameter?

The config_change_callback parameter could use some documentation in the docstring. WDYT about adding a description explaining:

  • What triggers the callback
  • The expected signature
  • Example usage

Example addition:

 def __init__(
     self,
     executor: Executor,
     name: str,
     config: dict[str, Any] | None = None,
     config_change_callback: Callable[[dict[str, Any]], None] | None = None,
     *,
     validate: bool = False,
 ) -> None:
     """Initialize the source.

     If config is provided, it will be validated against the spec if validate is True.
+
+    Args:
+        config_change_callback: Optional callback function that will be called when a
+            configuration change message is received. The callback receives the new
+            configuration as a dictionary.
     """

386-393: Consider enhancing type safety and error handling?

The implementation looks good! A couple of suggestions to make it even more robust:

  1. WDYT about using enum values for message types instead of string literals? This could prevent typos and make refactoring easier.
  2. Should we wrap the callback execution in a try-except to prevent callback errors from affecting the message processing pipeline?

Example enhancement:

+from enum import Enum
+
+class ControlMessageType(Enum):
+    CONNECTOR_CONFIG = "CONNECTOR_CONFIG"
+
 if (
     message.type == "CONTROL"
-    and message.control.type == "CONNECTOR_CONFIG"
+    and message.control.type == ControlMessageType.CONNECTOR_CONFIG.value
     and self.config_change_callback is not None
 ):
-    self.config_change_callback(message.control.config)
+    try:
+        self.config_change_callback(message.control.config)
+    except Exception as e:
+        self._print_error_message(f"Config change callback failed: {str(e)}")
     return
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 41afdcb and 957f7eb.

📒 Files selected for processing (5)
  • airbyte/_connector_base.py (4 hunks)
  • airbyte/destinations/base.py (3 hunks)
  • airbyte/destinations/util.py (3 hunks)
  • airbyte/sources/base.py (3 hunks)
  • airbyte/sources/util.py (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • airbyte/destinations/base.py
  • airbyte/sources/base.py
  • airbyte/sources/util.py
🧰 Additional context used
🪛 Ruff
airbyte/destinations/util.py

20-20: Too many arguments in function definition (9 > 8)

(PLR0913)

🔇 Additional comments (3)
airbyte/destinations/util.py (1)

Line range hint 57-74: Implementation looks clean! ✨

The config_change_callback is correctly passed through to the Destination constructor.

airbyte/_connector_base.py (2)

38-38: LGTM! Modern import approach.

Using collections.abc for Callable follows Python's modern type hinting practices. 👍


366-367: LGTM! Clear docstring update.

The docstring update clearly explains the new config change handling capability.

@tcboles tcboles marked this pull request as ready for review November 5, 2024 17:59
@tcboles tcboles changed the title Adds config_change_callback to Destinations and Sources Feat: Adds config_change_callback to Destinations and Sources Nov 5, 2024
@tcboles
Copy link
Contributor Author

tcboles commented Nov 5, 2024

@tcboles - This looks great! I like the approach you took here - it seems nicely extensible and also simple enough to easily understand and explain to new developers.

I'm running a quick auto-fix here in CI for any simple linting or formatting issues. The slash command will only work for maintainers but you can run something similar using Ruff or Poe commands locally.

For both "get_source()" and "get_destination()", the linter will require adding a docstring entry for the new arg. These descriptions will then feed into our auto-generated API documentation.

I'll try to provide more detailed feedback inline but for now just wanted to drop in and say "thanks!" while giving my first observations/thoughts on this implementation. Feel free to ping me here if you have other questions/blockers.

Thanks @aaronsteers. I went ahead and removed the emitted time time from the callback. I also added basic tests. Let me know if you need anything updated.

@aaronsteers
Copy link
Contributor

@tcboles - Looking good! Can you remind me which source you are wanting to use this feature with, and have you had any luck testing manually with that source?

@tcboles
Copy link
Contributor Author

tcboles commented Nov 7, 2024

@tcboles - Looking good! Can you remind me which source you are wanting to use this feature with, and have you had any luck testing manually with that source?

I am going to be testing it with Quickbooks. I haven't had time to complete the testing yet. I will let you know when that is finished. It will hopefully be today or tomorrow.

@tcboles
Copy link
Contributor Author

tcboles commented Nov 7, 2024

@aaronsteers Update on the testing. Everything is working for my use case with quickbooks.

@aaronsteers
Copy link
Contributor

@tcboles - That's great! I'll do a final review and then merge if nothing else outstanding.

@aaronsteers
Copy link
Contributor

aaronsteers commented Nov 7, 2024

@tcboles - Actually one more question so I can put this in the docs:

What is the shape of the config dict you received in the control message from QuickBooks? Can you tell me the keys/structure, obviously with anything sensitive removed?

@tcboles
Copy link
Contributor Author

tcboles commented Nov 8, 2024

@tcboles - Actually one more question so I can put this in the docs:

What is the shape of the config dict you received in the control message from QuickBooks? Can you tell me the keys/structure, obviously with anything sensitive removed?

Here is the full control message that was sent from source-quickbooks. The callback receives everything inside control.connectorConfig.config.

{
  "type": "CONTROL",
  "control": {
    "type": "CONNECTOR_CONFIG",
    "emitted_at": 1731090040516.7517,
    "connectorConfig": {
      "config": {
        "auth_type": "oauth2",
        "sandbox": false,
        "start_date": "2000-01-01T00:00:00Z",
        "credentials": {
          "client_id": "************",
          "client_secret": "************",
          "refresh_token": "************",
          "access_token": "************",
          "realm_id": "************",
          "token_expiry_date": "2024-11-08T19:20:40.516582+00:00"
        }
      }
    }
  }
}

@aaronsteers aaronsteers merged commit 6246e6b into airbytehq:main Nov 8, 2024
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants