Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sdk compiler):Fix v2 compile for exit handler Fixes #5854 #5899

Merged

Conversation

TrsNium
Copy link
Contributor

@TrsNium TrsNium commented Jun 22, 2021

Description of your changes:
If exit_handler is used, an error will occur at compile time because inputs are not set for tasks in that group(#5854)
So, set input as well as condition subgroup.

Checklist:

@google-oss-robot
Copy link

Hi @TrsNium. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@chensun
Copy link
Member

chensun commented Jun 22, 2021

/ok-to-test
/lgtm

Thank you @TrsNium for the fix! I actually plan to fix this today, but you beat me :)

Can you please also update the test sample:

def my_pipeline():
exit_task = print_op('Exit handler has worked!')
with dsl.ExitHandler(exit_task):
print_op('Hello World!')
fail_op('Task failed.')

You can change the 'Hello World!' constant to be a pipeline input param. For instance:

def my_pipeline(message: str = 'Hello World!'):

  exit_task = print_op('Exit handler has worked!')

  with dsl.ExitHandler(exit_task):
    print_op(message)
    fail_op('Task failed.')

Once done, you can update the golden json by executing the file: python3 pipeline_with_exit_handler.py

@TrsNium
Copy link
Contributor Author

TrsNium commented Jun 22, 2021

@chensun
Thank you for your review!
I updated test sample, please check it!
(Thank you for the information about the test😄 .)

@TrsNium TrsNium force-pushed the fix/v2-compiler-exit-handler branch from 09c6c11 to 67fe5cd Compare June 22, 2021 08:27
@TrsNium TrsNium force-pushed the fix/v2-compiler-exit-handler branch from 67fe5cd to bb8f5d6 Compare June 22, 2021 08:32
@chensun
Copy link
Member

chensun commented Jun 22, 2021

Thanks again for your contribution. This "punch the hole" thing is a tricky one. Very impressive that you were able to figure this out by yourself.

/lgtm
/approve

@google-oss-robot
Copy link

[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:

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

@google-oss-robot google-oss-robot merged commit a1740b4 into kubeflow:master Jun 22, 2021
@TrsNium TrsNium deleted the fix/v2-compiler-exit-handler branch June 22, 2021 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants