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

Check for None values in branch nodes #592

Closed
wants to merge 10 commits into from
Closed

Check for None values in branch nodes #592

wants to merge 10 commits into from

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Jul 19, 2023

TL;DR

blocked by flyteorg/flytekit#1747
Add support comparison (EQ and NEQ) between nil and non-nil values in the branch nodes.

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

import random
import typing
from flytekit import conditional, task, workflow


@task
def coin_toss(seed: int) -> typing.Optional[bool]:
    r = random.Random(seed)
    if r.random() < 0.5:
        return True
    return None


@task
def failed() -> int:
    return -1


@task
def success() -> int:
    return 0


@workflow
def wf(seed: int = 5) -> int:
    result = coin_toss(seed=seed)
    return conditional("test").if_(result.is_none()).then(success()).else_().then(failed())


if __name__ == '__main__':
    wf()
image

Tracking Issue

flyteorg/flyte#3514

Follow-up issue

NA

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #592 (1b7ee7a) into master (2aca906) will increase coverage by 0.43%.
The diff coverage is 66.15%.

❗ Current head 1b7ee7a differs from pull request most recent head 4473bd9. Consider uploading reports for the commit 4473bd9 to get more accurate results

Additional details and impacted files

pkg/controller/nodes/branch/evaluator.go Outdated Show resolved Hide resolved
pkg/controller/nodes/branch/comparator.go Outdated Show resolved Hide resolved
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@hamersaw
Copy link
Contributor

@pingsutw can we update here? looks like flyteorg/flyteidl#427 was merged.

@pingsutw
Copy link
Member Author

Sorry @hamersaw, forget this PR, could you +1? We've merged flytekit and flyteidl PRs

Signed-off-by: Kevin Su <[email protected]>
@eapolinario
Copy link
Contributor

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4154. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

@eapolinario eapolinario closed this Oct 3, 2023
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.

3 participants