DSL refactor #619
DSL refactor #619
Conversation
…build_conventional_artifact as a nested function
sdk/python/kfp/dsl/_pipeline.py
Outdated
| @@ -108,7 +105,7 @@ def add_op(self, op: _container_op.ContainerOp, define_only: bool): | |||
| op: An operator of ContainerOp or its inherited type. | |||
| """ | |||
|
|
|||
| kubernetes_name = _make_kubernetes_name(op.human_name) | |||
| kubernetes_name = _sanitize_k8s_name(op.human_name) | |||
There was a problem hiding this comment.
I missed the previous change, so I am adding my thoughts here: one design goal is to hide k8s as much as possible in "dsl" layer, and push the k8s stuff to compiler (I used to call it "argo compiler). This way the DSL layer is more generic, and that's why there are "dsl" and "compiler" separate directories.
I feel like we don't have to sanitize the pipeline name here; We can store it as it is (so respect user's choice) and in compiler https://github.com/kubeflow/pipelines/blob/master/sdk/python/kfp/compiler/compiler.py#L457 we can sanitize there. That way, we can move the util to compiler since it is very k8s specific.
WDYT?
There was a problem hiding this comment.
I'm all for the DSL hiding k8s. However, the pipeline sanitizes the name for operators which will be part of the final argo yaml. (in Pipeline.add_op() function).
I can sanitize the op names all in the compiler codes, though.
There was a problem hiding this comment.
Ah yes. Sorry I mixed pipeline name with step name. I think we can sanitize the name in compiler too as you mentioned.
That way, a different "compiler" may choose to sanitize it in a different way, or even not sanitize it at all.
There was a problem hiding this comment.
The Pipeline class (do not confuse with the @pipeline decorator) is a compiler helper class. It can probably moved to DSL compiler. It's only used during the compilation and it makes sense to sanitize the names/ids at this point. The original name remains untouched in op.human_name
There was a problem hiding this comment.
The Pipeline class is used in the @pipeline decorator and it might not be a good idea to move the Pipeline class to the Compiler because the dsl would depend on the compiler otherwise. Then there would be a dependency loop since the compiler depends on the dsl library. Right?
There was a problem hiding this comment.
The Pipeline class is used in the @pipeline decorator
This is an implementation detail. We've talked with @qimingj and AFAIK he agreed that it was a good idea to unlink them like we did with @python_component. I've prepared the code for the most common case, but there was a slight problem for multi-pipeline file compilation. If we deprecate that feature (it's not currently used anywhere) we can unlink them easier.
There was a problem hiding this comment.
That way, a different "compiler" may choose to sanitize it in a different way, or even not sanitize it at all.
I agree with that. That's why I moved the sanitization code from the ContainerOp to the compiler-relater Pipeline class. As you remember, I tried to detangle the ContainerOp from the Pipeline even more, by making it not required.
There was a problem hiding this comment.
Here is a proposal: Let's break the dependency between the DSL classes and the compiler by adding generic events/hooks for events like ContainerOp creation or @pipeline application. The compiler can set the handlers for those hooks to execute some compiler-specific code. This way, the DSL does not depend on the compiler.
There was a problem hiding this comment.
@Ark-kun can you send a separate change and we can discuss more details? I feel it should be a separate change from this one. Small changes are more manageable.
The reason we had a "Pipeline" class are: 1) we need a "scope" that can record all ContainerOp objects. Hence the global Pipeline._default_pipeline variable. 2) Someday we can expose the Pipeline class to support dynamic pipeline construction in DSL (we don't expose that now because we want to limit the surfacing area and promote pipeline function).
Events/hooks is one option. We can compare the approaches by 1) Keep DSL as simple as it is now.
2) Favor simplicity for compiler provider. 3) Hopefully reduce or remove global variables.
|
/lgtm |
|
/lgtm |
|
/test kubeflow-pipeline-build-image |
|
/test kubeflow-pipeline-e2e-test |
| # Sanitize operator names and param names | ||
| sanitized_ops = {} | ||
| for op in p.ops.values(): | ||
| sanitized_name = K8sHelper.sanitize_k8s_name(op.name) |
There was a problem hiding this comment.
AFAIK op.name is already sanitized and made unique when it's being added to the pipeline.
There was a problem hiding this comment.
Oh. You've removed that sanitization.
I'm not sure this is a good move. It's easier to fix the name before all the output references have been generated and passed around.
There was a problem hiding this comment.
The motivation was to move the k8s related functions from DSL to compilers such that the implementations of another compiler in the future will not depend on K8s. The PR aims to moves the sanitization to the compiler.
There was a problem hiding this comment.
The motivation was to move the k8s related functions from DSL to compilers such that the implementations of another compiler in the future will not depend on K8s. The PR aims to moves the sanitization to the compiler.
I agree with both those ideas. What's debatable is whether the Pipeline class belongs to the compiler or DSL.
| if param.op_name: | ||
| param.op_name = K8sHelper.sanitize_k8s_name(param.op_name) | ||
| for param in op.outputs.values(): | ||
| param.name = K8sHelper.sanitize_k8s_name(param.name) |
There was a problem hiding this comment.
Are you sure this will work at this stage? The argument placeholders are probably already embedded into op.command and op.args as strings.
There was a problem hiding this comment.
Good point. The op.args are handled in the _op_to_template function in this PR.
However, I have not handled the op.command. Do we already support parameterized commands?
There was a problem hiding this comment.
I'll update this PR for the command as well.
…h the whole serialized param str, Verify both param name and container name
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaoning777 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaoning777 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 |
* There is a bug in the code to detect completed jobs. For completed jobs the condition can be of type "Complete". As a result, we aren't properly detecting when jobs have been completed and rerunning them if needed. Related to kubeflow#762
* add init github actions * Remove old comments * Update test_kfp_samples.sh * add condition for venv
combine two sanitize name function into one;
add more comments;
Aggregate certain function with similar functionality.
This change is