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
feat(bindings): Task arguments default value binding #2401
base: master
Are you sure you want to change the base?
feat(bindings): Task arguments default value binding #2401
Conversation
b434ddf
to
cc13415
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.
Also, should we remove these lines? since if statement will always be false.
@@ -46,6 +46,14 @@ def t2(a: typing.Optional[int] = None) -> typing.Optional[int]: | |||
assert p.ref.var == "o0" | |||
assert len(p.ref.node.bindings) == 0 | |||
|
|||
@task | |||
def t_default_value(a: int = 1) -> typing.Optional[int]: |
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.
Could you also add a test like below?
@task
def。t_default_value(a: int = 1) -> typing.Optional[int]:
return a
@workflow
def wf() -> typing.Optional[int]:
return t_default_value(a=2)
I want to make sure the output of the workflow is 2
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.
Done
cc13415
to
277d88b
Compare
Done |
51e15e5
to
6446ad1
Compare
could you add more tests please? take a look at serialization tests, it should be pretty simple to test the compiled workflow. do we do anything special for [] and {}? You're not supposed to use those as function defaults anyways but some people might. |
Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
6446ad1
to
6c808ae
Compare
Done |
Tracking issue
Resolves: flyteorg/flyte#5321
Why are the changes needed?
See the issue description for details.
What changes were proposed in this pull request?
if the key is not in
kwargs
but ininterface.inputs_with_defaults
, add the value ininterface.inputs_with_defaults
tokwargs
.How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link