From b0f7815349891991ce5e93e164357517df2d4f54 Mon Sep 17 00:00:00 2001 From: Maxime Armstrong Date: Fri, 27 Dec 2024 12:11:58 -0500 Subject: [PATCH 1/4] [dagster-tableau] Use Tableau translator instance in spec loader and state-backed defs --- .../tableau/customize-tableau-asset-defs.py | 3 ++- .../dagster-tableau/dagster_tableau/resources.py | 15 +++++++-------- .../dagster_tableau_tests/test_asset_specs.py | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py b/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py index c2b3b2293032d..e3af4a5b524dd 100644 --- a/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py +++ b/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py @@ -37,6 +37,7 @@ def get_asset_spec(self, data: TableauContentData) -> dg.AssetSpec: tableau_specs = load_tableau_asset_specs( - tableau_workspace, dagster_tableau_translator=MyCustomTableauTranslator + tableau_workspace, + dagster_tableau_translator=MyCustomTableauTranslator, # type: ignore ) defs = dg.Definitions(assets=[*tableau_specs], resources={"tableau": tableau_workspace}) diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py b/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py index b81e22ecc350f..cffe96dac3993 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py @@ -533,14 +533,15 @@ def build_defs( @experimental def load_tableau_asset_specs( workspace: BaseTableauWorkspace, - dagster_tableau_translator: Type[DagsterTableauTranslator] = DagsterTableauTranslator, + dagster_tableau_translator: Optional[DagsterTableauTranslator] = None, ) -> Sequence[AssetSpec]: """Returns a list of AssetSpecs representing the Tableau content in the workspace. Args: workspace (Union[TableauCloudWorkspace, TableauServerWorkspace]): The Tableau workspace to fetch assets from. - dagster_tableau_translator (Type[DagsterTableauTranslator]): The translator to use - to convert Tableau content into AssetSpecs. Defaults to DagsterTableauTranslator. + dagster_tableau_translator (Optional[DagsterTableauTranslator]): The translator to use + to convert Tableau content into :py:class:`dagster.AssetSpec`. + Defaults to :py:class:`DagsterTableauTranslator`. Returns: List[AssetSpec]: The set of assets representing the Tableau content in the workspace. @@ -549,7 +550,7 @@ def load_tableau_asset_specs( return check.is_list( TableauWorkspaceDefsLoader( workspace=initialized_workspace, - translator_cls=dagster_tableau_translator, + translator=dagster_tableau_translator or DagsterTableauTranslator(), ) .build_defs() .assets, @@ -598,7 +599,7 @@ def build_client(self) -> None: @record class TableauWorkspaceDefsLoader(StateBackedDefinitionsLoader[Mapping[str, Any]]): workspace: BaseTableauWorkspace - translator_cls: Type[DagsterTableauTranslator] + translator: Type[DagsterTableauTranslator] @property def defs_key(self) -> str: @@ -608,8 +609,6 @@ def fetch_state(self) -> TableauWorkspaceData: return self.workspace.fetch_tableau_workspace_data() def defs_from_state(self, state: TableauWorkspaceData) -> Definitions: - translator = self.translator_cls() - all_external_data = [ *state.data_sources_by_id.values(), *state.sheets_by_id.values(), @@ -617,7 +616,7 @@ def defs_from_state(self, state: TableauWorkspaceData) -> Definitions: ] all_external_asset_specs = [ - translator.get_asset_spec( + self.translator.get_asset_spec( TableauTranslatorData(content_data=content, workspace_data=state) ) for content in all_external_data diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py index 899ed631ba35a..ca1a0b6ca5ab2 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py @@ -214,7 +214,7 @@ def test_translator_custom_metadata( resource.build_client() all_asset_specs = load_tableau_asset_specs( - workspace=resource, dagster_tableau_translator=MyCustomTranslator + workspace=resource, dagster_tableau_translator=MyCustomTranslator() ) asset_spec = next(spec for spec in all_asset_specs) From 2f58f9e389545b5bcd13d1438aad962ca5403221 Mon Sep 17 00:00:00 2001 From: Maxime Armstrong Date: Fri, 27 Dec 2024 14:47:28 -0500 Subject: [PATCH 2/4] Fix pyright and code --- docs/content/integrations/tableau.mdx | 3 ++- .../libraries/dagster-tableau/dagster_tableau/resources.py | 4 ++-- .../dagster_tableau_tests/test_reconstruction.py | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/content/integrations/tableau.mdx b/docs/content/integrations/tableau.mdx index 2c4ef2c89da6a..1325c7c02e452 100644 --- a/docs/content/integrations/tableau.mdx +++ b/docs/content/integrations/tableau.mdx @@ -143,7 +143,8 @@ class MyCustomTableauTranslator(DagsterTableauTranslator): tableau_specs = load_tableau_asset_specs( - tableau_workspace, dagster_tableau_translator=MyCustomTableauTranslator + tableau_workspace, + dagster_tableau_translator=MyCustomTableauTranslator, ) defs = dg.Definitions(assets=[*tableau_specs], resources={"tableau": tableau_workspace}) ``` diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py b/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py index cffe96dac3993..2190165d5c2a7 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py @@ -503,7 +503,7 @@ def build_defs( resource_key = "tableau" - asset_specs = load_tableau_asset_specs(self, dagster_tableau_translator) + asset_specs = load_tableau_asset_specs(self, dagster_tableau_translator()) non_executable_asset_specs = [ spec @@ -599,7 +599,7 @@ def build_client(self) -> None: @record class TableauWorkspaceDefsLoader(StateBackedDefinitionsLoader[Mapping[str, Any]]): workspace: BaseTableauWorkspace - translator: Type[DagsterTableauTranslator] + translator: DagsterTableauTranslator @property def defs_key(self) -> str: diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py index 788f0e261ed29..90db55f90e2f5 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py @@ -17,7 +17,7 @@ from dagster_tableau.asset_utils import parse_tableau_external_and_materializable_asset_specs from dagster_tableau.assets import build_tableau_materializable_assets_definition from dagster_tableau.resources import TableauCloudWorkspace, load_tableau_asset_specs -from dagster_tableau.translator import DagsterTableauTranslator +from dagster_tableau.translator import DagsterTableauTranslator, TableauTranslatorData from dagster_tableau_tests.conftest import ( FAKE_CONNECTED_APP_CLIENT_ID, @@ -67,12 +67,12 @@ def cacheable_asset_defs_refreshable_workbooks(): @lazy_definitions def cacheable_asset_defs_custom_translator(): class MyCoolTranslator(DagsterTableauTranslator): - def get_asset_spec(self, data) -> AssetSpec: + def get_asset_spec(self, data: TableauTranslatorData) -> AssetSpec: default_spec = super().get_asset_spec(data) return default_spec.replace_attributes(key=default_spec.key.with_prefix("my_prefix")) tableau_specs = load_tableau_asset_specs( - workspace=resource, dagster_tableau_translator=MyCoolTranslator + workspace=resource, dagster_tableau_translator=MyCoolTranslator() ) return Definitions(assets=[*tableau_specs], jobs=[define_asset_job("all_asset_job")]) From ec0c8f155ae0c8a817ea5030853dbef493316a14 Mon Sep 17 00:00:00 2001 From: Maxime Armstrong Date: Thu, 2 Jan 2025 13:35:21 -0500 Subject: [PATCH 3/4] Add deprecation warning --- .../tableau/customize-tableau-asset-defs.py | 2 +- .../dagster_tableau/resources.py | 19 +++++-- .../dagster_tableau_tests/test_asset_specs.py | 51 +++++++++++++++++++ .../test_reconstruction.py | 15 ++++++ 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py b/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py index e3af4a5b524dd..7db55adaf5529 100644 --- a/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py +++ b/examples/docs_snippets/docs_snippets/integrations/tableau/customize-tableau-asset-defs.py @@ -38,6 +38,6 @@ def get_asset_spec(self, data: TableauContentData) -> dg.AssetSpec: tableau_specs = load_tableau_asset_specs( tableau_workspace, - dagster_tableau_translator=MyCustomTableauTranslator, # type: ignore + dagster_tableau_translator=MyCustomTableauTranslator, ) defs = dg.Definitions(assets=[*tableau_specs], resources={"tableau": tableau_workspace}) diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py b/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py index 2190165d5c2a7..969f36d9fb65e 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau/resources.py @@ -23,6 +23,7 @@ from dagster._core.definitions.definitions_load_context import StateBackedDefinitionsLoader from dagster._record import record from dagster._utils.cached_method import cached_method +from dagster._utils.warnings import deprecation_warning from pydantic import Field, PrivateAttr from tableauserverclient.server.endpoint.auth_endpoint import Auth @@ -533,19 +534,31 @@ def build_defs( @experimental def load_tableau_asset_specs( workspace: BaseTableauWorkspace, - dagster_tableau_translator: Optional[DagsterTableauTranslator] = None, + dagster_tableau_translator: Optional[ + Union[DagsterTableauTranslator, Type[DagsterTableauTranslator]] + ] = None, ) -> Sequence[AssetSpec]: """Returns a list of AssetSpecs representing the Tableau content in the workspace. Args: workspace (Union[TableauCloudWorkspace, TableauServerWorkspace]): The Tableau workspace to fetch assets from. - dagster_tableau_translator (Optional[DagsterTableauTranslator]): The translator to use - to convert Tableau content into :py:class:`dagster.AssetSpec`. + dagster_tableau_translator (Optional[Union[DagsterTableauTranslator, Type[DagsterTableauTranslator]]]): + The translator to use to convert Tableau content into :py:class:`dagster.AssetSpec`. Defaults to :py:class:`DagsterTableauTranslator`. Returns: List[AssetSpec]: The set of assets representing the Tableau content in the workspace. """ + if isinstance(dagster_tableau_translator, type): + deprecation_warning( + subject="Support of `dagster_tableau_translator` as a Type[DagsterTableauTranslator]", + breaking_version="1.10", + additional_warn_text=( + "Pass an instance of DagsterTableauTranslator or subclass to `dagster_tableau_translator` instead." + ), + ) + dagster_tableau_translator = dagster_tableau_translator() + with workspace.process_config_and_initialize_cm() as initialized_workspace: return check.is_list( TableauWorkspaceDefsLoader( diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py index ca1a0b6ca5ab2..e92b284a7b14e 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py @@ -222,3 +222,54 @@ def test_translator_custom_metadata( assert asset_spec.metadata["custom"] == "metadata" assert asset_spec.key.path == ["prefix", "superstore_datasource"] assert asset_spec.tags["dagster/storage_kind"] == "tableau" + + +@responses.activate +@pytest.mark.parametrize( + "clazz,host_key,host_value", + [ + (TableauServerWorkspace, "server_name", "fake_server_name"), + (TableauCloudWorkspace, "pod_name", "fake_pod_name"), + ], +) +@pytest.mark.usefixtures("site_name") +@pytest.mark.usefixtures("sign_in") +@pytest.mark.usefixtures("get_workbooks") +@pytest.mark.usefixtures("get_workbook") +def test_translator_custom_metadata_legacy( + clazz: Union[Type[TableauCloudWorkspace], Type[TableauServerWorkspace]], + host_key: str, + host_value: str, + site_name: str, + sign_in: MagicMock, + get_workbooks: MagicMock, + get_workbook: MagicMock, +) -> None: + connected_app_client_id = uuid.uuid4().hex + connected_app_secret_id = uuid.uuid4().hex + connected_app_secret_value = uuid.uuid4().hex + username = "fake_username" + + with environ({"TABLEAU_CLIENT_ID": connected_app_client_id}): + resource_args = { + "connected_app_client_id": EnvVar("TABLEAU_CLIENT_ID"), + "connected_app_secret_id": connected_app_secret_id, + "connected_app_secret_value": connected_app_secret_value, + "username": username, + "site_name": site_name, + host_key: host_value, + } + + resource = clazz(**resource_args) + resource.build_client() + + # Pass the translator type + all_asset_specs = load_tableau_asset_specs( + workspace=resource, dagster_tableau_translator=MyCustomTranslator + ) + asset_spec = next(spec for spec in all_asset_specs) + + assert "custom" in asset_spec.metadata + assert asset_spec.metadata["custom"] == "metadata" + assert asset_spec.key.path == ["prefix", "superstore_datasource"] + assert asset_spec.tags["dagster/storage_kind"] == "tableau" diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py index 90db55f90e2f5..752cbd4391986 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py @@ -78,6 +78,21 @@ def get_asset_spec(self, data: TableauTranslatorData) -> AssetSpec: return Definitions(assets=[*tableau_specs], jobs=[define_asset_job("all_asset_job")]) +@lazy_definitions +def cacheable_asset_defs_custom_translator_legacy(): + class MyCoolTranslator(DagsterTableauTranslator): + def get_asset_spec(self, data: TableauTranslatorData) -> AssetSpec: + default_spec = super().get_asset_spec(data) + return default_spec.replace_attributes(key=default_spec.key.with_prefix("my_prefix")) + + # Pass the translator type + tableau_specs = load_tableau_asset_specs( + workspace=resource, dagster_tableau_translator=MyCoolTranslator + ) + + return Definitions(assets=[*tableau_specs], jobs=[define_asset_job("all_asset_job")]) + + def test_load_assets_workspace_data_refreshable_workbooks( sign_in: MagicMock, get_workbooks: MagicMock, From ed6fca90c7d0bfb5598335626fcf871f0cadafa9 Mon Sep 17 00:00:00 2001 From: Maxime Armstrong Date: Thu, 2 Jan 2025 15:03:33 -0500 Subject: [PATCH 4/4] Update post review --- .../dagster_tableau_tests/test_asset_specs.py | 10 ++++-- .../test_reconstruction.py | 35 +++++++++++++++++-- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py index e92b284a7b14e..37a92ffdc54de 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_asset_specs.py @@ -264,9 +264,13 @@ def test_translator_custom_metadata_legacy( resource.build_client() # Pass the translator type - all_asset_specs = load_tableau_asset_specs( - workspace=resource, dagster_tableau_translator=MyCustomTranslator - ) + with pytest.warns( + DeprecationWarning, + match=r"Support of `dagster_tableau_translator` as a Type\[DagsterTableauTranslator\]", + ): + all_asset_specs = load_tableau_asset_specs( + workspace=resource, dagster_tableau_translator=MyCustomTranslator + ) asset_spec = next(spec for spec in all_asset_specs) assert "custom" in asset_spec.metadata diff --git a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py index 752cbd4391986..21eb9ce4e7142 100644 --- a/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py +++ b/python_modules/libraries/dagster-tableau/dagster_tableau_tests/test_reconstruction.py @@ -1,5 +1,6 @@ from unittest.mock import MagicMock +import pytest from dagster._core.code_pointer import CodePointer from dagster._core.definitions.asset_spec import AssetSpec from dagster._core.definitions.definitions_class import Definitions @@ -86,9 +87,13 @@ def get_asset_spec(self, data: TableauTranslatorData) -> AssetSpec: return default_spec.replace_attributes(key=default_spec.key.with_prefix("my_prefix")) # Pass the translator type - tableau_specs = load_tableau_asset_specs( - workspace=resource, dagster_tableau_translator=MyCoolTranslator - ) + with pytest.warns( + DeprecationWarning, + match=r"Support of `dagster_tableau_translator` as a Type\[DagsterTableauTranslator\]", + ): + tableau_specs = load_tableau_asset_specs( + workspace=resource, dagster_tableau_translator=MyCoolTranslator + ) return Definitions(assets=[*tableau_specs], jobs=[define_asset_job("all_asset_job")]) @@ -208,3 +213,27 @@ def test_load_assets_workspace_data_translator( assert all( key.path[0] == "my_prefix" for key in repository_def.assets_defs_by_key.keys() ), repository_def.assets_defs_by_key + + +def test_load_assets_workspace_data_translator_legacy( + sign_in: MagicMock, + get_workbooks: MagicMock, + get_workbook: MagicMock, + get_view: MagicMock, + get_job: MagicMock, + refresh_workbook: MagicMock, + cancel_job: MagicMock, +) -> None: + with instance_for_test() as _instance: + repository_def = initialize_repository_def_from_pointer( + pointer=CodePointer.from_python_file( + __file__, + "cacheable_asset_defs_custom_translator_legacy", + None, + ) + ) + + assert len(repository_def.assets_defs_by_key) == 3 + assert all( + key.path[0] == "my_prefix" for key in repository_def.assets_defs_by_key.keys() + ), repository_def.assets_defs_by_key