fix(sdk): Throw 'exit_task cannot depend on any other tasks.' error when an ExitHandler has a parameter dependent on other task #11005

hbelmiro · 2024-07-15T18:34:53Z

Due to #11005 (comment), depends on:

Resolves #10187

The issue happens when an ExitHandler has a parameter that depends on other task.

from kfp import dsl
from kfp.dsl import PipelineTaskFinalStatus

@dsl.component
def test_step_a(input: str) -> str:
    return "step-a"


@dsl.component
def test_step_b(input: str, status: PipelineTaskFinalStatus):
    print("step-b")


@dsl.component
def test_step_c(input: str) -> str:
    return "step-c"


@dsl.pipeline
def test_pipeline():
    pre_task = test_step_a(input="pre")
    exit_task = test_step_b(input=pre_task.output)  <--- Here's the problem
    with dsl.ExitHandler(exit_task=exit_task):
        test_step_c(input="test")

If we change that line to:

    exit_task = test_step_b(input="some string")

The pipeline will compile.

This PR aims to show the proper error message when that happens.
This may not be the right approach. I changed the code to check the ExitHandler inputs. It seems to me that

pipelines/sdk/python/kfp/dsl/structures.py

Line 417 in f328f0b

dependent_tasks: List[str]

should contain test_step_a though. But I'm not sure that attribute actually means what I think it does.

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

google-oss-prow · 2024-07-15T18:34:56Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

gmfrasca · 2024-07-15T19:54:05Z

sdk/python/kfp/dsl/tasks_group.py

+            return False
+
+        return any(
+            type(task_input) is PipelineParameterChannel


i believe it's more pythontic to use isinstance here instead of manually checking type

It depends. type will assert without inheritance. If this is the case, then it is fine.

type will assert without inheritance.

That said, I think we need isinstance here.

google-oss-prow · 2024-07-17T12:25:59Z

@hbelmiro: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test kfp-kubernetes-test-python310
/test kfp-kubernetes-test-python311
/test kfp-kubernetes-test-python312
/test kfp-kubernetes-test-python38
/test kfp-kubernetes-test-python39
/test kubeflow-pipeline-mkp-snapshot-test
/test kubeflow-pipeline-mkp-test
/test kubeflow-pipelines-backend-visualization
/test kubeflow-pipelines-component-yaml
/test kubeflow-pipelines-integration-v2
/test kubeflow-pipelines-manifests
/test kubeflow-pipelines-sdk-docformatter
/test kubeflow-pipelines-sdk-execution-tests
/test kubeflow-pipelines-sdk-isort
/test kubeflow-pipelines-sdk-python310
/test kubeflow-pipelines-sdk-python311
/test kubeflow-pipelines-sdk-python312
/test kubeflow-pipelines-sdk-python38
/test kubeflow-pipelines-sdk-python39
/test kubeflow-pipelines-sdk-yapf
/test test-kfp-runtime-code-python310
/test test-kfp-runtime-code-python311
/test test-kfp-runtime-code-python312
/test test-kfp-runtime-code-python38
/test test-kfp-runtime-code-python39
/test test-run-all-gcpc-modules
/test test-upgrade-kfp-sdk

The following commands are available to trigger optional jobs:

/test kfp-kubernetes-execution-tests
/test kubeflow-pipeline-upgrade-test
/test kubeflow-pipeline-upgrade-test-v2
/test kubeflow-pipelines-samples-v2

Use /test all to run the following jobs that were automatically triggered:

kfp-kubernetes-execution-tests
kfp-kubernetes-test-python310
kfp-kubernetes-test-python311
kfp-kubernetes-test-python312
kfp-kubernetes-test-python38
kfp-kubernetes-test-python39
kubeflow-pipelines-component-yaml
kubeflow-pipelines-sdk-docformatter
kubeflow-pipelines-sdk-execution-tests
kubeflow-pipelines-sdk-isort
kubeflow-pipelines-sdk-python310
kubeflow-pipelines-sdk-python311
kubeflow-pipelines-sdk-python312
kubeflow-pipelines-sdk-python38
kubeflow-pipelines-sdk-python39
kubeflow-pipelines-sdk-yapf
test-kfp-runtime-code-python310
test-kfp-runtime-code-python311
test-kfp-runtime-code-python312
test-kfp-runtime-code-python38
test-kfp-runtime-code-python39
test-run-all-gcpc-modules
test-upgrade-kfp-sdk

In response to this:

/retest kfp-kubernetes-execution-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

hbelmiro · 2024-07-19T13:09:16Z

/retest-required

hbelmiro · 2024-07-19T14:21:57Z

kubeflow-pipelines-sdk-execution-tests should not be failing. When I run those tests on GitHub Actions, they pass. (I tried that by copying the workflow from #10975).

hbelmiro · 2024-07-19T14:36:58Z

/hold

Due to #11005 (comment), depends on:

google-oss-prow · 2024-07-19T15:15:48Z

@hbelmiro: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
kfp-kubernetes-execution-tests	`541dfa1`	link	false	`/test kfp-kubernetes-execution-tests`
kubeflow-pipelines-sdk-execution-tests	`541dfa1`	link	true	`/test kubeflow-pipelines-sdk-execution-tests`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

hbelmiro · 2024-07-19T15:54:31Z

@gmfrasca @diegolovison can you guys take a second look, please?
Failing tests are due to #11005 (comment).

chensun

/lgtm
/approve

google-oss-prow · 2024-07-24T22:20:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~sdk/OWNERS~~ [chensun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…n ExitHandler has a parameter dependent on other task Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

HumairAK · 2024-07-25T12:53:08Z

/lgtm

hbelmiro · 2024-07-25T12:56:16Z

/unhold

The depending PRs were merged.

DharmitD

/lgtm

google-oss-prow bot added do-not-merge/work-in-progress size/S labels Jul 15, 2024

google-oss-prow bot requested review from chensun and connor-mccarthy July 15, 2024 18:35

hbelmiro mentioned this pull request Jul 15, 2024

[sdk] ExitHandler doesn't compile with task Inputs #10187

Closed

hbelmiro force-pushed the issue-10187 branch 2 times, most recently from d7006ab to efedc3b Compare July 15, 2024 19:09

hbelmiro marked this pull request as ready for review July 15, 2024 19:44

google-oss-prow bot removed the do-not-merge/work-in-progress label Jul 15, 2024

gmfrasca reviewed Jul 15, 2024

View reviewed changes

hbelmiro marked this pull request as draft July 15, 2024 21:08

google-oss-prow bot added the do-not-merge/work-in-progress label Jul 15, 2024

hbelmiro force-pushed the issue-10187 branch from efedc3b to db6f2db Compare July 17, 2024 13:10

google-oss-prow bot added size/L and removed size/S labels Jul 17, 2024

hbelmiro force-pushed the issue-10187 branch from db6f2db to e84c5be Compare July 17, 2024 13:31

google-oss-prow bot added size/S size/M and removed size/L size/S labels Jul 17, 2024

hbelmiro mentioned this pull request Jul 17, 2024

feat(backend): move comp logic to workflow params #10979

Merged

1 task

hbelmiro force-pushed the issue-10187 branch from 2c79fde to 2ede4cb Compare July 18, 2024 20:49

google-oss-prow bot added size/S and removed size/M labels Jul 19, 2024

hbelmiro marked this pull request as ready for review July 19, 2024 14:22

google-oss-prow bot removed the do-not-merge/work-in-progress label Jul 19, 2024

google-oss-prow bot added the do-not-merge/hold label Jul 19, 2024

chensun approved these changes Jul 24, 2024

View reviewed changes

google-oss-prow bot assigned chensun Jul 24, 2024

google-oss-prow bot added the lgtm label Jul 24, 2024

google-oss-prow bot added the approved label Jul 24, 2024

fix: Throw 'exit_task cannot depend on any other tasks.' error when a…

0936752

…n ExitHandler has a parameter dependent on other task Signed-off-by: hbelmiro <helber.belmiro@gmail.com>

hbelmiro force-pushed the issue-10187 branch from 541dfa1 to 0936752 Compare July 25, 2024 11:42

google-oss-prow bot removed the lgtm label Jul 25, 2024

google-oss-prow bot assigned HumairAK Jul 25, 2024

google-oss-prow bot added the lgtm label Jul 25, 2024

google-oss-prow bot removed the do-not-merge/hold label Jul 25, 2024

DharmitD reviewed Jul 25, 2024

View reviewed changes

google-oss-prow bot assigned DharmitD Jul 25, 2024

google-oss-prow bot merged commit 08185e7 into kubeflow:master Jul 25, 2024
25 checks passed

hbelmiro deleted the issue-10187 branch July 25, 2024 13:33

DharmitD mentioned this pull request Aug 22, 2024

chore: Adding @DharmitD to SDK reviewers #11131

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sdk): Throw 'exit_task cannot depend on any other tasks.' error when an ExitHandler has a parameter dependent on other task #11005

fix(sdk): Throw 'exit_task cannot depend on any other tasks.' error when an ExitHandler has a parameter dependent on other task #11005

hbelmiro commented Jul 15, 2024 •

edited

Loading

google-oss-prow bot commented Jul 15, 2024

gmfrasca Jul 15, 2024

diegolovison Jul 15, 2024 •

edited

Loading

hbelmiro Jul 15, 2024

google-oss-prow bot commented Jul 17, 2024

hbelmiro commented Jul 19, 2024

hbelmiro commented Jul 19, 2024

hbelmiro commented Jul 19, 2024

google-oss-prow bot commented Jul 19, 2024

hbelmiro commented Jul 19, 2024

chensun left a comment

google-oss-prow bot commented Jul 24, 2024

HumairAK commented Jul 25, 2024

hbelmiro commented Jul 25, 2024

DharmitD left a comment

fix(sdk): Throw 'exit_task cannot depend on any other tasks.' error when an ExitHandler has a parameter dependent on other task #11005

fix(sdk): Throw 'exit_task cannot depend on any other tasks.' error when an ExitHandler has a parameter dependent on other task #11005

Conversation

hbelmiro commented Jul 15, 2024 • edited Loading

google-oss-prow bot commented Jul 15, 2024

gmfrasca Jul 15, 2024

Choose a reason for hiding this comment

diegolovison Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

hbelmiro Jul 15, 2024

Choose a reason for hiding this comment

google-oss-prow bot commented Jul 17, 2024

hbelmiro commented Jul 19, 2024

hbelmiro commented Jul 19, 2024

hbelmiro commented Jul 19, 2024

google-oss-prow bot commented Jul 19, 2024

hbelmiro commented Jul 19, 2024

chensun left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jul 24, 2024

HumairAK commented Jul 25, 2024

hbelmiro commented Jul 25, 2024

DharmitD left a comment

Choose a reason for hiding this comment

hbelmiro commented Jul 15, 2024 •

edited

Loading

diegolovison Jul 15, 2024 •

edited

Loading