Page MenuHomePhabricator

Enable setting pipeline-level default asset store
ClosedPublic

Authored by sandyryza on Sat, Oct 31, 12:49 AM.

Details

Summary

Depends on D4976.

Users can now set the default asset store across a pipeline with resource_keys={"default_asset_store": my_asset_store}.

Compatibility:

Implementation note: in an ideal world, this would include an IntermediateStorageWrappingAssetStore that allows us to narrow down to a single codepath. The main blocker for this is https://dagster.phacility.com/D4997. I.e. until that makes it in, we can't support conditional execution in an all-AssetStore world.

Test Plan

added tests

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sat, Oct 31, 1:34 AM
Harbormaster failed remote builds in B20624: Diff 24988!
sandyryza edited the summary of this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Nov 4, 6:06 PM
Harbormaster failed remote builds in B20780: Diff 25188!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Nov 5, 12:11 AM
Harbormaster failed remote builds in B20808: Diff 25224!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Nov 5, 1:45 AM
Harbormaster failed remote builds in B20813: Diff 25229!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Nov 5, 4:11 PM
Harbormaster failed remote builds in B20820: Diff 25241!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Nov 5, 4:50 PM
Harbormaster failed remote builds in B20823: Diff 25245!
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Nov 6, 1:53 AM
Harbormaster failed remote builds in B20843: Diff 25268!
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Nov 6, 6:00 AM
Harbormaster failed remote builds in B20844: Diff 25269!
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Nov 6, 4:03 PM
Harbormaster failed remote builds in B20853: Diff 25280!
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Nov 6, 9:06 PM
Harbormaster failed remote builds in B20877: Diff 25313!
Harbormaster returned this revision to the author for changes because remote builds failed.Tue, Nov 10, 5:47 PM
Harbormaster failed remote builds in B20948: Diff 25410!

default_asset_store is a bit of a strange formulation in this context IMO. Verbally I would say in this diff that the default asset store is the in-memory asset store, but then that is assigned to the default_asset_store key? I think it kind of assumes that there will be multiple asset stores per-pipeline and I don't think that will be the common case. asset_store makes more sense IMO. variable name default_is_default exposes this awkardness

python_modules/dagster/dagster/core/execution/plan/plan.py
504

default_is_default

python_modules/libraries/dagstermill/dagstermill_tests/test_manager.py
204

what is happening here

This revision now requires changes to proceed.Wed, Nov 11, 8:08 PM
python_modules/dagster/dagster/core/execution/plan/plan.py
504

default_asset_store is a bit of a strange formulation in this context IMO. Verbally I would say in this diff that the default asset store is the in-memory asset store, but then that is assigned to the default_asset_store key? I think it kind of assumes that there will be multiple asset stores per-pipeline and I don't think that will be the common case. asset_store makes more sense IMO. variable name default_is_default exposes this awkardness

I buy that. Updated to "asset_store".

python_modules/libraries/dagstermill/dagstermill_tests/test_manager.py
204

define_resource_pipeline dagstermill.examples.repository requires a resource_key named "list". I'm not sure on the context there

cool. plz consider final comments.

python_modules/dagster/dagster/core/definitions/mode.py
74โ€“76

it might be worth a comment saying that if the user passes in an asset store it will overwrite the mem_asset_store. it threw me a bit because you are always importing the mem_asset_store. I expect that the import would only occur if you needed it. the alternative code would be less elegant but far more explicit. just a thought.

python_modules/dagster/dagster/core/execution/context/system.py
295

isn't this method name misleading?

not_using_mem_asset_store much more accurate imo

This revision is now accepted and ready to land.Thu, Nov 12, 3:38 AM
python_modules/dagster/dagster/core/definitions/mode.py
74โ€“76

๐Ÿ‘

python_modules/dagster/dagster/core/execution/context/system.py
295

the way that I was looking at it is that this method answers the question "should we use an asset store for handling this step output?" the implementation happens to know that mem_asset_store is the default asset store and that we can use that to make the decision. but that calculus isn't relevant to the callers.