https://flyte.org logo
Join Slack
Powered by
Is it recommended to use `conditional` inside a `@...
# flytekit
r
Is it recommended to use 
conditional
inside a 
@dynamic
step or should normal python control flow work there? We're seeing a number of issues w/ failing to bind variables in a dynamic workflow using normal `if`/`else` blocks
s
Tasks within a dynamic workflow return Promise objects. I think conditional is a better choice. @Kevin Su, is 
conditional
supported within dynamic workflows?
k
Yes, it should work
r
Thanks, I'll give that a try!
Something feels strange here...I'm getting the following error w/ the below construct in a 
@dynamic
step (
regress
is a boolean)
Copy code
Logical (and/or/is/not) operations are not supported. Expressions Comparison (<,<=,>,>=,==,!=) or Conjunction (&/|) are supported.Received an evaluated expression with val False in train_estimator.if_
Copy code
flytekit.conditional("train_estimator")
        .if_(regress)
        .then(
            estimator.train_regressor()
         )
        .else_()
        .then(
          estimator.train_classifier()
        )
Seems odd that I can't directly pass a boolean value into 
flytekit.conditional
? (I also tried 
regress == True
as well and that resulted in the same error)
I guess I could push all of this into the task, but then that unfortunately forces us to create a second task for training the estimator vs re-using an existing task
k
Below is the workflow I ran in flyte cluster, it works for me. However, it doesn’t work in local execution now.
Copy code
from flytekit import workflow, task, dynamic, conditional


@task
def t1() -> bool:
    return True


@task
def t2() -> bool:
    return False


@dynamic
def d1() -> bool:
    a = t1()
    return (
        conditional("train_estimator")
        .if_(a==True)
        .then(t2())
        .else_()
        .then(t2()))


@workflow
def wf() -> bool:
    return d1()


if __name__ == "__main__":
    print(wf())
r
@Kevin Su does this work if 
a
is an arg to 
d1
vs the result of a task?
k
btw, should use 
regress == True
instead of 
regress
. Otherwise, it will fail
does this work if 
a
is an arg to 
d1
vs the result of a task?
let me try it.
it doesn’t work because the type of 
a
is bool instead of promise.
tl;dr 
a
in the if statement must be a promise
r
Hm...ok. Is there a way to do something like 
Promise.of(my_literal)
(kind of abusing JavaScript notation here but you get the point)?
Or maybe another way, is there a reason preventing this from working w/ both promises and literals?
k
Hm...ok. Is there a way to do something like 
Promise.of(my_literal)
(kind of abusing JavaScript notation here but you get the point)?
we don’t have that now, but you could add a task to convert it to a promise in @dynamic
Copy code
@task
def convert_to_promise(a: bool) -> bool:
    return a
Or maybe another way, is there a reason preventing this from working w/ both promises and literals
I think we can support pure python type in 
_if
as well. we can try to update it in the next release. For now, to work around this issue, just create a @task to convert it to a promise, sorry.
cc @Yee @Eduardo Apolinario (eapolinario) do we have another way to handle 
bool
or 
int
in 
_if
in 
condition
? currently, we can only use promise in condition.
r
Thanks @Kevin Su. I'd be happy to contribute this too if there are other milestones you guys are working towards for the next release
k
yeah, I just created an issue for it, feel free to create a pr, thanks.
159 Views
Open in Slack
PreviousNext
Powered by