feat(backend): support yaml with platform-specific specs #8983
Conversation
chensun
changed the title
|
/test kubeflow-pipeline-backend-test |
|
/lgtm Thank you, @Linchin! |
| assert.Equal(t, expectedTemplate, templateV2Spec) | ||
| } | ||
|
|
||
| func TestNewTemplate_WithExecutorConfig(t *testing.T) { |
There was a problem hiding this comment.
nit: WithPlatformSpec? Cause IR contains executor config, also the message is called PlatformSpec:
pipelines/api/v2alpha1/pipeline_spec.proto
Line 1003 in dabc027
There was a problem hiding this comment.
Good point, I will rename the function.
| return nil, util.NewInvalidInputErrorWithDetails(ErrorInvalidPipelineSpec, "invalid v2 pipeline spec: root component is empty") | ||
| } | ||
| v2Spec.spec = &spec | ||
| } else if isKubernetesExecutorConfig(valueBytes) { |
There was a problem hiding this comment.
Again on the naming, maybe PlatformSpec instead of ExecutorConfig?
There was a problem hiding this comment.
Thank you, I just updated it.
| @@ -217,3 +271,16 @@ func (t *V2Spec) RunWorkflow(modelRun *model.Run, options RunWorkflowOptions) (u | |||
| executionSpec.SetPodMetadataLabels(util.LabelKeyWorkflowRunId, options.RunId) | |||
| return executionSpec, nil | |||
| } | |||
|
|
|||
| func isKubernetesExecutorConfig(template []byte) bool { | |||
There was a problem hiding this comment.
This method should check for key == kubernetes, there could be other platform specs in the future.
There was a problem hiding this comment.
What is the purpose of making sure the kubernetes exists? As we allow pipeline spec without platform spec here, we can probably let it pass with a non-kubernetes platform spec. WDYT?
There was a problem hiding this comment.
We don't need to make sure kubernetes exists, but we do need to prevent mis-recognize other platform specs as kubernetes spec. The current change in the PR would treat any platform spec as kubernetes spec.
There was a problem hiding this comment.
A question that may be unrelated: should we keep all platforms' configs in memory, or only pick out the k8s related one?
There was a problem hiding this comment.
I added a check for the key "Kubernetes".
|
/test kubeflow-pipeline-backend-test |
| if err != nil { | ||
| return false | ||
| } | ||
| _, ok := platformSpec.Platforms["kubernetes"] |
There was a problem hiding this comment.
The function name should match what it checks, per this line of code, only kubernetes platform spec should return True, if that's the intention, the function name should reflect kubernetes as well.
There was a problem hiding this comment.
Thank you! I just updated the function name.
|
[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 |
* support yaml with platform-specific info * run go mod tidy and fix comments * license, test and small fixes * fix integration test and add test * address comments * address comments
Description of your changes:
Fixes #8758
Checklist: