-
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
Materialize assets without running checks #16460
Materialize assets without running checks #16460
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
cef7310
to
bc7f38d
Compare
bc7f38d
to
624bddd
Compare
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.
the None
vs []
shit is always nasty but alternative solutions ive ever come up with are worse. Agree test coverage is critical here
graphql_context, | ||
"asset_check_job", | ||
asset_selection=[{"path": ["asset_1"]}], | ||
asset_check_selection=[], |
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.
existing None
coverage is sufficient?
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.
yeah we test it in test_launch_subset_asset_and_included_check
I'll let @alangenfeld handle this one |
624bddd
to
4ccbc21
Compare
Deploy preview for dagster-university ready! ✅ Preview Built with commit 4ccbc21. |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 4ccbc21. |
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 4ccbc21. |
Addendum to e6ed078.
Remove two places where we turned
asset_check_selection
from[]
toNone
. This lets us passasset_selection=[whatever]
andasset_check_selection=[]
to only materialize the assets and not run checks.asset_selection=[whatever]
andasset_check_selection=None
will do both.This is somewhere that having a combined object might let us be more explicit instead of using these two values. But I think as long as we have a test cases for entrypoints we add, we'll catch if they get messed up