Refactor Python SDK #568
Refactor Python SDK #568
Conversation
|
/test kubeflow-pipeline-sample-test |
1 similar comment
|
/test kubeflow-pipeline-sample-test |
| @@ -140,9 +145,9 @@ def _op_to_template(self, op): | |||
| template['nodeSelector'] = op.node_selector | |||
|
|
|||
| if op.env_variables: | |||
| template['container']['env'] = list(map(self._convert_k8s_obj_to_dic, op.env_variables)) | |||
| template['container']['env'] = list(map(K8sHelper.convert_k8s_obj_to_json, op.env_variables)) | |||
There was a problem hiding this comment.
The output in not really JSON. It's a python dict.
P.S. The function (including the name comes from the K8s client library (ApiClient class))
There was a problem hiding this comment.
I changed the name as yang's docstring inside the function, which is a json serialized dictionary that has nested structure.
|
|
||
| Args: | ||
| value_or_reference: value or reference to be resolved. It could be basic python types or PipelineParam | ||
| potential_references(dict{str->str}): a dictionary of parameter names to task names |
There was a problem hiding this comment.
Thanks for clarifying this.
I think it can be made even more clear. Why are the references potentials? What kind parameter names are these?
P.S. The python dictionary type is:from typing import Mapping, potential_references: Mapping[str, str]. Is the type correct though?
There was a problem hiding this comment.
I'll sending some more refactoring PRs recently to make DSL more readable.
Will note this. Thanks for your feedback.
|
I have an ask: Can we make is so that the compiler is executable from source code clone without installing the package? It's probably a really small fix. |
|
Interesting idea. It might be useful for DSL development work. |
|
/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 |
1 similar comment
|
[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 |
…beflow#568) This reverts commit 4ebe571. The list of service accounts only includes service accounts owned by the project. So we ended up deleting bindings for service accounts owned by other projects such as the service accounts. related to: kubeflow#566
Add pipelineloop-controller, pipelineloop-webhook and anysequencer to the toolchain pipelines
Add some comments; refactor convert_k8s_obj_to_json; remove unnecessary imports; add license to dsl_bridge.
As the Python SDK codebase becomes larger, it requires some cleaning to make it more readable.
This change is