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

fellhorn commented 3 weeks ago

Why are the changes needed?

from flytekit import LaunchPlan, task, workflow

@task
def my_task(num: int) -> int:
    return num + 1

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

@workflow
def outer_workflow() -> int:
    return LaunchPlan.get_or_create(inner_workflow, "name_override", default_inputs={"num": 42})()

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.

To do so, one needs to compile workflows in the correct order, which I do by recursively calling a new handleWorkflow function. Please point me to other places if there is already code handling this.

As far as I have checked there are no recursion checks necessary as they would be caught during packaging?

How was this patch tested?

Added a unit test based on the example above. Additionally we tried the flytectl compile command on a number of our internal workflows.

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

[ ] I updated the documentation accordingly.
[x] All new and existing tests passed.
[x] All commits are signed-off.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.

Project coverage is 60.98%. Comparing base (59e18d1) to head (d5754b6). Report is 1 commits behind head on master.

Files	Patch %	Lines
flytectl/cmd/compile/compile.go	86.11%	4 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #5463 +/- ## ========================================== + Coverage 60.97% 60.98% +0.01% ========================================== Files 793 793 Lines 51350 51378 +28 ========================================== + Hits 31312 31335 +23 - Misses 17152 17156 +4 - Partials 2886 2887 +1 ``` | [Flag](https://app.codecov.io/gh/flyteorg/flyte/pull/5463/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/5463/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/5463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `58.66% <ø> (ø)` | | | [unittests-flytecopilot](https://app.codecov.io/gh/flyteorg/flyte/pull/5463/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/5463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `68.04% <86.11%> (+0.07%)` | :arrow_up: | | [unittests-flyteidl](https://app.codecov.io/gh/flyteorg/flyte/pull/5463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `79.04% <ø> (ø)` | | | [unittests-flyteplugins](https://app.codecov.io/gh/flyteorg/flyte/pull/5463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `61.84% <ø> (ø)` | | | [unittests-flytepropeller](https://app.codecov.io/gh/flyteorg/flyte/pull/5463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `57.30% <ø> (ø)` | | | [unittests-flytestdlib](https://app.codecov.io/gh/flyteorg/flyte/pull/5463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=flyteorg) | `65.80% <ø> (ø)` | | 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 2 weeks ago

The docs generation in CI seems to be broken, I saw other Actions fail with the same error at the moment.

davidmirror-ops commented 2 weeks ago

@fellhorn the docs CI test failure should be fixed by https://github.com/flyteorg/flyte/pull/5468 so if you rebase and merge it should do the trick

fellhorn commented 2 weeks ago

@fellhorn the docs CI test failure should be fixed by #5468 so if you rebase and merge it should do the trick

Rebased, thanks for the info :+1:

flyteorg / flyte