Page MenuHomePhabricator

asset-store public API & API Docs
ClosedPublic

Authored by yuhan on Fri, Nov 6, 11:34 PM.

Details

Summary
  • make the following public with @experimental
    • AssetStore
    • mem_asset_store
    • fs_asset_store - renamed it to be consistent with the intermediate storage naming
    • custom_path_fs_asset_store - same as above
  • improve docstring
  • update API Docs
Test Plan
make buildnext
yarn dev

screenshots:

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

yuhan requested review of this revision.Fri, Nov 6, 11:52 PM

I think we should also expose StepOutputHandle, so that users can unit-test their AssetStore methods.

docs/next/src/pages/_apidocs/index.mdx
74

I think we should call this "Asset Stores", to help avoid confusion with AssetMaterializations.

75

I think this might be a little clearer as "to define how inputs and outputs are stored and retrieved".

docs/sections/api/apidocs/assets.rst
9

I like it

python_modules/dagster/dagster/__init__.py
327

What do you think about calling this fs_asset_store for consistency with fs_intermediate_storage?

python_modules/dagster/dagster/core/storage/asset_store.py
46

Should this be Generator instead of List?

49

Nit: retrieves

51–52

This must be a dictionary, right?

75

Nit: retrieves

yuhan edited the test plan for this revision. (Show Details)
yuhan marked 7 inline comments as done.
  1. two more public apis 2) nits

lgtm! would be good to get @schrockn 's eyes because there's a lot of public API surface area here.

docs/sections/api/apidocs/execution.rst
39

It looks like this already exists under "Pipeline and solid result"

This revision is now accepted and ready to land.Tue, Nov 10, 1:26 AM
yuhan added inline comments.
docs/sections/api/apidocs/execution.rst
39

oh i see - i didn't introduce this. removing it from this section as it exists below

python_modules/dagster/dagster/core/storage/asset_store.py
51–52

right. updated the type check in AssetStoreHandle to ensure that

yuhan edited the test plan for this revision. (Show Details)

mem_asset_store not experimental

i think this looks great!

python_modules/dagster/dagster/core/storage/asset_store.py
143–154

one odd thing here is the asset_store_key in OutputDefinition in this case is not necessary.

I think we should two examples, one that uses the default "asset_store" resource name *without* requiring it on OutputDefinition, and one with a custom name

153

recommenting so not grey:

one odd thing here is the asset_store_key in OutputDefinition in this case is not necessary.

I think we should two examples, one that uses the default "asset_store" resource name *without* requiring it on OutputDefinition, and one with a custom name

238

bit a of a goofy name. let me think about it

python_modules/dagster/dagster/core/storage/asset_store.py
153

That's right! I will update it.
This was written before the reserved resource key "asset_store" was introduced. Thanks! This was a good catch.

This revision is now accepted and ready to land.Tue, Nov 17, 7:49 PM
This revision was automatically updated to reflect the committed changes.