Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DagsterSlingTranslator does not recognise target_prefix argument #20485

Closed
temminks opened this issue Mar 14, 2024 · 3 comments · Fixed by #20557
Closed

DagsterSlingTranslator does not recognise target_prefix argument #20485

temminks opened this issue Mar 14, 2024 · 3 comments · Fixed by #20557
Assignees
Labels
integration: embedded-elt Related to dagster-embedded-elt which uses Sling and data Load Tool (dlt) type: bug Something isn't working

Comments

@temminks
Copy link

Dagster version

1.6.9

What's the issue?

I copied the replication.yaml from the embedded-elt docs and tried to customise the target asset key's prefix, using the target_prefix argument in DagsterSlingTranslator(). The parameter defaults to target.

@sling_assets(replication_config=replication_config)
def gfdi_to_transformator_assets(context, sling: SlingResource):
    yield from sling.replicate(
        replication_config=replication_config,
        dagster_sling_translator=DagsterSlingTranslator(target_prefix="my_prefix"),
    )

I'd expect the asset's key to be prefixed with my_prefix, but the prefix stays target. This is unexpected and different from what the documentation describes.

Bildschirmfoto 2024-03-14 um 18 18 33

What did you expect to happen?

The prefix of the asset should be set to the value provided in target_prefix of the DagsterSlingTranslator.

How to reproduce?

Follow the documentation for Dagster Embedded ELT (https://docs.dagster.io/integrations/embedded-elt). and try setting the target_prefix parameter.

Deployment type

Local

Deployment details

No response

Additional information

No response

Message from the maintainers

Impacted by this issue? Give it a 👍! We factor engagement into prioritization.

@temminks temminks added the type: bug Something isn't working label Mar 14, 2024
@garethbrickman garethbrickman added the integration: embedded-elt Related to dagster-embedded-elt which uses Sling and data Load Tool (dlt) label Mar 14, 2024
@cmpadden cmpadden self-assigned this Mar 19, 2024
@cmpadden
Copy link
Contributor

cmpadden commented Mar 19, 2024

EDIT: disregard this message, and see the comment below!


Hi @temminks - thanks again for reporting this.

If you override the class without the @dataclass decorator, then the overridden target_prefix attribute will not be set, however, the use you showed with DagsterSlingTranslator(target_prefix='custom') seems like it should work -- I will investigate further.

from dataclasses import dataclass

@dataclass
class ExampleTranslator:
    target_prefix: str = "target"


t1 = ExampleTranslator()
t1.target_prefix
# 'target'


@dataclass  # <-- with dataclass decorator
class Custom(ExampleTranslator):
    target_prefix: str = "custom"

t2 = Custom()
t2.target_prefix
# 'custom' <-- 👍 


class CustomNotDataclass(DagsterSlingTranslator):
    target_prefix: str = "custom"

t3 = CustomNotDataclass()

t3.target_prefix
# 'target' <-- 👎 

t4 = DagsterSlingTranslator(target_prefix='custom')
t4.target_prefix
# 'custom'  <-- 👍 

@cmpadden
Copy link
Contributor

cmpadden commented Mar 19, 2024

@temminks, Ok, I think I've found the issue with what you've shared above. Both @sling_assets and replicate() take a DagsterSlingTranslator implementation. You've only added it to the replicate call. If you change this to the following it should work as expected.

# before
@sling_assets(replication_config=replication_config)
def gfdi_to_transformator_assets(context, sling: SlingResource):
    yield from sling.replicate(
        replication_config=replication_config,
        dagster_sling_translator=DagsterSlingTranslator(target_prefix="my_prefix"),
    )
   
# after
custom_dagster_sling_translator = DagsterSlingTranslator(target_prefix="my_prefix")
@sling_assets(replication_config=replication_config, dagster_sling_translator=custom_dagster_sling_translator)
def gfdi_to_transformator_assets(context, sling: SlingResource):
    yield from sling.replicate(
        replication_config=replication_config,
        dagster_sling_translator=custom_dagster_sling_translator,
    )

I believe that we can remove the dependency to pass the translator in two locations; I will create a ticket to track such an enhancement.

@cmpadden
Copy link
Contributor

Just to follow up here, a code change was pushed that makes it so you only have to specify the translator and replication config in the asset decorator. This is to avoid the confusion of the above scenario. You can see the details here:

#20564

This change also makes it so you can use the Sling integration from an @op directly if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration: embedded-elt Related to dagster-embedded-elt which uses Sling and data Load Tool (dlt) type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants