[dagster-sigma] Detach spec loading from Sigma resource #25431

benpankow · 2024-10-22T17:08:51Z

Summary

As part of migrating to the APIs mocked out in https://www.notion.so/dagster/Asset-integration-customization-options-11a18b92e462807c94c2f84675021120 and https://www.notion.so/dagster/Power-BI-semantic-model-API-options-11918b92e4628006a25af3f58a6b48d9, detaches the asset spec loading process from the resource.

resource = SigmaOrganization(
    base_url=SigmaBaseUrl.AWS_US,
    client_id=EnvVar("SIGMA_CLIENT_ID"),
    client_secret=EnvVar("SIGMA_CLIENT_SECRET"),
)

sigma_specs = load_sigma_asset_specs(resource, dagster_sigma_translator=MyCoolTranslator)

How I Tested These Changes

New unit test, updated existing unit tests.

benpankow · 2024-10-22T17:09:08Z

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

Join @benpankow and the rest of your teammates on Graphite

schrockn · 2024-10-22T17:23:12Z

python_modules/libraries/dagster-sigma/dagster_sigma/resource.py

@@ -61,6 +65,9 @@ class SigmaOrganization(ConfigurableResource):
        default=False,
        description="Whether to warn rather than raise when lineage data cannot be fetched for an element.",
    )
+    translator: ResourceDependency[Type[DagsterSigmaTranslator]] = Field(


hmmmm why are we making this a resource dependency? Seems like the translator should just be a vanilla object/property

Right now all params on a resource must be either a config type or a nested resource, e.g.

E dagster._core.errors.DagsterInvalidPythonicConfigDefinitionError: E Error defining Dagster config class <class 'dagster_sigma.resource.SigmaOrganization'> on field 'translator'. E Unable to resolve config type typing.Type[dagster_sigma.translator.DagsterSigmaTranslator] to a supported Dagster config type. E E E This config type can be a: E - Python primitive type E - int, float, bool, str, list E - A Python Dict or List type containing other valid types E - Custom data classes extending dagster.Config E - A Pydantic discriminated union type (https://docs.pydantic.dev/usage/types/#discriminated-unions-aka-tagged-unions) E E E If this config type represents a resource dependency, its annotation must either: E - Extend dagster.ConfigurableResource, dagster.ConfigurableIOManager, or E - Be wrapped in a ResourceDependency annotation, e.g. ResourceDependency[Type]

The actual passed translator can be a plain-old-python-object (a lowercase-r "resource", not a ConfigurableResource) - the ResourceDependency here just insists that it is not treated as config.

schrockn

Skeptical of attaching the translator to a resource in this way as it seems confusing to me.

q

schrockn

Can we just have load_sigma_asset_specs take a translator as an argument?

schrockn · 2024-10-22T23:23:22Z

python_modules/libraries/dagster-sigma/dagster_sigma_tests/pending_repo_with_translator.py

+    resource = SigmaOrganization(
+        base_url=SigmaBaseUrl.AWS_US,
+        client_id=EnvVar("SIGMA_CLIENT_ID"),
+        client_secret=EnvVar("SIGMA_CLIENT_SECRET"),
+        translator=MyCoolTranslator,
+    )
+
+    sigma_specs = load_sigma_asset_specs(resource)
+    defs = Definitions(assets=[*sigma_specs], jobs=[define_asset_job("all_asset_job")])


Given that in all the examples that the organization is just used during the load process and not attached to the actual Definitions object I'm skeptical that it should be a resource at all. I think it adds a lot of complexity for limited benefit.

schrockn

to your q for discussion

schrockn

Decision:

Split off translator from the resource and pass it to the load function as a separate argument. The translator is only needed during the definitions load process.
Keep SigmaOrganization as a resource but in the examples pass it in the resources dictionary so that it is visible in the UI

q

python_modules/libraries/dagster-sigma/dagster_sigma/resource.py

schrockn

One last thing: I think we should standardize on translate instance.

queue

maximearmstrong · 2024-10-24T19:33:33Z

python_modules/libraries/dagster-sigma/dagster_sigma/resource.py

+    """Returns a list of AssetSpecs representing the Sigma content in the organization.
+
+    Args:
+        organization (SigmaOrganization): The Sigma organization to fetch assets from.


nit: can we add the docstring for dagster_sigma_translator?

benpankow · 2024-10-24T20:07:04Z

Merge activity

Oct 24, 1:07 PM PDT: A user started a stack merge that includes this pull request via Graphite.
Oct 24, 1:08 PM PDT: Graphite rebased this pull request as part of a merge.
Oct 24, 1:09 PM PDT: A user merged this pull request with Graphite.

…tor to resource

## Summary As part of migrating to the APIs mocked out in https://www.notion.so/dagster/Asset-integration-customization-options-11a18b92e462807c94c2f84675021120 and https://www.notion.so/dagster/Power-BI-semantic-model-API-options-11918b92e4628006a25af3f58a6b48d9, detaches the asset spec loading process from the resource. ```python resource = SigmaOrganization( base_url=SigmaBaseUrl.AWS_US, client_id=EnvVar("SIGMA_CLIENT_ID"), client_secret=EnvVar("SIGMA_CLIENT_SECRET"), ) sigma_specs = load_sigma_asset_specs(resource, dagster_sigma_translator=MyCoolTranslator) ``` ## How I Tested These Changes New unit test, updated existing unit tests.

…5431) ## Summary As part of migrating to the APIs mocked out in https://www.notion.so/dagster/Asset-integration-customization-options-11a18b92e462807c94c2f84675021120 and https://www.notion.so/dagster/Power-BI-semantic-model-API-options-11918b92e4628006a25af3f58a6b48d9, detaches the asset spec loading process from the resource. ```python resource = SigmaOrganization( base_url=SigmaBaseUrl.AWS_US, client_id=EnvVar("SIGMA_CLIENT_ID"), client_secret=EnvVar("SIGMA_CLIENT_SECRET"), ) sigma_specs = load_sigma_asset_specs(resource, dagster_sigma_translator=MyCoolTranslator) ``` ## How I Tested These Changes New unit test, updated existing unit tests.

benpankow requested review from schrockn and maximearmstrong October 22, 2024 17:20

benpankow mentioned this pull request Oct 22, 2024

[dagster-sigma] Standardize translator signature #25432

Merged

schrockn reviewed Oct 22, 2024

View reviewed changes

schrockn previously requested changes Oct 22, 2024

View reviewed changes

benpankow mentioned this pull request Oct 22, 2024

[dagster-sigma] Implement materializing workbooks #25433

Draft

benpankow force-pushed the benpankow/asdf branch from 4b65182 to 78db26b Compare October 22, 2024 21:29

benpankow changed the base branch from master to benpankow/pending-repo-load-test October 22, 2024 21:29

benpankow mentioned this pull request Oct 22, 2024

Stash, retrieve StateBackedDefinitionsLoader reconstruction data in DefinitionsLoadContext #25436

Merged

benpankow requested a review from schrockn October 22, 2024 21:50

schrockn reviewed Oct 22, 2024

View reviewed changes

schrockn previously requested changes Oct 22, 2024

View reviewed changes

schrockn reviewed Oct 23, 2024

View reviewed changes

benpankow force-pushed the benpankow/asdf branch 2 times, most recently from 8fbbed9 to eeb1c8a Compare October 23, 2024 05:03

benpankow requested a review from schrockn October 23, 2024 05:56

schrockn reviewed Oct 23, 2024

View reviewed changes

python_modules/libraries/dagster-sigma/dagster_sigma/resource.py Outdated Show resolved Hide resolved

schrockn previously requested changes Oct 23, 2024

View reviewed changes

Base automatically changed from benpankow/pending-repo-load-test to master October 23, 2024 15:12

benpankow changed the title ~~[dagster-sigma] Detach spec loading from Sigma resource, bind translator to resource~~ [dagster-sigma] Detach spec loading from Sigma resource Oct 23, 2024

benpankow force-pushed the benpankow/asdf branch from f6da204 to c20033f Compare October 23, 2024 16:49

benpankow requested a review from schrockn October 23, 2024 17:23

benpankow force-pushed the benpankow/asdf branch 2 times, most recently from aadbeb5 to 9bc6079 Compare October 24, 2024 17:52

maximearmstrong approved these changes Oct 24, 2024

View reviewed changes

benpankow added 5 commits October 24, 2024 20:07

[dagster-sigma] Detach spec loading from Sigma resource, bind transla…

e7baa37

…tor to resource

ruff

1bda888

Move translator back to load fn

3e27f29

use callable instead

f6a3c11

callable

2f6818e

benpankow force-pushed the benpankow/asdf branch from 9bc6079 to 2f6818e Compare October 24, 2024 20:07

benpankow merged commit dd66c37 into master Oct 24, 2024
1 check was pending

benpankow deleted the benpankow/asdf branch October 24, 2024 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dagster-sigma] Detach spec loading from Sigma resource #25431

[dagster-sigma] Detach spec loading from Sigma resource #25431

benpankow commented Oct 22, 2024 •

edited

Loading

benpankow commented Oct 22, 2024 •

edited

Loading

schrockn Oct 22, 2024

benpankow Oct 22, 2024 •

edited

Loading

schrockn left a comment

schrockn left a comment

schrockn Oct 22, 2024

schrockn left a comment

schrockn left a comment

schrockn left a comment

maximearmstrong Oct 24, 2024 •

edited

Loading

benpankow commented Oct 24, 2024 •

edited

Loading

[dagster-sigma] Detach spec loading from Sigma resource #25431

[dagster-sigma] Detach spec loading from Sigma resource #25431

Conversation

benpankow commented Oct 22, 2024 • edited Loading

Summary

How I Tested These Changes

benpankow commented Oct 22, 2024 • edited Loading

schrockn Oct 22, 2024

Choose a reason for hiding this comment

benpankow Oct 22, 2024 • edited Loading

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

schrockn Oct 22, 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

schrockn left a comment

Choose a reason for hiding this comment

maximearmstrong Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

benpankow commented Oct 24, 2024 • edited Loading

Merge activity

benpankow commented Oct 22, 2024 •

edited

Loading

benpankow commented Oct 22, 2024 •

edited

Loading

benpankow Oct 22, 2024 •

edited

Loading

maximearmstrong Oct 24, 2024 •

edited

Loading

benpankow commented Oct 24, 2024 •

edited

Loading