destination config updates #783

sh-rp · 2023-11-22T10:52:51Z

Renames destionation_name to destionation_type on DestinationClientConfiguration
Adds new variables name and environment to DestinationClientConfiguration
Makes config section respect the destination name.

add name to destination config add environment to destination config

netlify · 2023-11-22T10:52:56Z

✅ Deploy Preview for dlt-hub-docs canceled.

Name	Link
🔨 Latest commit	`f48338b`
🔍 Latest deploy log	https://app.netlify.com/sites/dlt-hub-docs/deploys/65699923a7f6d20008ca1f3f

dlt/common/destination/reference.py

sh-rp · 2023-11-22T11:16:59Z

General notes:

I'm not sure that it makes sense to have the destination name in the config. I think it should be set in code and then you can select a config section based on that name. If you also have the name in the config it might be a bit confusing. Generally I am not quite sure how the setup for a user with multiple named destinations and environments would look like..
Should we also store the destination name in the pipeline state? I am not quite sure why the destination info is stored there in the first place, this is one of these (for me) confusing mechanisms where stuff gets stored somewhere and then dlt does thing that are not really reflected in the code. Anyway if we do not save the name but have config segments that are reliant on the name, then the second run might produce other results.
I have implemented the dynamic configuration resolution in the same way it was done before in the from_reference constructor on the destination. This seems to work good, but it is different than was specified in the ticket. Let me know if this also works. the one things that does not work now though, is to have common credentials for all duckdb destinations if those duckdb destinations have different names for example (if we also have other destinations types)
I have also made the dataset configurable via the config, I am not sure why this was not possible before, so maybe I am missing something there and will have to revert that..

sh-rp · 2023-11-22T14:34:01Z

dlt/pipeline/configuration.py

@@ -11,8 +11,8 @@
 class PipelineConfiguration(BaseConfiguration):
    pipeline_name: Optional[str] = None
    pipelines_dir: Optional[str] = None
-    destination_name: Optional[str] = None
-    staging_name: Optional[str] = None
+    destination_type: Optional[str] = None


maybe we should just call this destination?

I'd keep _type to be more explicit. also please link to this PR and add info that we changed the names in this config in here #763

also please add on_resolved and please keep destination_name and staging_name in the config but display deprecation warnings. Steini added a method to warn on deprecation somewhere in the Pipeline class

there are people that configure destinations via config.

btw. we may keep destination name in the longer run. so in fact we'll create a destination with given type and given name. so let's do that:

if only name is present or only type is present, we create a destination with name == type

if they are different we pass both as destination reference.
i think destination reference supports it

Ok, so we do not deprecated destination_name and staging_name, right?

no, we keep them

# Conflicts: # dlt/common/destination/reference.py # dlt/common/pipeline.py # dlt/destinations/impl/bigquery/factory.py # dlt/destinations/impl/duckdb/factory.py # dlt/destinations/impl/filesystem/configuration.py # dlt/destinations/impl/motherduck/factory.py # dlt/destinations/impl/redshift/factory.py # dlt/destinations/impl/snowflake/factory.py # dlt/destinations/impl/weaviate/factory.py # dlt/helpers/dbt/__init__.py # dlt/load/load.py # dlt/pipeline/__init__.py # dlt/pipeline/pipeline.py # tests/cli/test_init_command.py # tests/load/test_job_client.py # tests/load/test_sql_client.py # tests/load/utils.py # tests/pipeline/test_pipeline_trace.py

sh-rp · 2023-11-23T10:12:59Z

dlt/destinations/impl/athena/configuration.py

@@ -23,3 +23,19 @@ def __str__(self) -> str:
            return str(self.staging_config.credentials)
        else:
            return "[no staging set]"
+
+    if TYPE_CHECKING:


🥔🥔🥔

rudolfix

thanks for all missing mypy init delcarations...

I'm not sure that it makes sense to have the destination name in the config. I think it should be set in code and then you can select a config section based on that name. If you also have the name in the config it might be a bit confusing. Generally I am not quite sure how the setup for a user with multiple named destinations and environments would look like..

yes - that may be good idea. I even proposed to make "name" final in the config. so you can completely take it out.
User setup: if destination is entity independent from pipeline we can have many devel or production BigQuery project and users may want to name them and refer to them in many pipelines.

Should we also store the destination name in the pipeline state? I am not quite sure why the destination info is stored there in the first place, this is one of these (for me) confusing mechanisms where stuff gets stored somewhere and then dlt does thing that are not really reflected in the code. Anyway if we do not save the name but have config segments that are reliant on the name, then the second run might produce other results.

We should store both name and type of the destination in the pipeline state. You'll also need to bump state version and write a migration. The goal is to be able to retrieve exactly the same configuration for the pipeline (now we need both type and name - because the type is not stored in the config - I think we can keep it that way).

Right now it is used by streamlit app and CLI to do dlt.attach and obtain fully functional pipeline object outside of the original pipeline script.
We do not talk much about this in the docs - right. But this is quite powerful way for doing automations.

I have implemented the dynamic configuration resolution in the same way it was done before in the from_reference constructor on the destination. This seems to work good, but it is different than was specified in the ticket. Let me know if this also works. the one things that does not work now though, is to have common credentials for all duckdb destinations if those duckdb destinations have different names for example (if we also have other destinations types)

Heh you are right. But putting the config under [destination] will work

I have also made the dataset configurable via the config, I am not sure why this was not possible before, so maybe I am missing something there and will have to revert that..

I think it makes sense.

dlt/common/destination/reference.py

dlt/common/pipeline.py

dlt/destinations/impl/athena/factory.py

dlt/destinations/impl/bigquery/configuration.py

rudolfix · 2023-11-23T17:30:44Z

dlt/pipeline/configuration.py

@@ -11,8 +11,8 @@
 class PipelineConfiguration(BaseConfiguration):
    pipeline_name: Optional[str] = None
    pipelines_dir: Optional[str] = None
-    destination_name: Optional[str] = None
-    staging_name: Optional[str] = None
+    destination_type: Optional[str] = None


I'd keep _type to be more explicit. also please link to this PR and add info that we changed the names in this config in here #763

also please add on_resolved and please keep destination_name and staging_name in the config but display deprecation warnings. Steini added a method to warn on deprecation somewhere in the Pipeline class

rudolfix · 2023-11-23T17:33:45Z

dlt/pipeline/configuration.py

@@ -11,8 +11,8 @@
 class PipelineConfiguration(BaseConfiguration):
    pipeline_name: Optional[str] = None
    pipelines_dir: Optional[str] = None
-    destination_name: Optional[str] = None
-    staging_name: Optional[str] = None
+    destination_type: Optional[str] = None


there are people that configure destinations via config.

btw. we may keep destination name in the longer run. so in fact we'll create a destination with given type and given name. so let's do that:

if only name is present or only type is present, we create a destination with name == type

if they are different we pass both as destination reference.
i think destination reference supports it

sh-rp · 2023-11-27T11:24:26Z

Questions:

Did I understand correctly that destination_name is not to be marked as deprecated?
The pipeline steps currently support a destination and stage argument which can be a reference or a destination_type, should it be possible to passe type and name there too? Or should we just assume people will pass a destination instance with this info if they really want to use it?
Does the environment also need to be stored in the pipeline props? Probably yes? Should this maybe be a nested structure and not 3 props for staging and destination each? Maybe the TDestinationReferenceArg should be a union of str, destination instance and an object with three values type, name and environment.

sh-rp · 2023-11-27T12:13:37Z

Hm, maybe dataset should also be part of this destination object...

rudolfix

Did I understand correctly that destination_name is not to be marked as deprecated?
yes

The pipeline steps currently support a destination and stage argument which can be a reference or a destination_type, should it be possible to passe type and name there too? Or should we just assume people will pass a destination instance with this info if they really want to use it?
right now if someone wants to pass both type and name, s/he must pass the Destination instance. we may support a tuple later (if that is what you mean)

Does the environment also need to be stored in the pipeline props? Probably yes? Should this maybe be a nested structure and not 3 props for staging and destination each? Maybe the TDestinationReferenceArg should be a union of str, destination instance and an object with three values type, name and environment.

You can move destination_name and destination_type to destination but we do not need to add environment there. we just need a minimum information to identify right configuration element. OR we just dump a whole config there (without secrets).

what we keep in state supports only minimum information and assumes that config OR code has the rest. if that is not the case some things ie. CLI commands will not work

let's not change the TDestinationReferenceArg but what you can (should do) is to define a NewType for destination_type that is a special kind of string that can be a full module name or destination from dlt default module. and then use it TDestinationReferenceArg . you'll catch a lot of problems with that!

rudolfix · 2023-11-27T12:18:47Z

dlt/helpers/streamlit_helper.py

@@ -125,7 +125,7 @@ def _query_data_live(query: str, schema_name: str = None) -> pd.DataFrame:
        schema_names = ", ".join(sorted(pipeline.schema_names))
        st.markdown(f"""
        * pipeline name: **{pipeline.pipeline_name}**
-        * destination: **{str(credentials)}** in **{pipeline.destination.name}**
+        * destination: **{str(credentials)}** in **{pipeline.destination.destination_name}**


maybe we can display both name and type?

rudolfix · 2023-11-27T12:26:37Z

dlt/pipeline/configuration.py

@@ -11,8 +11,8 @@
 class PipelineConfiguration(BaseConfiguration):
    pipeline_name: Optional[str] = None
    pipelines_dir: Optional[str] = None
-    destination_name: Optional[str] = None
-    staging_name: Optional[str] = None
+    destination_type: Optional[str] = None


no, we keep them

rudolfix · 2023-11-27T12:27:26Z

dlt/pipeline/pipeline.py

@@ -251,7 +251,7 @@ class Pipeline(SupportsPipeline):
    """A working directory of the pipeline"""
    destination: TDestination = None
    staging: TDestination = None
-    """The destination reference which is ModuleType. `destination.name` returns the name string"""
+    """The destination reference which is ModuleType. `destination.destination_name` returns the name string"""


it is a Destination class now

rudolfix · 2023-11-27T12:30:16Z

dlt/pipeline/pipeline.py

@@ -1407,18 +1415,18 @@ def _restore_state_from_destination(self) -> Optional[TPipelineState]:
                    if state is None:
                        logger.info(
                            "The state was not found in the destination"
-                            f" {self.destination.name}:{dataset_name}"
+                            f" {self.destination.destination_name}:{dataset_name}"


should we show also type?

tests/destinations/test_destination_name_and_config.py

rudolfix · 2023-11-27T13:26:20Z

tests/pipeline/test_pipeline_state.py

+        "test_pipeline", state_v2, state_v2["_state_engine_version"], STATE_ENGINE_VERSION  # type: ignore
+    )
+    assert state_v2["destination_name"] == "redshift"
+    assert state_v2["destination_type"] == "redshift"


this looks wrong. it must always be a full path to a module that can be imported.

rudolfix · 2023-11-27T13:26:28Z

tests/pipeline/test_pipeline_state.py

+    assert state_v2["destination_type"] == "redshift"
+    assert "destination" not in state_v2
+    assert state_v2["staging_name"] == "filesystem"
+    assert state_v2["staging_type"] == "filesystem"


rudolfix · 2023-11-27T13:27:02Z

tests/pipeline/test_pipeline_state.py

@@ -485,6 +490,11 @@ def test_migrate_state(test_storage: FileStorage) -> None:
    assert state["_state_engine_version"] == STATE_ENGINE_VERSION
    assert "_local" in state

+    # check destination migration
+    assert state["destination_name"] == "dlt.destinations.postgres"


name should be postgres

rudolfix · 2023-11-27T13:35:50Z

dlt/common/destination/reference.py

        """The destination name will either be explicitely set while creating the destination or will be taken from the type"""
-        return self.config_params.get("name") or self.destination_type
+        return self.config_params.get("destination_name") or self.destination_type

    @property
    def destination_type(self) -> str:


is it returning a full class name? from which we can always recreate instance? I think Stein messed up a little here. destination type is always a full type that you can pass to from_reference and get back the instance (with default config)
this must be tested.
make sure that state keeps a full type in pipeline and that this type is valid - also after migration

the line below returns wrong type

maybe let's make it to return Type? then you'll catch all the bugs

I am not sure what you mean. We already have the type, no? I have changed it that it will return the full path to the destination module and added / updated tests for this.

rudolfix · 2023-11-27T13:38:47Z

dlt/common/destination/reference.py

        """The destination name will either be explicitely set while creating the destination or will be taken from the type"""
-        return self.config_params.get("name") or self.destination_type
+        return self.config_params.get("destination_name") or self.destination_type


use def to_name(ref: TDestinationReferenceArg) -> str: to get name from type. use it everywhere you do that

sh-rp · 2023-11-27T20:30:35Z

let's not change the TDestinationReferenceArg but what you can (should do) is to define a NewType for destination_type that is a special kind of string that can be a full module name or destination from dlt default module. and then use it TDestinationReferenceArg . you'll catch a lot of problems with that!

This i don't get. A module name can just be a string, no? In that case the linter will not be able to differentiate between some wrong string and a valid module name. But I probably am missing sth here.

tests/destinations/test_destination_name_and_config.py

sh-rp · 2023-11-27T21:01:05Z

Changing the detination_type property to the full module path changes the output of some other places to this full path as well. I am not sure if we want that everywhere, or if we want to shorten it to only the class name for dlt destinations, you can let me know if the way it is is alright.
I have two questions in the other comments.

sh-rp · 2023-11-28T10:39:29Z

I can't get tests/cli/test_pipeline_command.py::test_pipeline_command_operations to work. If I use the dlt commands regularly from the console, i can create a pipeline, run it and then drop a table. but somehow in the test it fails and I am not sure why..

This reverts commit 0b624b05b7c0d5880099b7a3d9e9ea2a63378002.

sh-rp · 2023-11-29T15:22:10Z

tests/destinations/test_destination_name_and_config.py

+    assert load_info.environment == "production"
+
+
+def test_config_respects_name() -> None:


@rudolfix here is the test for config with name

# Conflicts: # Makefile # dlt/pipeline/pipeline.py # dlt/pipeline/state_sync.py # tests/pipeline/test_pipeline_state.py

rudolfix

this is good now! thanks! a few simple tests that make sure that pipeline props to state works right is needed but I'll do this with new ticket

sh-rp added 2 commits November 22, 2023 11:21

rename destination name to destination type

a24284b

add name to destination config add environment to destination config

clean up and extend destination configs

9bd96f5

sh-rp changed the base branch from master to devel November 22, 2023 10:53

sh-rp commented Nov 22, 2023

View reviewed changes

dlt/common/destination/reference.py Outdated Show resolved Hide resolved

allow destination config sections to be resolved by name

694bf0a

sh-rp added 3 commits November 22, 2023 12:19

fix destination test

2a1556d

select config section based on name and add tests

be724a0

update pipeline config

072670f

sh-rp commented Nov 22, 2023

View reviewed changes

small test fixes

5cf7481

sh-rp added the devel label Nov 22, 2023

sh-rp commented Nov 23, 2023

View reviewed changes

rudolfix requested changes Nov 23, 2023

View reviewed changes

sh-rp added 2 commits November 26, 2023 21:54

pr fixes (no tests yet)

d4f8a24

small fixes

45e2cb4

add some tests

a3ff0b3

rudolfix requested changes Nov 27, 2023

View reviewed changes

pr fixes

4df026d

sh-rp force-pushed the d#/destination_config_updates branch from 472fe51 to 4df026d Compare November 27, 2023 20:57

sh-rp commented Nov 27, 2023

View reviewed changes

tests/destinations/test_destination_name_and_config.py Outdated Show resolved Hide resolved

sh-rp added 2 commits November 28, 2023 11:53

test updates

28d56d2

Revert "Auxiliary commit to revert individual files from 28d56d2"

ce41021

This reverts commit 0b624b05b7c0d5880099b7a3d9e9ea2a63378002.

sh-rp commented Nov 29, 2023

View reviewed changes

rudolfix and others added 4 commits November 29, 2023 22:02

does not overwrite destination with state if already set

1df0189

adds destinations tests to ci

9d782ad

improves tests and fixes small bugs

8b9d309

Merge branch 'devel' into d#/destination_config_updates

a206c5e

# Conflicts: # Makefile # dlt/pipeline/pipeline.py # dlt/pipeline/state_sync.py # tests/pipeline/test_pipeline_state.py

sh-rp force-pushed the d#/destination_config_updates branch from 5d05aab to a206c5e Compare November 30, 2023 09:30

sh-rp and others added 3 commits November 30, 2023 11:01

small fix

9719278

fix migration bug and migration tests

dabeceb

fixes final and classvar type modifiers

fdf0615

rudolfix force-pushed the d#/destination_config_updates branch from 9421551 to fdf0615 Compare November 30, 2023 22:15

ignores config values if field is final

6568d36

rudolfix force-pushed the d#/destination_config_updates branch from 9e9715f to 6568d36 Compare December 1, 2023 07:45

fixes getting destination init signature

f48338b

rudolfix marked this pull request as ready for review December 1, 2023 09:08

rudolfix approved these changes Dec 1, 2023

View reviewed changes

rudolfix merged commit fcb7d5e into devel Dec 1, 2023
39 of 40 checks passed

rudolfix deleted the d#/destination_config_updates branch December 1, 2023 09:10

rudolfix mentioned this pull request Dec 7, 2023

update destinations documentation #808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

destination config updates #783

destination config updates #783

sh-rp commented Nov 22, 2023 •

edited

Loading

netlify bot commented Nov 22, 2023 •

edited

Loading

sh-rp commented Nov 22, 2023 •

edited

Loading

sh-rp Nov 22, 2023

rudolfix Nov 23, 2023

rudolfix Nov 23, 2023

sh-rp Nov 26, 2023

rudolfix Nov 27, 2023

sh-rp Nov 23, 2023

rudolfix left a comment

rudolfix Nov 23, 2023

rudolfix Nov 23, 2023

sh-rp commented Nov 27, 2023 •

edited

Loading

sh-rp commented Nov 27, 2023

rudolfix left a comment

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

rudolfix Nov 27, 2023

sh-rp Nov 27, 2023

rudolfix Nov 27, 2023

sh-rp commented Nov 27, 2023

sh-rp commented Nov 27, 2023

sh-rp commented Nov 28, 2023

sh-rp Nov 29, 2023

rudolfix left a comment

		assert load_info.environment == "production"


		def test_config_respects_name() -> None:

destination config updates #783

destination config updates #783

Conversation

sh-rp commented Nov 22, 2023 • edited Loading

netlify bot commented Nov 22, 2023 • edited Loading

✅ Deploy Preview for dlt-hub-docs canceled.

sh-rp commented Nov 22, 2023 • edited Loading

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

sh-rp commented Nov 27, 2023 • edited Loading

sh-rp commented Nov 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh-rp commented Nov 27, 2023

sh-rp commented Nov 27, 2023

sh-rp commented Nov 28, 2023

Choose a reason for hiding this comment

rudolfix left a comment

Choose a reason for hiding this comment

sh-rp commented Nov 22, 2023 •

edited

Loading

netlify bot commented Nov 22, 2023 •

edited

Loading

sh-rp commented Nov 22, 2023 •

edited

Loading

sh-rp commented Nov 27, 2023 •

edited

Loading