Page MenuHomeElementl

asset-store AssetStoreContext 3/ nullable pipeline run and include execution plan
ClosedPublic

Authored by yuhan on Nov 18 2020, 8:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 27, 10:43 AM
Unknown Object (File)
Sat, Nov 26, 12:34 AM
Unknown Object (File)
Sun, Nov 20, 4:51 PM
Unknown Object (File)
Mon, Nov 14, 6:19 AM
Unknown Object (File)
Sun, Nov 13, 3:35 AM
Unknown Object (File)
Sat, Nov 12, 3:30 PM
Unknown Object (File)
Sat, Nov 12, 3:28 PM
Unknown Object (File)
Wed, Nov 9, 12:42 PM
Subscribers
None

Details

Summary
  • make AssetStoreContext take all the flat info we offer as properties, so mocking would be easy
  • introduce a static method AssetStoreContext.construct to parse the info from internal representations and build the context
Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
yuhan/asset-store-context-2
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

python_modules/dagster/dagster/core/execution/context/system.py
406–407

I don't think we need to include execution plan in the initialization, but rather pre-compute what we need from it. I think this will make testing easier as well.

This revision now requires changes to proceed.Nov 18 2020, 10:46 PM
yuhan added inline comments.
python_modules/dagster/dagster/core/execution/context/system.py
406–407

updated. the context should be straightforward to mock now.

422

@cdecarolis i was thinking maybe we could do the same in a new helper execution_plan.construct_asset_store_context and feed the step there.

  • for testing, the user doesn't need to provide step but internally, it gives us the pipeline_name and solid_name.
  • from solid_name, D5083 can find the solid_def and solid_def.tags via execution_plan.pipeline_def

i didn't include these^ in this diff because it'd be easier to test that method in your diff.

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

Nitpick: are these parentheses needed?

403

For the slack digest pipeline, we'll need the solid's tags. If we're providing both those and the name, IMO best to just provide the whole solid definition.

422

Do we want to expose this method to users? If not, maybe preferable to put it elsewhere. (Maybe that's what you're already suggesting in your comment?)

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

cool, that makes sense. Do you think it makes more sense to include execution_plan.construct_asset_store_context here, or in my subsequent diffs, due to it being easier to test there as well?

LGTM! Fwiw @sandyryza I think exposing the construct method makes sense, if at least for the testing case that we talked about before.

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

@

This revision is now accepted and ready to land.Nov 19 2020, 12:59 AM
python_modules/dagster/dagster/core/execution/context/system.py
422

^ignore

yuhan marked 2 inline comments as done.

comments + execution_plan.construct_asset_store_context

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

ok i think i was just being lazy.. but then i realized i needed to test it anyways, so i wrote that in this diff.

422

i moved it to SystemStepExecutionContext.for_asset_store and ExecutionPlan.construct_asset_store_context. it also made my life so much easier.

@cdecarolis @sandyryza i made some big changes including execution_plan.construct_asset_store_context so would like to have you take another look before landing

LGTM! I think these changes are gonna enable the versioning workstreams in a big way.

This revision is now accepted and ready to land.Nov 19 2020, 4:45 PM