[docs] remove I/O managers from SDA page (DOC-108) #19269
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| @@ -168,15 +111,6 @@ def my_derived_asset(): | |||
| return execute_query("SELECT * from a_source_asset").as_list() + [4] | |||
| ``` | |||
|
|
|||
| You can also define a dependency on a `SourceAsset` that will load the data of the asset: | |||
There was a problem hiding this comment.
this should probably get moved to the I/O manager concept page
| @@ -271,31 +205,6 @@ def context_asset(context: AssetExecutionContext): | |||
| ... | |||
| ``` | |||
|
|
|||
| ### Conditional materialization | |||
There was a problem hiding this comment.
This should be replaced with a section discussion conditional yielding of MaterializeResult (assuming that that is the "conditional materialization" equivalent in no-io world)
There was a problem hiding this comment.
Will this still be valid if someone does elect to use an I/O manager? Should this be deleted entirely, or just moved elsewhere and re-written here?
There was a problem hiding this comment.
Ah, yes. This section should get moved to the I/O manager concept page.
I need to do a little testing to ensure i fully understand the syntax for indicating a conditional materialization when not using I/O managers, and then I can write the replacement section
There was a problem hiding this comment.
hmmm the API to do this without I/O managers isn't great....
There was a problem hiding this comment.
Either way, I think the section as written right now should be moved to the I/O manager concept page. And then we can either:
- be done. Decide that the API ergonomics need to be fixed before publicly documenting this use case for non I/O managed assets
- write a replacement section for non I/O managed assets that uses the less-than-great APIs
I'll think a bit on what my opinion is for which option we take, but if you have a rule of thumb for this type of situation in the docs, I will follow that!
There was a problem hiding this comment.
I think we can punt on that so it doesn't hold up this PR.
I don't have a hard and fast rule for handling this kind of thing in the docs. I tend to take into account how often users would ask about something when deciding whether to include a less-than-ideal approach to something in the docs, especially if we're going to change it soon.
Perhaps we could throw the non-ideal workaround in a discussion (which IMO a 'less official' set of content) until the ergonomics are addressed? It could be something like "We plan on improving this thing, but for now, here's how you can do it"
There was a problem hiding this comment.
I like the discussion approach! i'll merge this and then write that up
| @@ -593,26 +490,6 @@ def test_uses_context(): | |||
|
|
|||
| --- | |||
|
|
|||
| ## Loading asset values outside of Dagster runs | |||
There was a problem hiding this comment.
should be moved to the I/O manager page
| @@ -665,9 +542,6 @@ def table1() -> Output[None]: | |||
| return Output(None, metadata={"num_rows": 25}) | |||
| ``` | |||
|
|
|||
| #### Recording materialization metadata using I/O managers | |||
There was a problem hiding this comment.
should be moved to I/O manager concept page
|
@jamiedemaria it looks like the SDA page was reverted (or at least it's not showing up in the changed files atm) - is this still the PR I should be looking at? |
i have no idea how that happened ughhhhh. i'll see if i can find it in the change history |
ea9f538
to
ec842d7
Compare
38d8428
to
18e9e03
Compare
|
@erinkcochran87 I got the page back, so this is reviewable now! |
| @@ -271,31 +205,6 @@ def context_asset(context: AssetExecutionContext): | |||
| ... | |||
| ``` | |||
|
|
|||
| ### Conditional materialization | |||
There was a problem hiding this comment.
Will this still be valid if someone does elect to use an I/O manager? Should this be deleted entirely, or just moved elsewhere and re-written here?
I think it depends a bit on how much we want to make I/O managers not exist on this page. I was envisioning that a new user reading this page would finish and have no idea what I/O managers are or how to do any of the things that opt you in to using I/O managers (most relevant for this page, that's having defining dependencies using parameters). In that case, I think the right thing to do would be to move them to the I/O manager concept page, and kind of frame it like "If you use I/O managers, now you can define dependencies like this to have the I/O manager load the value". |
|
I think that's spot-on. @jamiedemaria |
ec842d7
to
585b1a4
Compare
ab8d1fd
to
d5089a2
Compare
d5089a2
to
7cf4809
Compare
18c1276
to
7681e2a
Compare
7cf4809
to
d982f65
Compare
7681e2a
to
afafc1f
Compare
6101a88
to
4d28536
Compare
4d28536
to
b202c68
Compare
b202c68
to
62ab1ee
Compare
f9233ee
to
762325a
Compare
762325a
to
c0cfac4
Compare
Summary & Motivation
Updates the SDA page to remove references to I/O managers
Outstanding questions/todos:
How I Tested These Changes