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

Update context typing rules for asset checks #17137

Merged
merged 5 commits into from
Oct 16, 2023
Merged

Update context typing rules for asset checks #17137

merged 5 commits into from
Oct 16, 2023

Conversation

jamiedemaria
Copy link
Contributor

@jamiedemaria jamiedemaria commented Oct 10, 2023 •
edited
Loading

Summary & Motivation

Update the type hint rules for @asset_check to follow

step type        type hint               context type provided
asset_check   AssetExecutionContext     AssetExecutionContext
asset_check   OpExecutionContext        OpExecutionContext
asset_check   None                      AssetExecutionContext

How I Tested These Changes

alangenfeld
alangenfeld approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@alangenfeld alangenfeld left a comment

Choose a reason for hiding this comment

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

i think this makes sense

@jamiedemaria jamiedemaria force-pushed the jamie/asset-check-context branch from 4690d1d to 4197e0f Compare October 11, 2023 20:14
@jamiedemaria
Copy link
Contributor Author

jamiedemaria commented Oct 11, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions
Copy link

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-9zxhljsgo-elementl.vercel.app
https://jamie-asset-check-context.dagster.dagster-docs.io

Direct link to changed pages:

@github-actions
Copy link

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-jd10acyap-elementl.vercel.app
https://jamie-asset-check-context.core-storybook.dagster-docs.io

Built with commit 4197e0f.
This pull request is being automatically deployed with vercel-action

@jamiedemaria jamiedemaria mentioned this pull request Oct 11, 2023
Merged
@jamiedemaria jamiedemaria force-pushed the jamie/asset-check-context branch from 4197e0f to 36a87ba Compare October 12, 2023 19:34
jamiedemaria
jamiedemaria commented Oct 13, 2023
View reviewed changes
python_modules/dagster/dagster_tests/core_tests/execution_tests/test_context.py

@asset_check(asset=to_check)
def no_annotation(context):
assert isinstance(context, OpExecutionContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johannkm this is the test case that shows an asset check getting an OpExecutionContext by default when it is used in build_asset_with_blocking_check. Note that if the user instead chose to use this asset check in the "normal" use case (ie not as a blocking check) it would get an AssetExecutionContext as shown in [1]

jamiedemaria
jamiedemaria commented Oct 13, 2023
View reviewed changes
python_modules/dagster/dagster_tests/core_tests/execution_tests/test_context.py

@asset_check(asset=to_check)
def asset_annotation(context: AssetExecutionContext):
assert isinstance(context, AssetExecutionContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[1]

@jamiedemaria jamiedemaria changed the base branch from master to jamie/change-graph-asset-check October 13, 2023 16:52
@jamiedemaria jamiedemaria force-pushed the jamie/asset-check-context branch from 1b4bc77 to 16e9465 Compare October 13, 2023 16:52
@jamiedemaria jamiedemaria mentioned this pull request Oct 13, 2023
Merged
Base automatically changed from jamie/change-graph-asset-check to master October 16, 2023 14:16
@jamiedemaria jamiedemaria force-pushed the jamie/asset-check-context branch from 53f24ef to 1adc591 Compare October 16, 2023 14:18
@jamiedemaria jamiedemaria merged commit f9fbe68 into master Oct 16, 2023
@jamiedemaria jamiedemaria deleted the jamie/asset-check-context branch October 16, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johannkm johannkm johannkm approved these changes

@alangenfeld alangenfeld alangenfeld approved these changes

@yuhan yuhan Awaiting requested review from yuhan

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

Successfully merging this pull request may close these issues.
3 participants
@jamiedemaria @alangenfeld @johannkm