fix(sdk): fix bug where dsl.OneOf with multiple consumers cannot be compiled
#10452
Conversation
|
Skipping CI for Draft Pull Request. |
f3ed311 to
fcfbe29
Compare
fcfbe29 to
d9b00a3
Compare
|
@connor-mccarthy: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/assign @chensun |
| @@ -4786,6 +4786,12 @@ def flip_coin_pipeline(execute_pipeline: bool): | |||
| x = dsl.OneOf(print_task_1.outputs['a'], | |||
| print_task_2.outputs['a']) | |||
| print_artifact(a=x) | |||
| # test can be consumed multiple times from same oneof object | |||
| print_artifact(a=x) | |||
| y = dsl.OneOf(print_task_1.outputs['a'], | |||
There was a problem hiding this comment.
This seems to me redundant oneof, x and y would never differ from each other. In which scenario would we need y instead of reusing x?
There was a problem hiding this comment.
It is redundant, but it should be permitted, similar to how this would be permitted in normal Python:
x = 1
y = 1
add(x, y)even though add(x, x) is equivalent due to Python's use of call by value for immutable variables.
Prior to this PR, this would have failed. Since the PR adds a fix, it also adds test coverage.
There was a problem hiding this comment.
It's fine to allow this redundancy. Albeit I think it's also reasonable to block it. Reasons include:
dsl.OneOfis a special syntax that is paired with a conditional branch set. There're restrictions on which inputs it can accept--e.g. it needs to include all exclusive outputs, not something else nor a subset of such outputs. Arguably, its argument list is even redundant and skippable. The special use case warrants special treatment/requirement.- I had this concern earlier that the pair seems loosely connected given it allows arbitrary code in between of the conditional branch and the oneof statement. Allowing multiple collection seems exposing this loose-pair further.
- Conceptually, the oneof resolution happens only once (at runtime). Enforcing a single oneof matches and highlights the actual behavior, and avoids possible misconception.
There was a problem hiding this comment.
Discussed offline and will merge as-is. As far as we can tell, there's no obvious cost to supporting this user code other than increased flexibility, which is both a cost and a benefit.
| @@ -4786,6 +4786,12 @@ def flip_coin_pipeline(execute_pipeline: bool): | |||
| x = dsl.OneOf(print_task_1.outputs['a'], | |||
| print_task_2.outputs['a']) | |||
| print_artifact(a=x) | |||
| # test can be consumed multiple times from same oneof object | |||
| print_artifact(a=x) | |||
| y = dsl.OneOf(print_task_1.outputs['a'], | |||
There was a problem hiding this comment.
It's fine to allow this redundancy. Albeit I think it's also reasonable to block it. Reasons include:
dsl.OneOfis a special syntax that is paired with a conditional branch set. There're restrictions on which inputs it can accept--e.g. it needs to include all exclusive outputs, not something else nor a subset of such outputs. Arguably, its argument list is even redundant and skippable. The special use case warrants special treatment/requirement.- I had this concern earlier that the pair seems loosely connected given it allows arbitrary code in between of the conditional branch and the oneof statement. Allowing multiple collection seems exposing this loose-pair further.
- Conceptually, the oneof resolution happens only once (at runtime). Enforcing a single oneof matches and highlights the actual behavior, and avoids possible misconception.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: connor-mccarthy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
… compiled (kubeflow#10452) Signed-off-by: Alexey Roytman <[email protected]>
Description of your changes:
Checklist: