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

[maximearmstrong]

Summary & Motivation

This PR is motivated by this discussion

Context:

Translators for BI integrations required the contextual data from the workspace to create the asset specs. To pass this data to the translator, we ask users to pass the type of the translator to the spec loader, and we initialize the translator with its context in the state-back defs.

This approach works well, but is not flexible for users that want to customize their translators - users often customize the __init__ method, but passing this approach prevent them to initialize the translator with the correct arguments.

We want to update BI integrations so that users can pass an instance of the translator to spec loaders, which is more flexible, instead of the type.

Challenges:

• the translator object should to be immutable
• users can customize __init__ method and add new parameters to its signature, which are unknown to us
• once a context is set in a translator instance, it should not change
• keep this as simple as possible for users

Solution:

Use the copy_with_context method to rebuild a translator with its context.

To make this possible we need to:

• infer the __init__ kwargs from the attributes and the __init__ signature, using _get_init_kwargs_from_instance
• have matching attributes and params in the translator
• have the context as a parameter in the __init__ method of a custom translator

So, to make this possible, we require a few things from users:

• the signature of the custom __init__ method must include the context parameter
• Every attribute or private attribute must match their __init__ param, eg. the attribute for my_param can be my_param or _my_param

class MyCustomTranslator(DagsterPowerBITranslator):
    def __init__(self, my_param: str, context: Optional[PowerBIWorkspaceData] = None):
        self.my_param = my_param
        super().__init__(context=context)

    def get_asset_spec(self, data: PowerBIContentData) -> AssetSpec:
        default_spec = super().get_asset_spec(data)
        return default_spec.merge_attributes(
            metadata={"custom": self.my_param},
        )
 
# user code
translator = MyCustomTranslator(my_param=test_param)

# internal code
translator.copy_with_context(
    context=context
)
asset_spec = translator.get_asset_spec(...)

Proposals that were not retained:

1. Pass a callable factory to construct a translator, see here - this complexifies the user code
2. Pass the context to get_asset_spec, see here - this is breaking change that we would like to avoid.
3. Set the context on a translator after it’s initialization, see here - that makes the object mutable

How I Tested These Changes

New tests with BK

Changelog

Insert changelog entry or delete this section.

[maximearmstrong]

• [RFC][dagster-powerbi] Implement copy_with_context in DagsterPowerBITranslator #26617 👈 (View in Graphite)
• master

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

[maximearmstrong]

State-backed defs and spec loader to accept a translator instance in a subsequent PR.

[OwenKephart]

I'm on board with all the stuff about passing an instance of the translator etc.

However, I think it's probably too sketchy to try to infer the init arguments and reconstruct the class etc.

It'd work for a lot of cases but I think it's not intuitive for the user that their custom init function would get called twice, and I don't have high confidence that there isn't some sneaky way of breaking this with fancy parameter specs (like **kwargs etc.)

We usually avoid mutable objects (for good reason), but I think this is a prime example of a place where that'd be a better solution.

We could just have:

class DagsterPowerBITranslator:
    _context: Optional[PowerBiWorkspaceData]

    def set_context(self, context: PowerBiWorkspaceData) -> None:
        self._context = context

    @property
    def context(self) -> PowerBIWorkspaceData:
        return check.not_none(self._context, "helpful message explaining you probbably called a method outside of framework code")

This lets us avoid all the fancy inspect stuff and is much easier to wrap your head around (imo)

[maximearmstrong]

However, I think it's probably too sketchy to try to infer the init arguments and reconstruct the class etc.

It'd work for a lot of cases but I think it's not intuitive for the user that their custom init function would get called twice, and I don't have high confidence that there isn't some sneaky way of breaking this with fancy parameter specs (like **kwargs etc.)

We usually avoid mutable objects (for good reason), but I think this is a prime example of a place where that'd be a better solution.

@OwenKephart I agree with you 100%.

I just updated the description of this PR to add proposals that were not retained. Your proposal was previously suggested, but not retained because it would make the object mutable.

That said, I agree that inferring the init arguments from the instance is error-prone.

@schrockn Considering the implementation it requires to make the object immutable, do you think it would be a reasonable trade-off to use a setter and make this object mutable? I think there'd be a lot less risk of error.

[maximearmstrong]

Closing this RFC - we found an alternative, see here.