flyteorg / flyte

Scalable and flexible workflow orchestration platform that seamlessly unifies data, ML and analytics stacks.
https://flyte.org
Apache License 2.0
5.17k stars 551 forks source link

Fix: Make 'flytectl compile' consider launchplans used within workflows #5437

Closed fg91 closed 1 week ago

fg91 commented 1 month ago

Why are the changes needed?

from flytekit import LaunchPlan, task, workflow

@task
def my_task():
    pass

@workflow
def inner_workflow():
    my_task()

@workflow
def outer_workflow():
    LaunchPlan.get_or_create(inner_workflow, "name_override")()

This workflow can be registered with pyflyte register and executed in a cluster. However, pyflyte --pkgs <module> package + flytectl compile gives the following error:

...
Compiling workflow: wfs.wf.outer_workflow
:( Error Compiling workflow: wfs.wf.outer_workflow
Error: Collected Errors: 1
        Error 0: Code: WorkflowReferenceNotFound, Node Id: start-node, Description: Referenced Workflow [resource_type:LAUNCH_PLAN  name:"name_override"] not found.

What changes were proposed in this pull request?

The error occurs since flytectl compile does not consider all launchplans but only the default launchplan for the currently compiled workflow.

How was this patch tested?

Added a unit test based on the example above.

Note to reviewers: We couldn't find whether the source files for the testdata/*.tgz should be checked in as well - should they?

Check all the applicable boxes

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 61.11%. Comparing base (d04cf66) to head (0ffe186). Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5437 +/- ## ======================================= Coverage 61.10% 61.11% ======================================= Files 793 793 Lines 51164 51171 +7 ======================================= + Hits 31265 31272 +7 Misses 17027 17027 Partials 2872 2872 ``` | [Flag](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | Coverage Δ | | |---|---|---| | [unittests-datacatalog](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `69.31% <ø> (ø)` | | | [unittests-flyteadmin](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `58.90% <ø> (ø)` | | | [unittests-flytecopilot](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `17.79% <ø> (ø)` | | | [unittests-flytectl](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `68.35% <100.00%> (+0.04%)` | :arrow_up: | | [unittests-flyteidl](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `79.30% <ø> (ø)` | | | [unittests-flyteplugins](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `61.94% <ø> (ø)` | | | [unittests-flytepropeller](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `57.32% <ø> (ø)` | | | [unittests-flytestdlib](https://app.codecov.io/gh/flyteorg/flyte/pull/5437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `65.82% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fellhorn commented 1 month ago

We noticed that this PR still needs some more work as it still fails with LaunchPlan outputs used for other nodes. Here is a minimal example:

from flytekit import LaunchPlan, task, workflow

@task
def my_task(inp: int) -> int:
    return 42 + inp

@workflow
def inner_workflow(inp: int) -> int:
    return my_task(inp=inp)

@workflow
def outer_workflow() -> int:
    res = LaunchPlan.get_or_create(inner_workflow, "name_override", default_inputs={"inp": 3})()
    return my_task(inp=res)

It fails with:

Compiling tasks...

Compiling workflow: wfs.wf.inner_workflow

Compiling workflow: wfs.wf.outer_workflow
:( Error Compiling workflow: wfs.wf.outer_workflow
Error: Collected Errors: 3
        Error 0: Code: ParameterNotBound, Node Id: n1, Description: Parameter not bound [inp].
        Error 1: Code: VariableNameNotFound, Node Id: n0, Description: Variable [inp] not found on node [n0].
        Error 2: Code: VariableNameNotFound, Node Id: n0, Description: Variable [o0] not found on node [n0].

We tried to understand what is missing/different in flytectl compile to how the admin handles workflow registration but have not found the root cause yet. Generally wondering if we need to compile the workflows in a certain order and somehow also compile/populate the LaunchPlans as they currently don't have the input and output infos.

@dav009 do you maybe have some time to look at this with us or give us some hints where to start?

fg91 commented 2 weeks ago

Closing in favour of https://github.com/flyteorg/flyte/pull/5463.