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.

Sign up for GitHub

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

Jump to bottom

[docs] [external-assets] - Round 2 #17177

Merged
merged 5 commits into from
Oct 13, 2023
Merged

[docs] [external-assets] - Round 2 #17177

merged 5 commits into from
Oct 13, 2023

Conversation

erinkcochran87
Copy link
Contributor

@erinkcochran87 erinkcochran87 commented Oct 12, 2023 •
edited
Loading

Summary & Motivation

This PR does another round of edits on the External Assets concept page.

How I Tested These Changes

@github-actions
Copy link

github-actions bot commented Oct 12, 2023 •
edited
Loading

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-8x2ssx720-elementl.vercel.app
https://erin-dogfooding-10-12.dagster.dagster-docs.io

Direct link to changed pages:

@erinkcochran87 erinkcochran87 changed the title [docs] [external-assets | pipes] - Dogfooding [docs] [external-assets] - Round 2 Oct 12, 2023
erinkcochran87
erinkcochran87 commented Oct 12, 2023
View reviewed changes
docs/content/concepts/assets/external-assets.mdx
@@ -124,7 +123,7 @@ height={1654}
</TabItem>
</TabGroup>

### Fully-managed assets with external asset dependencies
### Dagster-native assets with external asset dependencies
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on this terminology?

Copy link
Contributor

Choose a reason for hiding this comment

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

i can't think of anything better atm. "Dagster-managed"? "Dagster-materialized"? Listing out the actual decorators "@asset, @multi_asset, and @graph_asset with external ..."?

@erinkcochran87
Copy link
Contributor Author

erinkcochran87 commented Oct 12, 2023 •
edited
Loading

These things came up during dogfooding that I could use some help answering:

  • line 149: Should external_assets_from_specs have the asterisk?
  • line 313: Can report_runless_asset_event be used for non-external assets?
  • line 320: Any reason not to include the instance call (DagsterInstance.get())?

Also, should we include anything about Pipes in here? Ex: "If you want to trigger the execution of external assets from Dagster, use Pipes. Otherwise, use this."

jamiedemaria
jamiedemaria approved these changes Oct 12, 2023
View reviewed changes
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.

these changes look good to me

@yuhan
Copy link
Contributor

yuhan commented Oct 12, 2023

  • line 149: Should external_assets_from_specs have the asterisk?
    • yes
  • line 313: Can report_runless_asset_event be used for non-external assets?
    • i believe technically yes, but not recommended which means it's doable but may pollute the non-external assets. disclaimer: i didn't have the time to actually test it - i just read the code of that method. so i can't 100% confirm it. so i think we can keep line 313 as-is.
  • line 320: Any reason not to include the instance call (DagsterInstance.get())?
    • when it's inside, say, a sensor, instance can be access through context.instance rather than DagsterInstance.get(), so i think keeping it general as instance. is fine.

@schrockn
Copy link
Member

schrockn commented Oct 12, 2023 •
edited
Loading

  • i believe technically yes, but not recommended which means it's doable but may pollute the non-external assets. disclaimer: i didn't have the time to actually test it - i just read the code of that method. so i can't 100% confirm it. so i think we can keep line 313 as-is.

For sure. This is also how we will implement the new "report event" functionality in the UI, and that works on any asset type.

@erinkcochran87 erinkcochran87 merged commit 5b6088b into master Oct 13, 2023
1 check passed
@erinkcochran87 erinkcochran87 deleted the erin/dogfooding-10-12 branch October 13, 2023 00:49
yuhan pushed a commit that referenced this pull request Oct 13, 2023
@erinkcochran87 @yuhan
[docs] [external-assets] - Round 2 (#17177)
8c0db52 
## Summary & Motivation

This PR does another round of edits on the External Assets concept page.

## How I Tested These Changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamiedemaria jamiedemaria jamiedemaria approved these changes

@yuhan yuhan Awaiting requested review from yuhan

@schrockn schrockn Awaiting requested review from schrockn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erinkcochran87 @yuhan @schrockn @jamiedemaria