Move replace_attributes, merge_attributes onto AssetSpec, publicise #25941

benpankow · 2024-11-14T23:57:59Z

Summary

Now that they've baked for a little bit, moves the replace_attributes and merge_attributes utilities directly onto AssetSpec, and marks them public:

spec: AssetSpec = ...

updated_spec = spec.replace_attributes(description="A new description.")

spec: AssetSpec = ...

# add new python kind, does not affect existing kinds
updated_spec = spec.merge_attributes(kinds={"python"})

How I Tested These Changes

Update unit test.

Changelog

Introduced AssetSpec.replace_attributes and AssetSpec.merge_attributes to easily alter properties of an asset spec.

benpankow · 2024-11-14T23:58:18Z

Move replace_attributes, merge_attributes onto AssetSpec, publicise #25941 : 2 dependent PRs (#25942 , #26109 ) 👈 (View in Graphite)
master

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

github-actions · 2024-11-15T00:15:44Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-ezhxtrshe-elementl.vercel.app
https://benpankow-replace-attributes.dagster.dagster-docs.io

Direct link to changed pages:

OwenKephart

generally supportive, just want to be careful about old params

OwenKephart · 2024-11-15T17:07:59Z

python_modules/dagster/dagster/_core/definitions/asset_spec.py

+        skippable: bool = ...,
+        group_name: Optional[str] = ...,
+        code_version: Optional[str] = ...,
+        freshness_policy: Optional[FreshnessPolicy] = ...,


can we remove freshness_policy to avoid needing to go through a deprecation cycle ?

You just mean freshness_policy, right? or are we removing automation_condition too 👀

schrockn · 2024-11-20T14:55:13Z

Having a method makes sense. My only question before we introduce this is how we handle specs and AssetsDefinitions uniformly. Ideally we would have a one liner for that.

benpankow · 2024-11-20T16:39:50Z

My only question before we introduce this is how we handle specs and AssetsDefinitions uniformly. Ideally we would have a one liner for that.

I can think of a couple options:

Rename these methods to replace_spec_attributes, merge_spec_attributes and also define them on AssetsDefinition. This leaves us with very nice behavior in comprehensions:

my_specs_and_defs = ...

my_updated_specs_and_defs = [
  asset.replace_spec_attributes(owner="ben@dagsterlabs.com")
  for asset in my_specs_and_defs
]

Retain & publicize standalone methods which can operate on both specs and definitions, e.g. dg.replace_spec_attributes, dg.merge_spec_attributes. Under the hood, they just call spec.replace_attributes(...) or def.map_specs(lambda spec: spec.replace_attributes(...))

my_specs_and_defs = ...

my_updated_specs_and_defs = [
  dg.replace_spec_attributes(asset, owner="ben@dagsterlabs.com")
  for asset in my_specs_and_defs
]

# Can still call directly on specs
my_spec = ...
my_spec = my_spec.replace_attributes(...)

No change, use ternary in comprehension:

my_specs_and_defs = ...

my_updated_specs_and_defs = [
  asset.replace_attributes(owner="ben@dagsterlabs.com")
  for asset in my_specs_and_defs
  if isinstance(asset, AssetSpec)
  else
  asset.map_specs(lambda spec: spec.replace_attributes(owner="ben@dagsterlabs.com")
]

dpeng817 · 2024-11-20T17:07:55Z

@benpankow wrt unifying behavior around assetsdefinition and assetspec
I think replace_spec_attributes looks nice in theory but will get complicated quickly. EG, if you're often predicating on metadata tags etc which don't have a direct analog on assetsdefinition, you're still leaking the complexity

benpankow · 2024-11-20T17:31:09Z

I think replace_spec_attributes looks nice in theory but will get complicated quickly. EG, if you're often predicating on metadata tags etc which don't have a direct analog on assetsdefinition, you're still leaking the complexity

That's true - I do think the predicate case will be much less common in the mixed spec/definitions case, e.g. if you're conditionally changing properties, it's probably right after pulling in specs from an integration, since at that point you have a relatively homogenous set of specs where you can reason about what metadata you're predicating on.

That being said, maybe we want something like

def map_specs(func: Callable[[AssetSpec], AssetSpec], iter: Iterable[Union[AssetsDefinition, AssetSpec]]) -> Sequence[Union[AssetsDefinition, AssetSpec]]):
  return [obj.map_specs(func) for obj in iter if isinstance(x, AssetsDefinition) else func(obj)]

my_specs_and_defs = ...

my_predicate: Callable[[AssetSpec], bool] = ...

my_updated_specs_and_defs = dg.map_specs(
  lambda spec: spec.replace_attributes(owner="ben@dagsterlabs.com") if my_predicate(spec) else spec,
  my_specs_and_defs
)

This way, semantics match map(lambda spec: ..., ...) closely, just with auto-marshalling of specs into and out of definitions. This world would basically be a combination of this PR, as-is (adding customization utilities directly to AssetSpec) plus stacking map_specs.

dpeng817 · 2024-11-20T17:50:27Z

I find myself more convinced by map_specs than AssetsDefinition.replace_attributes - to our discussion in product review, I think replace_attributes on AssetsDefinition seems overly specified. We're wrapping a primitive with map_specs, true, but I don't think it actually removes any of the flexibility of that primitive.

The problem; I think we'll unfortunately end up with one of these for every kind of spec. IE; map_check_specs. I think that's preferable to the alternative though of forcing users to encounter this awkwardness.

schrockn · 2024-11-22T20:38:23Z

map_specs sounds great but I think it should be map_asset_specs.

schrockn

Let's do a standalone function map_asset_specs 👍🏻

benpankow · 2024-11-23T00:12:17Z

Introduced in #26109

queue

graphite-app · 2024-11-25T17:08:47Z

docs/content/integrations/airlift/federation-tutorial/observe.mdx

+customer_metrics_dag_asset = customer_metrics_dag_asset.replace_attributes(
    customer_metrics_dag_asset,
    deps=[load_customers],
 )


The replace_attributes() method is being called with customer_metrics_dag_asset as both the receiver and first argument. Since this method only accepts keyword arguments, the first argument should be removed:

customer_metrics_dag_asset = customer_metrics_dag_asset.replace_attributes( deps=[load_customers] )

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

## Summary Based on conversation in #25941, introduces a new `dg.map_asset_specs` function which can be used to map a user-provided function over a set of `AssetSpec`s and `AssetDefinitions`. ```python @multi_asset(specs=[ AssetSpec(key="foo"), AssetSpec(key="bar"), ]) def my_multi_asset(): ... my_specs_and_defs = [ my_multi_asset, AssetSpec(key="external") ] my_mapped_specs_and_defs = map_asset_specs( lambda spec: spec.replace_attributes(owners="ben@dagsterlabs.com"), my_specs_and_defs ) ``` ## Test Plan New unit tests. ## Changelog > Introduced dg.map_asset_specs to enable modifying `AssetSpec`s and `AssetsDefinition`s in bulk.

#26027) ## Summary & Motivation Following an internal discussion [here](dagster-io/internal#12670) and product review [here](https://www.notion.so/dagster/Product-Reviews-1805443bfd5d4d40b3f6be8c79897295?p=13f18b92e4628020bd6fd3ac45f28af0&pm=s), get_asset_spec().key is the new recommended way to access the asset key in the Translator. Asset keys can now be customized in a custom translator using `replace_attributes`, like: ```python class MyCustomTableauTranslator(DagsterTableauTranslator): def get_asset_spec(self, data: TableauContentData) -> dg.AssetSpec: default_spec = super().get_asset_spec(data) return replace_attributes( default_spec, key=default_spec.key.with_prefix("prefix"), ) ``` This PR stack will be merged after #25941 lands, so that `replace_attributes()` can be called as `AssetSpec().replace_attributes()` These changes will also be done to the following integrations: - Power BI - Sigma - Looker - dlt - Sling - sdf We will need to rework dbt to use this pattern. ## How I Tested These Changes Updated unit test with BK

…ranslator (#26028) ## Summary & Motivation Mark DagsterTableauTranslator.get_asset_key as deprecated. To be removed in 1.10. This PR stack will be merged after #25941 lands, so that `replace_attributes()` can be called as `AssetSpec().replace_attributes()` ## Changelog [dagster-tableau] `DagsterTableauTranslator.get_asset_key` is deprecated in favor of `DagsterTableauTranslator.get_asset_spec().key`

…26029) ## Summary & Motivation Update the custom translator example in the Tableau docs to reflect `get_asset_spec().key`. This PR stack will be merged after #25941 lands, so that `replace_attributes()` can be called as `AssetSpec().replace_attributes()`

) ## Summary Follow-on to #25941 which updates some non-type-checked `_replace` callsites to use these new methods.

benpankow mentioned this pull request Nov 15, 2024

Update callsites of AssetSpec._replace to use replace_attributes #25942

Merged

benpankow force-pushed the benpankow/replace-attributes branch from 5d7fb8c to e1756ba Compare November 15, 2024 00:31

benpankow requested review from maximearmstrong, dpeng817 and OwenKephart November 15, 2024 16:50

benpankow force-pushed the benpankow/replace-attributes branch from e1756ba to 5f4b4a9 Compare November 15, 2024 16:55

OwenKephart requested changes Nov 15, 2024

View reviewed changes

OwenKephart approved these changes Nov 15, 2024

View reviewed changes

benpankow requested a review from schrockn November 15, 2024 17:43

This was referenced Nov 20, 2024

[dagster-tableau] Update custom translator example in Tableau docs #26029

Merged

[dagster-tableau] Mark get_asset_key as deprecated in DagsterTableauTranslator #26028

Merged

[dagster-tableau] Use get_asset_spec().key in DagsterTableauTranslator #26027

Merged

schrockn previously requested changes Nov 22, 2024

View reviewed changes

benpankow force-pushed the benpankow/replace-attributes branch from bb94e62 to 04fb74c Compare November 23, 2024 00:11

benpankow requested a review from neverett as a code owner November 23, 2024 00:11

benpankow mentioned this pull request Nov 23, 2024

Introduce dg.map_asset_specs #26109

Merged

benpankow requested a review from schrockn November 23, 2024 00:12

schrockn approved these changes Nov 23, 2024

View reviewed changes

benpankow added 2 commits November 25, 2024 09:05

Move replace_attributes, merge_attributes onto AssetSpec, publicise

a6a81b5

doc

540825a

benpankow added 2 commits November 25, 2024 09:05

rm freshness condition, amp

89391a6

amc

8634ced

benpankow force-pushed the benpankow/replace-attributes branch from 04fb74c to 8634ced Compare November 25, 2024 17:08

graphite-app bot reviewed Nov 25, 2024

View reviewed changes

doc

9434b4d

benpankow merged commit 0de7987 into master Nov 25, 2024
1 of 2 checks passed

benpankow deleted the benpankow/replace-attributes branch November 25, 2024 17:52

benpankow added a commit that referenced this pull request Nov 26, 2024

Update callsites of AssetSpec._replace to use replace_attributes (#25942

94d2490

) ## Summary Follow-on to #25941 which updates some non-type-checked `_replace` callsites to use these new methods.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move replace_attributes, merge_attributes onto AssetSpec, publicise #25941

Move replace_attributes, merge_attributes onto AssetSpec, publicise #25941

benpankow commented Nov 14, 2024 •

edited

Loading

benpankow commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

OwenKephart left a comment

OwenKephart Nov 15, 2024 •

edited

Loading

benpankow Nov 15, 2024

OwenKephart Nov 15, 2024

schrockn commented Nov 20, 2024

benpankow commented Nov 20, 2024

dpeng817 commented Nov 20, 2024

benpankow commented Nov 20, 2024 •

edited

Loading

dpeng817 commented Nov 20, 2024 •

edited

Loading

schrockn commented Nov 22, 2024

schrockn left a comment

benpankow commented Nov 23, 2024

graphite-app bot Nov 25, 2024

Move replace_attributes, merge_attributes onto AssetSpec, publicise #25941

Move replace_attributes, merge_attributes onto AssetSpec, publicise #25941

Conversation

benpankow commented Nov 14, 2024 • edited Loading

Summary

How I Tested These Changes

Changelog

benpankow commented Nov 14, 2024 • edited Loading

github-actions bot commented Nov 15, 2024 • edited Loading

OwenKephart left a comment

Choose a reason for hiding this comment

OwenKephart Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

benpankow Nov 15, 2024

Choose a reason for hiding this comment

OwenKephart Nov 15, 2024

Choose a reason for hiding this comment

schrockn commented Nov 20, 2024

benpankow commented Nov 20, 2024

dpeng817 commented Nov 20, 2024

benpankow commented Nov 20, 2024 • edited Loading

dpeng817 commented Nov 20, 2024 • edited Loading

schrockn commented Nov 22, 2024

schrockn left a comment

Choose a reason for hiding this comment

benpankow commented Nov 23, 2024

graphite-app bot Nov 25, 2024

Choose a reason for hiding this comment

benpankow commented Nov 14, 2024 •

edited

Loading

benpankow commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024 •

edited

Loading

OwenKephart Nov 15, 2024 •

edited

Loading

benpankow commented Nov 20, 2024 •

edited

Loading

dpeng817 commented Nov 20, 2024 •

edited

Loading