Handle op outputs in default asset IO manager #8074
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| if context.has_asset_key: | ||
| path = context.get_asset_output_identifier() | ||
| else: | ||
| path = [context.run_id, context.step_key] |
There was a problem hiding this comment.
nit: should this be context.get_output_identifier() for parity with PickledObject...
maybe even return super()._get_path(context), but I'm not picky on that
There was a problem hiding this comment.
Thanks! didn't realize that function existed
|
If I understand correctly, this makes the behavior of fs_asset_io_manager identical to the behavior of fs_io_manager, when the output is not an asset. Thoughts on just merging them together? I.e. getting rid of fs_asset_io_manager and adding logic inside fs_io_manager? A small advantage is that we'd now have a name that's not as long. Also, we should merge this after this external contribution lands so that we don't force them to rebase: #8007. |
| @@ -131,7 +131,7 @@ def asset2(asset1): | |||
|
|
|||
| return graph.to_job( | |||
| resource_defs=merge_dicts( | |||
| {"io_manager": fs_asset_io_manager}, all_resource_defs, {"root_manager": root_manager} | |||
| {"io_manager": fs_io_manager}, all_resource_defs, {"root_manager": root_manager} | |||
There was a problem hiding this comment.
I believe the to_job() function will by default supply fs_io_manager as the default io_manager, so it's possible that we can remove this bit. (not 100% sure)
clairelin135
force-pushed
the
claire/handle-op-outputs-asset-io-mgr
branch
from
1887771 to
afd6dc3
Compare
|
@sryza I've adjusted this PR to merge functionality of |
Addresses #7713
This PR adjusts the default
fs_asset_io_managerto handle op outputs by storing op outputs in a file directory/run_id/step_keyto ensure unique directories for each step.