Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add support optional input #452

Merged
merged 4 commits into from
Jun 16, 2022
Merged

Add support optional input #452

merged 4 commits into from
Jun 16, 2022

Conversation

pingsutw
Copy link
Member

TL;DR

flyteorg/flyte#2426

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

Follow-up issue

NA

pingsutw added 4 commits June 8, 2022 22:54
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #452 (9e4dd9e) into master (24e93cd) will increase coverage by 0.25%.
The diff coverage is n/a.

❗ Current head 9e4dd9e differs from pull request most recent head 55cbe7b. Consider uploading reports for the commit 55cbe7b to get more accurate results

@pingsutw
Copy link
Member Author

cc @EngHabu @kumare3

@hamersaw
Copy link
Contributor

I am having trouble understanding how FlytePropeller resolves bindings for Union types. It doesn't look like there were any changes to the binding resolver in the original Union PR. It seems to me that a change would be necessary for the case that the value does not exist to pass nil or something rather than error? Can you help me understand how FlytePropeller resolves bindings in the test case you have (ie. Union[int, none])?

@pingsutw
Copy link
Member Author

Let's say we have a task like the below.

@task
def t1(x: typing.Union[int, none]):
     ...

@workflow
def wf():
    t1()

In flytekit, We will not add any binding to the t1 node if calling t1() without input, so, Here I have to check if the params that aren't in providedBindings are optional type.

if !providedBindings.Has(paramName) && !IsOptionalType(*Variable) {

And Resolver return empty literalmap because the bindings is a empty list.

As a result, I only add "y" to VariableMap in the test, but didn't add "y" in the bindinds

@pingsutw pingsutw merged commit 68b840e into master Jun 16, 2022
@pingsutw pingsutw deleted the optional-type branch June 16, 2022 17:34
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
* Support optional input

Signed-off-by: Kevin Su <[email protected]>

* updates

Signed-off-by: Kevin Su <[email protected]>

* updates

Signed-off-by: Kevin Su <[email protected]>

* update

Signed-off-by: Kevin Su <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants