Parametrized destinations #746

steinitzu · 2023-11-09T18:27:49Z

Description

Draft implementation of parametrized destinations.

I added a base DestinationFactory with same interface as the destination modules just to hold the config arguments.

Usage is e.g.

import dlt
from dlt.destinations import snowflake, filesystem


pipeline = dlt.pipeline(
    "callable_dest_staging",
    destination=snowflake(),
    staging=filesystem("s3://my-bucket")
)

The biggest change is that I moved all destinations to dlt.destinations.impl. Just because I think it would be more confusing to have a module and a callable with the same name.
I.e. dlt.destinations.postgres(...) is the factory, dlt.destinations.impl.postgres is the package with all the destination code.

Related Issues

Resolves allow to parametrize destinations by converting them into callables #508

netlify · 2023-11-09T18:27:54Z

✅ Deploy Preview for dlt-hub-docs canceled.

Name	Link
🔨 Latest commit	`1f16c8f`
🔍 Latest deploy log	https://app.netlify.com/sites/dlt-hub-docs/deploys/6557c59bd300e00008b25c4e

steinitzu · 2023-11-09T18:28:09Z

@rudolfix would be great to know what you think of the approach before I finalize it.

rudolfix

@steinitzu I really like it!. using partial to get initial config is smart. I think factory impls can be simplified even more.

btw. we'll have another ticket to improve destinations even further ie. to let people assign names to them so we have dynamic configs in case of having several environments ie.
progress-dev
will be taken from [destination.progress-dev] just a headsup

but we'll merge that first

rudolfix · 2023-11-10T19:57:17Z

dlt/destinations/impl/filesystem/factory.py

+
+    destination = _filesystem
+
+    @with_config(spec=FilesystemDestinationClientConfiguration, sections=(known_sections.DESTINATION, 'filesystem'), accept_partial=True)


using partial is smart!

rudolfix · 2023-11-10T20:01:49Z

dlt/destinations/impl/snowflake/factory.py

+    ) -> None:
+        cfg: SnowflakeClientConfiguration = kwargs['_dlt_config']
+        self.credentials = cfg.credentials
+        self.config_params = {


all configurations are also dictionaries. so you could get dict, assign credentials (part of the base) and pass rest of the dict as config_params

also possible at run time IMO - when actual client/capabilities are created

rudolfix

@steinitzu on the second look: I think we should also instantiate destination factory in pipeline, when we pass the destination as string and completely get rid of ability to pass a module.

I bet we can get rid of most of the code in __init__.py of each of destination implementations: for sure the _configure and client() methods. maybe all of them?

steinitzu · 2023-11-13T03:15:39Z

Not quite done, but some more progress.

DestinationReference is removed entirely. client/capabilities/spec defined on the factory and always use it
Also renamed DestinationFactory to Destination, think that makes sense as now it's "the" base destination
I create capabilities at import time ( https://github.com/dlt-hub/dlt/blob/sthor%2Fparametrized-destination/dlt/destinations/impl/postgres/factory.py#L15 ). I couldn't see a reason to have them behind a callable (except for dummy where they can be configured). Any problem with this?

rudolfix

pls see how I'd simplify configuring the destination even more

dlt/common/destination/__init__.py

dlt/common/destination/reference.py

dlt/destinations/impl/duckdb/configuration.py

dlt/destinations/impl/duckdb/factory.py

rudolfix · 2023-11-13T13:19:23Z

dlt/destinations/impl/duckdb/factory.py

+    @with_config(spec=DuckDbClientConfiguration, sections=(known_sections.DESTINATION, 'duckdb'), accept_partial=True)
+    def __init__(
+        self,
+        credentials: DuckDbCredentials = None,


would be better to get connection string or path and create DuckDbCredentials from it. it's constructor should accept

Yeah probably all destinations we should accept either TCredentials | str or any native value types.

rudolfix · 2023-11-13T13:30:13Z

dlt/destinations/impl/duckdb/configuration.py

@@ -95,28 +96,25 @@ class DuckDbCredentials(DuckDbBaseCredentials):

    __config_gen_annotations__: ClassVar[List[str]] = []

-    def on_resolved(self) -> None:


What is the problem here? on_resolved is not called in new model because we allow for partial? but at some point we must make sure that configuration is complete. pls see my other comments

I was having an issue where on_resolved was called outside of pipeline context, so when using :pipeline: it gets reset to None.
The top level client config is partial but the credentials are not partial, maybe that's why?
I think it was just right when I instantiated the factory from here: https://github.com/dlt-hub/dlt/blob/sthor%2Fparametrized-destination/dlt/pipeline/__init__.py#L119

It took me some time to understand what was happening.
But I just moved the same logic to the _database_path method which is called on the fly when conn string is generated, at that point pipeline should always be set up already.

Ok my fix was a problem for dbt runner so I went back to the original on_resolved method, but added this https://github.com/dlt-hub/dlt/blob/sthor%2Fparametrized-destination/dlt/destinations/impl/duckdb/configuration.py#L99-L104 so it doesn't trigger right when the instance is created

rudolfix · 2023-11-13T13:42:11Z

dlt/destinations/impl/duckdb/factory.py

+
+        return DuckDbClient
+
+    @with_config(spec=DuckDbClientConfiguration, sections=(known_sections.DESTINATION, 'duckdb'), accept_partial=True)


OK, we can simplify this even more.

I'd not use @with_config here at. I'd just let people pass initial parameters and do not pull config at all.

I'd use with_config not as decorator but within a function that would return a configuration. there's no partial.
so let's imagine we have this in Destination:

def configuration(initial: TConfig) -> TConfig: return with_config(lambda x: x, spec=DuckDbClientConfiguration, sections=(known_sections.DESTINATION, self.destination_name), initial_config=initial)

we'd need to extend with_config to accept inital config in the decorator BUT if you look in the code there's already mechanims to handle that:

# if one of arguments is spec the use it as initial value if spec_arg: config = bound_args.arguments.get(spec_arg.name, None)

if we do that, we do not use partial, we always resolve configuration etc.
where initial is coming from? we form it in __init__ from the aguments.

Hmm yeah I think that makes sense. So we'd just be leaving resolving config until the last moment when it's needed.

Question, why use with_config and not resolve_config directly for this?

you can use resolve_config! looks even better

rudolfix

this is really close! please remember to deprecate credentials argument usage in Pipeline run and load method

rudolfix · 2023-11-15T20:36:33Z

dlt/destinations/impl/bigquery/factory.py

@@ -0,0 +1,69 @@
+# import typing as t


not needed :)

rudolfix · 2023-11-15T20:48:28Z

dlt/destinations/impl/athena/factory.py

+    def __init__(
+        self,
+        query_result_bucket: t.Optional[str] = None,
+        credentials: t.Optional[AwsCredentials] = None,


for each init in each destination let's handle credentials as:
Union[SomeCredentials, Dict[str, Any], Any] then look how CredentialsConfiguration implements

def __init__(self, init_value: Any = None) -> None:

so you can just pass the credentials the constructor of SomeCredentials (if they are not already credentials). basically doing what pipeline is not doing with "credentials" argument

client_spec = destination.spec() # initialize explicit credentials if not as_staging: # explicit credentials passed to dlt.pipeline should not be applied to staging credentials = credentials or self.credentials if credentials is not None and not isinstance(credentials, CredentialsConfiguration): # use passed credentials as initial value. initial value may resolve credentials credentials = initialize_credentials( client_spec.get_resolvable_fields()["credentials"], credentials ) ```

rudolfix · 2023-11-15T20:49:30Z

dlt/destinations/impl/duckdb/configuration.py

@@ -95,6 +96,13 @@ class DuckDbCredentials(DuckDbBaseCredentials):

    __config_gen_annotations__: ClassVar[List[str]] = []

+    def is_partial(self) -> bool:
+        partial = super().is_partial()


you should not call super(). config resolver will do that for you

Huh? How does that work? :/

rudolfix · 2023-11-15T20:49:49Z

dlt/destinations/impl/duckdb/configuration.py

@@ -173,6 +180,9 @@ def _path_from_pipeline(self, default_path: str) -> Tuple[str, bool]:

        return default_path, True

+    def _conn_str(self) -> str:


do we still need it?

Seems just for motherduck compatiblity it's needed, uses the same method.

dlt/destinations/impl/athena/factory.py

rudolfix · 2023-11-15T20:52:42Z

dlt/destinations/impl/duckdb/factory.py

+    # @with_config(spec=DuckDbClientConfiguration, sections=(known_sections.DESTINATION, 'duckdb'), accept_partial=True)
+    def __init__(
+        self,
+        credentials: t.Union[DuckDbCredentials, str, "DuckDBPyConnection"] = None,


rudolfix · 2023-11-15T20:57:30Z

dlt/destinations/impl/mssql/factory.py

+
+    def __init__(
+        self,
+        credentials: t.Union[MsSqlCredentials, str] = None,


thx for accepting str :)

rudolfix · 2023-11-15T20:58:28Z

dlt/destinations/impl/redshift/factory.py

@@ -0,0 +1,67 @@
+# import typing as t
+
+# from dlt.common.destination import Destination, DestinationCapabilitiesContext


rudolfix · 2023-11-15T21:00:54Z

dlt/destinations/impl/weaviate/factory.py

+    def __init__(
+        self,
+        credentials: t.Optional[WeaviateCredentials] = None,
+        **kwargs: t.Any,


maybe we could add vectorizer and module_config?

rudolfix · 2023-11-15T21:03:43Z

tests/load/duckdb/test_duckdb_client.py



 def test_duckdb_database_delete() -> None:
    db_path = "_storage/path_test_quack.duckdb"
-    p = dlt.pipeline(pipeline_name="quack_pipeline", credentials=db_path, destination="duckdb")
+    p = dlt.pipeline(pipeline_name="quack_pipeline", destination=duckdb(credentials=DuckDbCredentials(db_path)))


this should still work. we just need to create the instance ourselves if stirng is passed. I explained that above

rudolfix

LGTM! thanks for the docstrings!

steinitzu requested a review from rudolfix November 9, 2023 18:27

rudolfix reviewed Nov 10, 2023

View reviewed changes

This was referenced Nov 10, 2023

extend destination factory to allow for custom names and config sections #750

Closed

improve destination fingerprinting and info #751

Open

implement destination sinks #752

Closed

steinitzu force-pushed the sthor/parametrized-destination branch from c5385fb to ed55b0d Compare November 13, 2023 02:59

rudolfix mentioned this pull request Nov 13, 2023

0.4.1 release notes and master merge #763

Closed

rudolfix requested changes Nov 13, 2023

View reviewed changes

steinitzu force-pushed the sthor/parametrized-destination branch 2 times, most recently from b8f9d97 to 69b3bc5 Compare November 14, 2023 21:09

steinitzu added 15 commits November 14, 2023 16:27

Move destination modules to subfolder

3cdab60

Mockup destination factory

3b53861

Destination factory replacing reference and dest __init__

4b449e3

Update factories

f195fa7

Defer duckdb credentials resolving in pipeline context

340f637

Simplify destination config resolution

4ae4ceb

capabilities are callable

bccfa9d

bigquery, athena factories

3bde2f4

Add rest of factories

0d59d51

Cleanup

3ee262d

Destination type vars

9243b2f

Cleanup

9ad561c

Fix test

007d7a3

Create initial config from non-defaults only

62b5a57

Update naming convention path

d77b54a

steinitzu force-pushed the sthor/parametrized-destination branch from 69b3bc5 to d77b54a Compare November 14, 2023 21:31

steinitzu added 2 commits November 14, 2023 18:00

Fix config in bigquery location test

0d4ad49

Only keep non-default config args in factory

f657071

steinitzu force-pushed the sthor/parametrized-destination branch from 85c2b86 to f657071 Compare November 15, 2023 01:05

steinitzu added 2 commits November 14, 2023 20:45

Resolve duckdb credentials in pipeline context

56e922a

Cleanup

08a8fdc

rudolfix requested changes Nov 15, 2023

View reviewed changes

steinitzu added 2 commits November 15, 2023 17:36

Union credentials arguments

2567ca0

Common tests without dest dependencies

3537c03

steinitzu force-pushed the sthor/parametrized-destination branch from fba1d21 to 3537c03 Compare November 16, 2023 00:15

steinitzu added 5 commits November 15, 2023 19:17

Forward all athena arguments

25a937a

Delete commented code

97f1afc

Reference docstrings

fe24e14

Add deprecation warning for credentials argument

481a7cb

Init docstrings for destination factories

d91402c

rudolfix added the sprint label Nov 17, 2023

steinitzu added 3 commits November 17, 2023 12:12

Fix tests

fc92929

Destination name in output

13ec6fb

Correct exception in unknown destination test

1f16c8f

steinitzu marked this pull request as ready for review November 17, 2023 19:57

rudolfix previously approved these changes Nov 18, 2023

View reviewed changes

Merge branch 'devel' into sthor/parametrized-destination

2541d49

rudolfix dismissed their stale review via 2541d49 November 18, 2023 17:53

rudolfix merged commit 105795c into devel Nov 18, 2023
26 of 32 checks passed

rudolfix deleted the sthor/parametrized-destination branch November 18, 2023 17:54

rudolfix mentioned this pull request Dec 7, 2023

update destinations documentation #808

Closed

emmanuel-ferdman mentioned this pull request Sep 29, 2024

Update weaviate reference #1896

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parametrized destinations #746

Parametrized destinations #746

steinitzu commented Nov 9, 2023

netlify bot commented Nov 9, 2023 •

edited

Loading

steinitzu commented Nov 9, 2023

rudolfix left a comment

rudolfix Nov 10, 2023

rudolfix Nov 10, 2023

rudolfix left a comment

steinitzu commented Nov 13, 2023

rudolfix left a comment

rudolfix Nov 13, 2023

steinitzu Nov 13, 2023

rudolfix Nov 13, 2023

steinitzu Nov 13, 2023

steinitzu Nov 15, 2023

rudolfix Nov 13, 2023

steinitzu Nov 13, 2023

steinitzu Nov 13, 2023

rudolfix Nov 13, 2023

rudolfix left a comment

rudolfix Nov 15, 2023

rudolfix Nov 15, 2023

rudolfix Nov 15, 2023

steinitzu Nov 15, 2023

rudolfix Nov 15, 2023

steinitzu Nov 15, 2023

rudolfix Nov 15, 2023

rudolfix Nov 15, 2023

rudolfix Nov 15, 2023

rudolfix Nov 15, 2023

rudolfix Nov 15, 2023

rudolfix left a comment


		destination = _filesystem

		@with_config(spec=FilesystemDestinationClientConfiguration, sections=(known_sections.DESTINATION, 'filesystem'), accept_partial=True)

		@@ -95,28 +96,25 @@ class DuckDbCredentials(DuckDbBaseCredentials):

		__config_gen_annotations__: ClassVar[List[str]] = []

		def on_resolved(self) -> None:


		return DuckDbClient

		@with_config(spec=DuckDbClientConfiguration, sections=(known_sections.DESTINATION, 'duckdb'), accept_partial=True)

		@@ -173,6 +180,9 @@ def _path_from_pipeline(self, default_path: str) -> Tuple[str, bool]:

		return default_path, True

		def _conn_str(self) -> str:

		@@ -0,0 +1,67 @@
		# import typing as t

		# from dlt.common.destination import Destination, DestinationCapabilitiesContext

Parametrized destinations #746

Parametrized destinations #746

Conversation

steinitzu commented Nov 9, 2023

Description

Related Issues

netlify bot commented Nov 9, 2023 • edited Loading

✅ Deploy Preview for dlt-hub-docs canceled.

steinitzu commented Nov 9, 2023

rudolfix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolfix left a comment

Choose a reason for hiding this comment

steinitzu commented Nov 13, 2023

rudolfix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolfix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolfix left a comment

Choose a reason for hiding this comment

netlify bot commented Nov 9, 2023 •

edited

Loading