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

[ext] asset check support #16466

Merged
merged 1 commit into from
Sep 23, 2023
Merged

[ext] asset check support #16466

merged 1 commit into from
Sep 23, 2023

Conversation

smackesey
Copy link
Collaborator

@smackesey smackesey commented Sep 12, 2023 •
edited
Loading

Summary & Motivation

Add support for asset checks to dagster-ext.

  • New report_asset_check_result method on ExtContext
  • Asset checks are added to the MaterializationResult objects generated by ExtOrchestrationContext

How I Tested These Changes

New unit test.

@smackesey
Copy link
Collaborator Author

smackesey commented Sep 12, 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-13t56anvn-elementl.vercel.app
https://sean-ext-asset-check.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-7cq4tqxht-elementl.vercel.app
https://sean-ext-asset-check.core-storybook.dagster-docs.io

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

@github-actions
Copy link

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-l19xvgq62-elementl.vercel.app
https://sean-ext-asset-check.components-storybook.dagster-docs.io

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

@smackesey smackesey force-pushed the sean/ext-asset-check branch 5 times, most recently from c9cb902 to 9a9fd7d Compare September 14, 2023 00:30
@smackesey smackesey changed the base branch from master to sean/urllib3 September 14, 2023 00:37
@smackesey smackesey mentioned this pull request Sep 14, 2023
Merged
Base automatically changed from sean/urllib3 to master September 14, 2023 00:38
@smackesey smackesey force-pushed the sean/ext-asset-check branch 2 times, most recently from fb4d9c7 to 736aa11 Compare September 14, 2023 00:49
@smackesey smackesey marked this pull request as ready for review September 14, 2023 00:49
schrockn
schrockn reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

I would like to unify the language between ext and our internal contexts (and in fact implement the same interface cc: @jamiedemaria ). Right now ext is report and mainline dagster is add. They have slightly different semantics (streaming versus not) which is unfortunate.

@jamiedemaria jamiedemaria mentioned this pull request Sep 14, 2023
Closed
smackesey
smackesey commented Sep 14, 2023
View reviewed changes
python_modules/dagster/dagster/_core/execution/plan/compute_generator.py Outdated
output_def
for output_def in output_defs
if not context.has_asset_check_result_for_output(output_def.name)
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

X

Copy link
Contributor

Choose a reason for hiding this comment

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

sketchy 🙂

Copy link
Contributor

@johannkm johannkm Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

how do we solve for materialization?

johannkm
johannkm reviewed Sep 15, 2023
View reviewed changes
python_modules/dagster/dagster/_core/definitions/asset_layer.py Outdated
@@ -644,6 +644,15 @@ def asset_checks_def_for_node(
def asset_checks_defs(self) -> Iterable[AssetChecksDefinition]:
return self.asset_checks_defs_by_node_handle.values()

def get_asset_check_for_output_name(self, output_name: str) -> Optional[AssetCheckHandle]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think output names are unique across the asset layer

Copy link
Contributor

Choose a reason for hiding this comment

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

instead I would use check_handle_by_node_output_handle, looks like you could get a node output handle in the callsite

schrockn
schrockn requested changes Sep 15, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

requesting changes based on johann's feedback. need to add test case with a more expansive asset graph

@smackesey smackesey mentioned this pull request Sep 15, 2023
Merged
smackesey added a commit that referenced this pull request Sep 15, 2023
@smackesey
Add asset checks, data version to MaterializeResult (#16514)
1eeafa8 
## Summary & Motivation

Add `check_results` and `data_version` to `MaterializeResult`. This
supports streaming asset check results back from an ext process added
upstack in #16466.

## How I Tested These Changes

New unit test.
johannkm pushed a commit that referenced this pull request Sep 15, 2023
@smackesey @johannkm
Add asset checks, data version to MaterializeResult (#16514)
74c61a6 
## Summary & Motivation

Add `check_results` and `data_version` to `MaterializeResult`. This
supports streaming asset check results back from an ext process added
upstack in #16466.

## How I Tested These Changes

New unit test.
zyd14 pushed a commit to zyd14/dagster that referenced this pull request Sep 15, 2023
@smackesey @zyd14
Add asset checks, data version to MaterializeResult (dagster-io#16514)
0b0e1fc 
## Summary & Motivation

Add `check_results` and `data_version` to `MaterializeResult`. This
supports streaming asset check results back from an ext process added
upstack in dagster-io#16466.

## How I Tested These Changes

New unit test.
@smackesey smackesey changed the base branch from master to sean/ext-use-materialize-result September 19, 2023 15:58
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from 90fbb88 to 6d635e3 Compare September 22, 2023 11:39
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch 3 times, most recently from 61d2116 to 82e09c3 Compare September 22, 2023 13:05
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from 82e09c3 to 47acf2c Compare September 22, 2023 14:27
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from 47acf2c to 762811c Compare September 22, 2023 19:04
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from 762811c to 9a7b448 Compare September 22, 2023 19:19
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from 9a7b448 to 8efbe63 Compare September 22, 2023 20:10
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from 8efbe63 to 979cbc7 Compare September 22, 2023 20:25
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from 979cbc7 to d884d4b Compare September 22, 2023 20:33
@smackesey smackesey force-pushed the sean/ext-use-materialize-result branch from d884d4b to f7f8147 Compare September 22, 2023 20:39
@smackesey smackesey force-pushed the sean/ext-asset-check branch 2 times, most recently from 04fa87e to c024041 Compare September 22, 2023 20:42
@smackesey
Copy link
Collaborator Author

Very specifically, I think we should have a super clean PR that we can point people to as a "how-to" for adding new event types. Will be the easiest way to document that.

I think we're there now

@smackesey smackesey requested a review from schrockn September 22, 2023 20:43
Base automatically changed from sean/ext-use-materialize-result to master September 22, 2023 21:06
schrockn
schrockn approved these changes Sep 23, 2023
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

This is great

@smackesey smackesey merged commit 095f959 into master Sep 23, 2023
@smackesey smackesey deleted the sean/ext-asset-check branch September 23, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johannkm johannkm johannkm left review comments

@schrockn schrockn schrockn approved these changes

@alangenfeld alangenfeld Awaiting requested review from alangenfeld

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
@smackesey @johannkm @schrockn