Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[prototype] return AssetMaterialization #14931

Closed
wants to merge 1 commit into from
Closed

[prototype] return AssetMaterialization #14931

wants to merge 1 commit into from

Conversation

alangenfeld
Copy link
Member

explorations for returning AssetMaterializtion directly

@alangenfeld
Copy link
Member Author

alangenfeld commented Jun 23, 2023 •
edited
Loading

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@alangenfeld alangenfeld force-pushed the al/06-23-_prototype_return_AssetMaterialization branch from 9f29736 to 556c934 Compare June 23, 2023 21:58
alangenfeld
alangenfeld commented Jun 26, 2023
View reviewed changes
python_modules/dagster/dagster_tests/asset_defs_tests/test_assets.py Outdated
Comment on lines 1535 to 1537
# supporting AssetMaterialization without asset_key requires handling it being in a partial state.
# could do AssetMaterialization.partial(...) with special partial state obj
asset_key=context.asset_key_for_output(),
Copy link
Member Author

@alangenfeld alangenfeld Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idea: allow omitting in constructor args but avoid nullability on the object itself by grabbing the current asset_key indirectly (and erroring when can not)

edit :this has since been implemented and in the PR

@alangenfeld alangenfeld changed the base branch from master to al/06-26-add_indirect_execution_context_access June 26, 2023 17:36
@alangenfeld alangenfeld force-pushed the al/06-23-_prototype_return_AssetMaterialization branch from 556c934 to 51870da Compare June 26, 2023 17:36
@alangenfeld alangenfeld mentioned this pull request Jun 26, 2023
Merged
@sryza
Copy link
Contributor

sryza commented Jun 26, 2023

Have you considered a new MaterializationMetadata type?

@alangenfeld
Copy link
Member Author

Have you considered a new MaterializationMetadata type?

Ya thats the other option, in previous discussions no one was hot on the idea of adding a new noun. Whats your take?

@sryza
Copy link
Contributor

sryza commented Jun 27, 2023

My initial lean would be towards MaterializationMetadata I think. allowing AssetMaterialization to be in two different states (with asset key or without) feels weird.

It's also a breaking change, right? For anyone depending on the asset key attribute to be non-None.

And it means that anyone using type-checking needs to guard against None whenever they access AssetMaterialization.asset_key.

@alangenfeld
Copy link
Member Author

allowing AssetMaterialization to be in two different states (with asset key or without) feels weird.

Agree, and that is not the case in the current updated state of this prototype PR. We allow omitting from the constructor in cases we can infer it and error otherwise [2].

alangenfeld
alangenfeld commented Jun 27, 2023
View reviewed changes
python_modules/dagster/dagster/_core/definitions/events.py
Comment on lines +507 to +527
elif asset_key is None:
current_ctx = get_execution_context()
if current_ctx is None:
raise DagsterInvariantViolationError(
"Could not infer asset_key, not currently in the context of an execution."
)
keys = current_ctx.selected_asset_keys
if len(keys) != 1:
raise DagsterInvariantViolationError(
f"Could not infer asset_key, there are {len(keys)} in the current execution"
" context. Specify the appropriate asset_key."
)
asset_key = next(iter(keys))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[2]

@alangenfeld
Copy link
Member Author

How do you feel about [2] and with that does that change your stance on MaterializationMetadata?

@sryza
Copy link
Contributor

sryza commented Jun 28, 2023

How do you feel about [2] and with that does that change your stance on MaterializationMetadata?

Ah, I missed this. I'm having trouble putting my finger on any concrete negatives, but this kind of pattern gives me some generalized discomfort – I think it's difficult for users to look at the code and guess how it's working.

@alangenfeld
Copy link
Member Author

this kind of pattern gives me some generalized discomfort – I think it's difficult for users to look at the code and guess how it's working.

Makes sense, this is definitely a "may be lesser of two evils" type of proposal.

Probably need to flesh this prototype out to include multi_asset as I think the full picture including how things work there will influence the final call.

@alangenfeld alangenfeld force-pushed the al/06-26-add_indirect_execution_context_access branch from 37be28c to 8b69394 Compare July 10, 2023 20:31
@alangenfeld alangenfeld force-pushed the al/06-23-_prototype_return_AssetMaterialization branch from 51870da to da70d2a Compare July 10, 2023 20:31
sryza
sryza reviewed Aug 16, 2023
View reviewed changes
python_modules/dagster/dagster/_core/execution/plan/execute_step.py
@@ -165,6 +171,15 @@ def _step_output_error_checked_user_event_sequence(
f'Emitting implicit Nothing for output "{step_output_def.name}" on {op_label}'
)
yield Output(output_name=step_output_def.name, value=None)

if step_context.is_sda_step and step_context.get_observed_user_asset_mat(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this has any consequence with the way that execution currently works, but imagine a world where we could kick off downstream steps as soon as an upstream output is produced, even if the upstream step hasn't finished yet.

In that world, we would ideally yield the Output as soon the materialization is reported/yielded/returned, right?

@alangenfeld alangenfeld force-pushed the al/06-26-add_indirect_execution_context_access branch from 8b69394 to d0752c8 Compare August 18, 2023 14:46
@alangenfeld alangenfeld force-pushed the al/06-23-_prototype_return_AssetMaterialization branch from da70d2a to 928618e Compare August 18, 2023 14:46
@alangenfeld alangenfeld mentioned this pull request Aug 18, 2023
Merged
alangenfeld added a commit that referenced this pull request Aug 25, 2023
@alangenfeld
add MaterializeResult (#15932)
711d2e5 
The latest evolution of #14931
& #15392 intentionally
aligned with #15928 this PR
adds support for a new "Result" return type from assets that do not deal
with "Outputs" to be able to communicate materialization metadata.

## How I Tested These Changes

added tests.
@alangenfeld alangenfeld closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sryza sryza sryza left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alangenfeld @sryza