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

[docs] on multi-asset concept page, use AssetSpec, not AssetOut #23111

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

sryza
Copy link
Contributor

@sryza sryza commented Jul 19, 2024

Summary & Motivation

How I Tested These Changes

@sryza sryza requested a review from jamiedemaria July 19, 2024 16:08
@graphite-app graphite-app bot requested a review from erinkcochran87 July 19, 2024 16:08
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Jul 19, 2024
Copy link

github-actions bot commented Jul 19, 2024

Copy link
Contributor

@jamiedemaria jamiedemaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good to me!

one thing i noticed is that when i read the "conditional materialization" and "subsettting" section right after each other i had a moment of confusion about the difference between the two and why can_subset isn't required in the conditional materialization case. I think part of it is that these two paragraphs are pretty dense https://github.com/dagster-io/dagster/pull/23111/files#diff-212b6a0eff6255e40aa72e669a5843113f3f2512e889ddd6e08749231ad34d6bR78-R80 which makes it hard to understand when you would use each strategy. Not required for this PR, but it would be good to revisit this section in the future. maybe an example of when you would use each one would be helpful

docs/content/concepts/assets/multi-assets.mdx Outdated Show resolved Hide resolved
@sryza sryza changed the title On multi-asset concept page, use AssetSpec, not AssetOut [docs] on multi-asset concept page, use AssetSpec, not AssetOut Jul 24, 2024
docs/content/concepts/assets/multi-assets.mdx Outdated Show resolved Hide resolved
```

### Subsetting multi-assets

By default, it is assumed that the computation inside of a multi-asset will always produce the contents all of the associated assets. This means that attempting to execute a set of assets that produces some, but not all, of the assets defined by a given multi-asset will result in an error.

Sometimes, the underlying computation is sufficiently flexible to allow for computing an arbitrary subset of the assets associated with it. In these cases, set the `is_required` attribute of the outputs to `False`, and set the `can_subset` parameter of the decorator to `True`.
Sometimes, the underlying computation is sufficiently flexible to allow for computing an arbitrary subset of the assets associated with it. In these cases, set the `skippable` attribute of the asset specs to `True`, and set the `can_subset` parameter of the decorator to `True`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Sometimes, the underlying computation is sufficiently flexible to allow for computing an arbitrary subset of the assets associated with it. In these cases, set the `skippable` attribute of the asset specs to `True`, and set the `can_subset` parameter of the decorator to `True`.
Sometimes, the underlying computation is sufficiently flexible to allow for computing an arbitrary subset of the assets associated with it. In these cases, you should set the following to `True`:
- The asset spec's `skippable` attribute
- The decorator's `can_subset` parameter

Similar suggestion here to make this less dense. I'm not sure I formatted 'spec' (or 'specs'?) here correctly, but overall, wdyt of this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind if I punt this suggestion to a separate change? Seems like a reasonable change to make, but out of scope of what I'm trying to fix in this PR.

@sryza sryza force-pushed the multi-asset-spec-doc branch from 87a3aef to d976b9f Compare August 7, 2024 21:01
@sryza sryza requested a review from erinkcochran87 August 8, 2024 17:27
@sryza
Copy link
Contributor Author

sryza commented Aug 16, 2024

Going to merge this because I got the approve from Jamie, and the changes are primarily to code snippets. @erinkcochran87 - let me know if there are any followups to the text that you think are important for me to address.

@sryza sryza merged commit ac170b3 into master Aug 16, 2024
2 checks passed
@sryza sryza deleted the multi-asset-spec-doc branch August 16, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Related to documentation in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants