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.

Sign up for GitHub

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

Jump to bottom

fix(sdk): Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest #7141

Merged
merged 5 commits into from
Jan 20, 2022
Merged

fix(sdk): Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest #7141

merged 5 commits into from
Jan 20, 2022

Conversation

SebBlin
Copy link
Contributor

@SebBlin SebBlin commented Jan 5, 2022

No description provided.

@google-oss-prow google-oss-prow bot requested review from chensun and ji-yaqi January 5, 2022 12:51
@google-oss-prow
Copy link

Hi @SebBlin. 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.

@SebBlin SebBlin changed the title Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest fix(sdk): Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest Jan 5, 2022
@SebBlin SebBlin mentioned this pull request Jan 5, 2022
Closed
Arhell
Arhell reviewed Jan 6, 2022
View reviewed changes
Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ok-to-test

@SebBlin SebBlin requested a review from Arhell January 6, 2022 14:39
@SebBlin
Copy link
Contributor Author

SebBlin commented Jan 10, 2022

Hi @Arhell I'm a newbie and this is my first PR. Am I supposed to do something else to make this PR progress or is it fine for my ? Sorry for this question.

@chensun chensun removed the request for review from Arhell January 11, 2022 05:45
@chensun chensun self-assigned this Jan 11, 2022
chensun
chensun reviewed Jan 11, 2022
View reviewed changes
sdk/python/kfp/compiler/compiler.py
@@ -832,8 +832,8 @@ def _create_pipeline_workflow(self,

# set ttl after workflow finishes
if pipeline_conf.ttl_seconds_after_finished >= 0:
workflow['spec'][
'ttlSecondsAfterFinished'] = pipeline_conf.ttl_seconds_after_finished
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a breaking change, do we know from which Argo version was this change introduced? Consequently, which KFP version has that Argo upgrade? We'll need to document this change in the release note.

sdk/python/kfp/compiler/compiler.py Outdated
@@ -832,8 +832,8 @@ def _create_pipeline_workflow(self,

# set ttl after workflow finishes
if pipeline_conf.ttl_seconds_after_finished >= 0:
workflow['spec'][
'ttlSecondsAfterFinished'] = pipeline_conf.ttl_seconds_after_finished
workflow['spec']['ttlStrategy'] = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the following line can be combined into this one as
workflow['spec']['ttlStrategy'] = {'secondsAfterCompletion' = pipeline_conf.ttl_seconds_after_finished}

@SebBlin
Copy link
Contributor Author

SebBlin commented Jan 11, 2022

Here are details about this breaking change:

I'm not sure where to put this in the release note; I would put it in "Current Version"/"Bug Fixes and Other Changes" as this is a fix that correct a breaking from Argo; it doesn't introduce a breaking change in KFP.
Is this correct?

@chensun
Copy link
Member

chensun commented Jan 19, 2022

Here are details about this breaking change:

I'm not sure where to put this in the release note; I would put it in "Current Version"/"Bug Fixes and Other Changes" as this is a fix that correct a breaking from Argo; it doesn't introduce a breaking change in KFP. Is this correct?

Thanks for the info. On KFP side, upgrading Argo workflow from 2.x to 3.x was done by #5922, which in in KFP (backend) 1.7.0-alpha.1 release per the changelog. That means this SDK change would make the next KFP SDK work with KFP backend 1.7+, but incompatible with earlier version. In that sense, it's a breaking change for KFP pre-1.7 versions. Does that make sense?

@SebBlin
Update RELEASE.md
5de15d7 
Add Braking change due to incompatibility with  KFP pre 1.7 due to Argo 2.X
@SebBlin
Copy link
Contributor Author

SebBlin commented Jan 19, 2022

That means this SDK change would make the next KFP SDK work with KFP backend 1.7+, but incompatible with earlier version. In that sense, it's a breaking change for KFP pre-1.7 versions. Does that make sense?

Indeed, you are completely right. I updated the changelog accordingly

@chensun
Copy link
Member

chensun commented Jan 20, 2022

/lgtm
/approve

Thanks!

@google-oss-prow google-oss-prow bot added the lgtm label Jan 20, 2022
@google-oss-prow
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-prow google-oss-prow bot merged commit 577902c into kubeflow:master Jan 20, 2022
chensun pushed a commit to chensun/pipelines that referenced this pull request Jan 21, 2022
@SebBlin @chensun
fix(sdk): Support per workflow TTL (ttl_seconds_after_finished) with …
4bdad85 
…new format of Argo workflow manifest (kubeflow#7141)

* Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest

* Update test for TTL

* Declare fix in release note of SDK

* update syntaxe

* Update RELEASE.md

Add Braking change due to incompatibility with  KFP pre 1.7 due to Argo 2.X
chensun added a commit that referenced this pull request Jan 24, 2022
@chensun @casassg @SebBlin
chore(sdk1.8): cherry pick #6866, #7056, #7141, #7143, #7168, #7172, #…
7b2ca38 
…7182 into sdk/release-1.8 branch. (#7200)

* test: fix tfx presubmit test. (#7143)

* fix(sdk): Respect namespace if set on client (#7056)

* respect namespace if set on client

* make sure is_multi_user

* overwrite directly context_setting

* revert non-related changes

* revert overwrite_namespace

* expand text

* add release notes

* add PR link

* fix(sdk): Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest (#7141)

* Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest

* Update test for TTL

* Declare fix in release note of SDK

* update syntaxe

* Update RELEASE.md

Add Braking change due to incompatibility with  KFP pre 1.7 due to Argo 2.X

* chore(sdk): Remove redundant check in set_gpu_limit (#6866)

* remove redundant check in set_gpu_limit

* release note

* fix(sdk): Fix create_runtime_artifact not covering all types. (#7168)

* Fix create_runtime_artifact not covering all types.

* release note

* fix(sdk): bumps up absl version to 1.x for compatibility with TF 2.8+. Fixes #7171 (#7172)

* Bumps up absl version to 1.x for compatibility with TF 2.8+.

* Adds a release note for absl version.

* chore(sdk): Update SDK release note for the upcoming 1.8.11 release (#7182)

Co-authored-by: Gerard Casas Saez <[email protected]>
Co-authored-by: Seb <[email protected]>
Co-authored-by: Jiyong Jung <[email protected]>
This was referenced Mar 24, 2022
Closed
Merged
abaland pushed a commit to abaland/pipelines that referenced this pull request May 29, 2022
@SebBlin @abaland
fix(sdk): Support per workflow TTL (ttl_seconds_after_finished) with …
975f306 
…new format of Argo workflow manifest (kubeflow#7141)

* Support per workflow TTL (ttl_seconds_after_finished) with new format of Argo workflow manifest

* Update test for TTL

* Declare fix in release note of SDK

* update syntaxe

* Update RELEASE.md

Add Braking change due to incompatibility with  KFP pre 1.7 due to Argo 2.X
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arhell Arhell Arhell left review comments

@chensun chensun chensun left review comments

@ji-yaqi ji-yaqi Awaiting requested review from ji-yaqi

Assignees

@chensun chensun

Labels
approved lgtm ok-to-test size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SebBlin @chensun @Arhell