-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(sdk): Implement Pipeline Configuration with TTL #11269
base: master
Are you sure you want to change the base?
Conversation
70f4d6d
to
cc9690e
Compare
pipeline_config_json = json_format.ParseDict( | ||
{'pipelineConfig': { | ||
'pipelineTtl': pipelineConfig.get_ttl(), | ||
}}, platformSpec.platforms['kubernetes']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chensun the SDK code part of this PR correctly compiles and generates the Yaml IR as per the requirements we decided upon here.
Code: https://gist.github.com/rimolive/e0c31ccad1f6c963ec010c455b424afe
resulting IR yaml: https://gist.github.com/rimolive/fc900c8172e661a470a6890555159f97
Could you confirm that the hard coded specification of kubernetes
platform here would work? Does the platform key in the IR always stay the same, i.e., kubernetes
, or is it customized (to have it set as vertex
, for example)? If yes, then we can look into having this key dynamically specified and inserted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I fully understand the question. The key is hard coded for individual platforms, and the platform backend is expected to look after the key they recognize and ignore the ones they don't. For instance, the KFP backend extracts only kubernetes
key:
pipelines/backend/src/apiserver/template/v2_template.go
Lines 72 to 78 in 6a35ee5
// Pick out Kubernetes platform configs | |
var kubernetesSpec *pipelinespec.SinglePlatformSpec | |
if t.platformSpec != nil { | |
if _, ok := t.platformSpec.Platforms["kubernetes"]; ok { | |
kubernetesSpec = t.platformSpec.Platforms["kubernetes"] | |
} | |
} |
The specific feature you're adding here, TTL, doesn't apply to Vertex. So what you have in the code and IR links look good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for clarifying @chensun !
cc9690e
to
079d482
Compare
d07bf4a
to
961daf4
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Verified the changes and was able to generate main.yaml with the below fields:
|
Signed-off-by: Ricardo M. Oliveira <[email protected]>
961daf4
to
86f1025
Compare
Hold this PR until we get full implementation. /hold |
@@ -17,7 +17,9 @@ | |||
class PipelineConfig: | |||
"""PipelineConfig contains pipeline-level config options.""" | |||
|
|||
def __init__(self): | |||
pass | |||
def __init__(self, ttl=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would suggest adding a test to ensure compiled IR yaml has ttl fields as expected. Could use this test as a reference.
Description of your changes:
This PR introduces a new class
PipelineConfig
in KFP SDK to add pipeline-specific configurations with the first one: Pipeline TTL.Testing instructions
SDK
main.yaml
file:Checklist: