-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
fix multi asset check selection #16877
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
ac6653d
to
eda1731
Compare
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-849szcrq9-elementl.vercel.app Direct link to changed pages: |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit b5d2493. |
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit b5d2493. |
eda1731
to
a1884d2
Compare
@@ -669,6 +680,7 @@ def _get_execution_plan_from_run( | |||
and execution_plan_snapshot.can_reconstruct_plan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note, not sure when this would be false. If the job was updated between us launching and executing the run?
@@ -258,23 +283,6 @@ def __init__( | |||
backfill_policy, "backfill_policy", BackfillPolicy | |||
) | |||
|
|||
if selected_asset_check_keys is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed the structure here to match what we do in asset layer
@@ -794,28 +794,6 @@ def _get_job_def_for_asset_selection( | |||
" or job." | |||
) | |||
|
|||
# Test that selected asset checks can be run individually. Currently this is only supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this, we fall back a slightly less interpretable error. But we'll be replacing that with a better error once I land atomic execution unit ids on checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I stacked #16801 on top of this, and subsetting is working smoothly when executing checks in the UI 🙏
19f5311
to
b5d2493
Compare
I didn't test selecting inside a multi asset over the graphql layer, and lo and behold it wasn't being passed properly. Yet more fodder for a combo selection object. --------- Co-authored-by: Rex Ledesma <[email protected]>
I didn't test selecting inside a multi asset over the graphql layer, and lo and behold it wasn't being passed properly. Yet more fodder for a combo selection object.