Page MenuHomeElementl

add dagster type to Input/OutputContext
ClosedPublic

Authored by yuhan on Dec 10 2020, 6:26 PM.

Details

Summary

This adds dagster_type to InputContext, OutputContext, and AssetStoreContext

This will be used by the intermediate storage deprecation where object_manager needs to accept serialization_strategy from context.dagster_type. in the long term, it makes sense to write object managers based on the input/output_def's type

Test Plan

bk

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

yuhan removed reviewers: sandyryza, schrockn.
yuhan requested review of this revision.Dec 10 2020, 6:43 PM
yuhan retitled this revision from intermediate-deprecation-0 add dagster type to Input/OutputContext to add dagster type to Input/OutputContext.Dec 10 2020, 7:23 PM
yuhan edited the summary of this revision. (Show Details)
yuhan added reviewers: alangenfeld, schrockn, sandyryza.

LGTM! Note my comment about argument ordering.

Worth considering whether to just put the InputDefinition/OutputDefinition on the context? Though I think passing all the properties through is probably worth saving the user from needing to type context.upstream_output.output_def.name.

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

Would be better to put this above version, because version is experimental

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

AssetStoreContext is only being kept around for compatibility, so I don't think we need to add this functionality to it.

This revision is now accepted and ready to land.Dec 10 2020, 7:31 PM
python_modules/dagster/dagster/core/storage/asset_store.py
342

My first revision didn't it to asset store context. But later in the stack the info got lost somehow as the asset store <-> object manager is in this intermediate state.
I also think we will have something like [1] for testing the ability to mock Input/OutputContext once we fully deprecate the asset store. So just to avoid introducing more confusion, I'm adding it to asset store context to keep they two behave the same while we are in the transition phase

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_asset_store.py
320–330

[1]

yuhan marked an inline comment as done.

up

This revision was automatically updated to reflect the committed changes.