[ext] remove report_asset_metadata, report_asset_data_version #16683
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
smackesey
changed the base branch from
sean/ext-use-materialize-result
to
master
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
from
87806df to
39a0273
Compare
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
from
39a0273 to
e5e0778
Compare
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
from
e5e0778 to
d6bb8aa
Compare
| ) | ||
| new_metadata[key] = cast(ExtMetadataValue, value) | ||
| else: | ||
| new_metadata[key] = {"raw_value": value, "type": None} |
There was a problem hiding this comment.
With type None, we feed the raw value to our metadata type inference routine on the orchestration side, instead of replicating that logic here. I added an explanatory comment.
There was a problem hiding this comment.
where is the related code on the orch side?
There was a problem hiding this comment.
Untyped metadata values always get run through this: https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/definitions/metadata/__init__.py#L106-L135
Which is called here: https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster/_core/ext/context.py#L54-L55
There was a problem hiding this comment.
Ok. I would much rather have this be an explicit value rather than None. e.g. "infer", "untyped" etc I think that would be more clear and obvious
There was a problem hiding this comment.
Good idea, I made it "__infer__"
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
from
d6bb8aa to
6f21afe
Compare
smackesey
force-pushed
the
sean/ext-minor-fixes
branch
from
1b2d25a to
c50e6b4
Compare
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
from
6f21afe to
c2aab86
Compare
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
from
c2aab86 to
b6d86eb
Compare
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
2 times, most recently
from
186a160 to
c8300d8
Compare
| EXT_METADATA_TYPE_INFER = "__infer__" | ||
|
|
||
| ExtMetadataType = Literal[ | ||
| "__infer__", |
smackesey
force-pushed
the
sean/ext-remove-reporters
branch
from
c8300d8 to
8788e2f
Compare
|
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-8fk07ppsv-elementl.vercel.app Direct link to changed pages: |
## Summary & Motivation Remove `report_asset_metadata` and `report_asset_data_version` in favor of a single `report_asset_materialization` method, which requires reporting all of the data associated with a materialization at once. Calling `report_asset_materialization` twice for the same asset is an error. ## How I Tested These Changes Updated unit tests.
Summary & Motivation
Remove
report_asset_metadataandreport_asset_data_versionin favor of a singlereport_asset_materializationmethod, which requires reporting all of the data associated with a materialization at once. Callingreport_asset_materializationtwice for the same asset is an error.How I Tested These Changes
Updated unit tests.