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

[maximearmstrong]

Summary & Motivation

This PR implements PowerBITranslatorData, a subclass of PowerBIContentData, 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]

• [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]

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]

Deploy preview for dagster-docs ready!

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

Direct link to changed pages:

• https://dagster-docs-mur16xmts-elementl.vercel.app
  https://maxime-move-translator-context-to-translator-props-class-powerb.dagster.dagster-docs.io/integrations/airbyte/airbyte-cloud

[maximearmstrong]

@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
• 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]

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

[maximearmstrong]

Done in 7c63440

[schrockn]

Container class works for me!