Fix error on op selection #17233
Fix error on op selection #17233
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
johannkm
force-pushed
the
johann/10-16-Fix_error_on_op_selection
branch
from
42fd4c9 to
c9a2df7
Compare
johannkm
force-pushed
the
johann/10-16-Fix_error_on_op_selection
branch
from
c9a2df7 to
b622a21
Compare
| @@ -201,6 +201,8 @@ def infer_pipeline_selector( | |||
| { | |||
| "pipelineName": pipeline_name, | |||
| "solidSelection": op_selection, | |||
| "assetSelection": [], | |||
There was a problem hiding this comment.
So this now means we have coverage for the case where these are set. Is there a case where these aren't set that we're losing coverage for?
There was a problem hiding this comment.
Good question. The front end always provides them now, but that's not guaranteed forever and gql api users might not. I'll add coverage
There was a problem hiding this comment.
updated with a specific case for []. Going to start a discussion about how to handle this going forwards
johannkm
force-pushed
the
johann/10-16-Fix_error_on_op_selection
branch
from
b622a21 to
75c1ac9
Compare
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 75c1ac9. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 75c1ac9. |
| # asset_selection gets coerced from [] to None, but asset_check_selection doesn't. We | ||
| # allow asset_check_selection to be [] when op_selection is set. |
There was a problem hiding this comment.
I'm still struggling to follow this. Where does the coercing being referenced here happen? Can we add some whys in addition to some whats here / be more verbose?
johannkm
force-pushed
the
johann/10-16-Fix_error_on_op_selection
branch
from
75c1ac9 to
f98f0f7
Compare
With https://github.com/dagster-io/dagster/pull/16640/files#diff-3a0d9c2e7d488c33f5be6fdf25298f9b03277b1bc73417d53e0a51688d32e8b6, we started passing assetCheckSelection: [] for op jobs. This was untested, and hit this invariant because unlike for assets where [] gets turned in to None, we maintain a difference between None and [] with asset checks This is ripe for rethinking
With https://github.com/dagster-io/dagster/pull/16640/files#diff-3a0d9c2e7d488c33f5be6fdf25298f9b03277b1bc73417d53e0a51688d32e8b6, we started passing assetCheckSelection: [] for op jobs. This was untested, and hit this invariant because unlike for assets where [] gets turned in to None, we maintain a difference between None and [] with asset checks
This is ripe for rethinking