[dagster-powerbi] Move contextual data from DagsterPowerBITranslator to PowerBITranslatorData #26654

maximearmstrong · 2024-12-20T22:36:04Z

Summary & Motivation

This PR implements PowerBITranslatorData, a container class containing a PowerBIContentData object (props) and a PowerBIWorkspaceData object (context), to pass contextual data to the translator without breaking the get_asset_spec signature or requiring context to be passed to the __init__ method of the DagsterPowerBITranslator.

This is PR is an alternative to the proposals mentioned in #26617.

How I Tested These Changes

Updated tests with BK

Changelog

[dagster-powerbi] Type hints in the signature of DagsterPowerBITranslator.get_asset_spec have been updated - the parameter data is now of type PowerBITranslatorData instead of PowerBIContentData. Custom Power BI translators should be updated.

maximearmstrong · 2024-12-20T22:36:22Z

[dagster-powerbi] Update Power BI docs/examples to use translator instance and PowerBITranslatorData #26735
[dagster-powerbi] Use Power BI translator instance in spec loader and state-backed defs #26734
[dagster-powerbi] Move contextual data from DagsterPowerBITranslator to PowerBITranslatorData #26654 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

schrockn · 2024-12-20T22:47:18Z

Is this every going to cause a recursive explosion of data?

I guess we are relying on the fact that PowerBIWorkspaceData will only ever hold references to PowerBIContentData rather than PowerBITranslatorData.

That's kind of why I was imagining a container class that holds a reference

class PowerBITranslatorData:
   content: PowerBIContentData
   workspace: PowerBIWorkspaceData

As it would make the possibility structurally impossible via the type system.

Overall this looks good though.

github-actions · 2024-12-26T19:53:56Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-1mngnv18l-elementl.vercel.app
https://maxime-move-translator-context-to-translator-props-class-powerb.dagster.dagster-docs.io

Direct link to changed pages:

maximearmstrong · 2024-12-26T20:13:16Z

@schrockn in theory, it's not a problem because PowerBIWorkspaceData is expected to be created with PowerBIContentData objects and not PowerBITranslatorData objects.

Currently, DagsterPowerBITranslator.get_asset_spec takes a PowerBIContentData object - Making PowerBITranslatorData a subclass of PowerBIContentData avoid breaking the signature.

That said, after thinking about this, we should not subclass PowerBIContentData for 2 reasons:

To make sure we avoid the recursion explosion of data, we would need some type checks in PowerBIWorkspaceData, which is kinda awkward
Adding these type checks is an anti-pattern and breaks the Liskov substitution principle in Python - instances of a subclass can be passed where instances of a class are expected without causing any problems.

I updated this PR to make PowerBITranslatorData a container class, and updated the signature of DagsterPowerBITranslator.get_asset_spec so that the data param is of type PowerBITranslatorData.

Here's why I think we can move forward with this approach:

After testing previous custom translators, this change does not raise an exception
Only type checkers like Pyright will fail with this change, if users are using one and if they use type hints in their custom translator. Note: the code doesn't break with this change, only type hints in the signature of DagsterPowerBITranslator.get_asset_spec.
The translator classes for BI integrations are not marked as experimental, but every callsite is (spec loader, asset def builder, etc.). Adding a changelog in this PR to state that the type hints in the signature of DagsterPowerBITranslator.get_asset_spec changed seems reasonable enough to warn users.

schrockn · 2024-12-26T23:04:28Z

python_modules/libraries/dagster-powerbi/dagster_powerbi/translator.py

@@ -78,6 +78,23 @@ class PowerBIContentData:
    properties: Dict[str, Any]


+class PowerBITranslatorData(NamedTuple):


can just make this @record but not serializable to be consistent with the rest of this interface.

Done in 7c63440

schrockn

Container class works for me!

schrockn

Hey @maximearmstrong I think one thing we could do here to make the transition a little easier would be to implement the properties of PowerBIContentData on PowerBITranslatorData. That way the code wouldn't break, just the the typehints.

Meaning

class PowerBITranslatorData:
    
   @property
   def content_type(self) -> PowerBIContentType:
      return self.content_data.content_type

maximearmstrong · 2025-01-02T14:50:27Z

Hey @maximearmstrong I think one thing we could do here to make the transition a little easier would be to implement the properties of PowerBIContentData on PowerBITranslatorData. That way the code wouldn't break, just the the typehints.

@schrockn Exactly - that's the current state of this PR.

The changes requested here were already implemented in bb5b904 when we switched to using a container class.

Only type checkers like Pyright will fail with this change, if users are using one and if they use type hints in their custom translator

I will update this part of this comment to emphasize that code does not break in the current state.

q

dpeng817 · 2025-01-02T17:52:31Z

examples/project_atproto_dashboard/project_atproto_dashboard/dashboard/definitions.py

@@ -21,7 +21,7 @@ class CustomDagsterPowerBITranslator(DagsterPowerBITranslator):
    def get_report_spec(self, data: PowerBIContentData) -> dg.AssetSpec:
        return (
            super()
-            .get_report_spec(data)
+            .get_report_spec(data)  # type: ignore


why new type: ignore?

dpeng817

looks good, let's see if we can get rid of the type ignore in the docs snippet

…to PowerBITranslatorData

schrockn

Please update PR summary to be accurate (still refers to subclassing)

…to TableauTranslatorData (#26739) ## Summary & Motivation Same as #26654 but for Tableau. ## How I Tested These Changes Updated tests with BK ## Changelog [dagster-tableau] Type hints in the signature of `DagsterTableauTranslator.get_asset_spec` have been updated - the parameter `data` is now of type `TableauTranslatorData` instead of `TableauContentData`. Custom Tableau translators should be updated.

… to LookerApiTranslatorStructureData (#26742) ## Summary & Motivation Same as #26654 but for Looker. ## How I Tested These Changes Updated tests with BK ## Changelog [dagster-looker] Type hints in the signature of `DagsterLookerApiTranslator.get_asset_spec` have been updated - the parameter `looker_structure` is now of type `LookerApiTranslatorStructureData` instead of `LookerStructureData`. Custom Looker API translators should be updated.

…ontainer classes (#26798) ## Summary & Motivation Same as #26654 but for Sigma. ## How I Tested These Changes Updated tests with BK ## Changelog [dagster-sigma] Type hints in the signature of `DagsterLookerApiTranslator.get_asset_spec` have been updated - the parameter `data` is now of type `Union[SigmaDatasetTranslatorData, SigmaWorkbookTranslatorData]` instead of `Union[SigmaDataset, SigmaWorkbook]`. Custom Looker API translators should be updated.

maximearmstrong marked this pull request as ready for review December 20, 2024 22:40

maximearmstrong self-assigned this Dec 20, 2024

maximearmstrong requested review from schrockn, OwenKephart, benpankow and dpeng817 December 20, 2024 22:40

maximearmstrong mentioned this pull request Dec 20, 2024

[RFC][dagster-powerbi] Implement copy_with_context in DagsterPowerBITranslator #26617

Closed

schrockn approved these changes Dec 20, 2024

View reviewed changes

maximearmstrong force-pushed the maxime/move-translator-context-to-translator-props-class-powerbi branch from cb21360 to 9ab65c3 Compare December 26, 2024 19:49

schrockn reviewed Dec 26, 2024

View reviewed changes

This was referenced Dec 27, 2024

[dagster-powerbi] Use Power BI translator instance in spec loader and state-backed defs #26734

Merged

[dagster-powerbi] Update Power BI docs/examples to use translator instance and PowerBITranslatorData #26735

Merged

maximearmstrong force-pushed the maxime/move-translator-context-to-translator-props-class-powerbi branch 3 times, most recently from adf4154 to 88115b1 Compare December 27, 2024 19:39

This was referenced Dec 27, 2024

[dagster-tableau] Move contextual data from DagsterTableauTranslator to TableauTranslatorData #26739

Merged

[dagster-looker] Move contextual data from DagsterLookerApiTranslator to LookerApiTranslatorStructureData #26742

Merged

schrockn previously requested changes Dec 30, 2024

View reviewed changes

maximearmstrong requested a review from schrockn January 2, 2025 14:53

dpeng817 reviewed Jan 2, 2025

View reviewed changes

dpeng817 approved these changes Jan 2, 2025

View reviewed changes

maximearmstrong added 2 commits January 2, 2025 13:07

[dagster-powerbi] Move contextual data from DagsterPowerBITranslator …

11da50c

…to PowerBITranslatorData

Make PowerBITranslatorData a container class

632d9d4

maximearmstrong added 4 commits January 2, 2025 13:07

Use record instead of namedtuple

e1aa285

Lint

8c5de80

Update tests

bcf4e44

Fix pyright

24fe471

maximearmstrong force-pushed the maxime/move-translator-context-to-translator-props-class-powerbi branch from 88115b1 to 24fe471 Compare January 2, 2025 18:27

schrockn approved these changes Jan 2, 2025

View reviewed changes

maximearmstrong merged commit 05d029a into master Jan 2, 2025
2 checks passed

maximearmstrong deleted the maxime/move-translator-context-to-translator-props-class-powerbi branch January 2, 2025 19:51

maximearmstrong mentioned this pull request Jan 3, 2025

[dagster-sigma] Move contextual data from DagsterSigmaTranslator to container classes #26798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dagster-powerbi] Move contextual data from DagsterPowerBITranslator to PowerBITranslatorData #26654

[dagster-powerbi] Move contextual data from DagsterPowerBITranslator to PowerBITranslatorData #26654

maximearmstrong commented Dec 20, 2024 •

edited

Loading

maximearmstrong commented Dec 20, 2024 •

edited

Loading

schrockn commented Dec 20, 2024

github-actions bot commented Dec 26, 2024 •

edited

Loading

maximearmstrong commented Dec 26, 2024 •

edited

Loading

schrockn Dec 26, 2024

maximearmstrong Dec 27, 2024

schrockn left a comment

schrockn left a comment

maximearmstrong commented Jan 2, 2025

dpeng817 Jan 2, 2025

dpeng817 left a comment

schrockn left a comment

		@@ -78,6 +78,23 @@ class PowerBIContentData:
		properties: Dict[str, Any]


		class PowerBITranslatorData(NamedTuple):

[dagster-powerbi] Move contextual data from DagsterPowerBITranslator to PowerBITranslatorData #26654

[dagster-powerbi] Move contextual data from DagsterPowerBITranslator to PowerBITranslatorData #26654

Conversation

maximearmstrong commented Dec 20, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

Changelog

maximearmstrong commented Dec 20, 2024 • edited Loading

schrockn commented Dec 20, 2024

github-actions bot commented Dec 26, 2024 • edited Loading

maximearmstrong commented Dec 26, 2024 • edited Loading

schrockn Dec 26, 2024

Choose a reason for hiding this comment

maximearmstrong Dec 27, 2024

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

maximearmstrong commented Jan 2, 2025

dpeng817 Jan 2, 2025

Choose a reason for hiding this comment

dpeng817 left a comment

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

maximearmstrong commented Dec 20, 2024 •

edited

Loading

maximearmstrong commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 26, 2024 •

edited

Loading

maximearmstrong commented Dec 26, 2024 •

edited

Loading