- 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
Details
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 | ||
---|---|---|
401 | 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. |
python_modules/dagster/dagster/core/execution/context/system.py | ||
---|---|---|
401 | updated. the context should be straightforward to mock now. | |
441 | @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.
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? | |
400 | 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. | |
441 | 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 | ||
---|---|---|
441 | 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 | ||
---|---|---|
441 | @ |
python_modules/dagster/dagster/core/execution/context/system.py | ||
---|---|---|
441 | ^ignore |
python_modules/dagster/dagster/core/execution/context/system.py | ||
---|---|---|
441 | 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. | |
441 | 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.