Add Katib Client in v1alpha2 #480
Conversation
|
|
||
| const ( | ||
| trialTemplatesName = "trial-template" | ||
| metricsCollectorTemplatesName = "metricscollector-template" |
There was a problem hiding this comment.
How about adding it in pkg/api/operators/apis/experiment/v1alpha2/constants.go? trialTemplate is already defined
|
@andreyvelich Most likely yes, we will have similar metrics collector templates. Can we rename the template as |
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| cl "sigs.k8s.io/controller-runtime/pkg/client" |
|
|
||
| ns := getNamespace(namespace...) | ||
| trialTemplates := &apiv1.ConfigMap{} | ||
|
|
There was a problem hiding this comment.
Suggest move getConfigMap into this file, too and then please call getConfigMap
| return nil | ||
| } | ||
|
|
||
| func (k *katibClient) GetTrialTemplates(namespace ...string) (map[string]string, error) { |
There was a problem hiding this comment.
for now, trial template can be stored in any configmap, not just trial-template
There was a problem hiding this comment.
Is there any way to get Trial templates configmap?
| } | ||
|
|
||
| func (k *katibClient) UpdateMetricsCollectorTemplates(newMCTemplates map[string]string, namespace ...string) error { | ||
|
|
|
Client code can be common util code which can be used by all components. I think, it should be moved out of experiment controller |
|
We can create a |
|
@johnugeorge @richardsliu What do you think about moving it into manager folder, like here: |
|
@andreyvelich I think we need something like that, but I am not sure why that directory is named "manager". For v1alpha1 it just contains:
Seems more like an util folder to me. |
Move templates const
|
@richardsliu Should we name this folder util in v1alpha2 ? |
|
@andreyvelich Sounds good. |
|
@richardsliu @johnugeorge Done |
|
/retest |
| @@ -27,4 +27,10 @@ const ( | |||
|
|
|||
| // Default value of Spec.TemplatePath | |||
| DefaultTrialTemplatePath = "defaultTrialTemplate.yaml" | |||
|
|
|||
| // Name of the configMap for Trial templates | |||
| TrialTemplatesName = "trial-template" | |||
There was a problem hiding this comment.
line no 23 has it added already
| TrialTemplatesName = "trial-template" | ||
|
|
||
| // Name of the configMap for MetricsCollector templates | ||
| MetricsCollectorTemplatesName = "metrics-collector-template" |
There was a problem hiding this comment.
Is this the default metric config map similar to trial?
There was a problem hiding this comment.
Yes, I will rename it, like for Trial template
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardsliu 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 |
I added Katib Client functionality in the util.
We need it for the UI, like we have here:
https://github.com/kubeflow/katib/blob/master/pkg/manager/v1alpha1/studyjobclient/studyjobclient.go.
Will we have config map with Trial templates and Metrics collector templates, like here:
https://github.com/kubeflow/katib/blob/master/manifests/v1alpha1/studyjobcontroller/metricsControllerConfigMap.yaml
https://github.com/kubeflow/katib/blob/master/manifests/v1alpha1/studyjobcontroller/workerConfigMap.yaml ?
I named these config maps as:
trial-templateandmetricscollector-template/assign @hougangliu @YujiOshima @johnugeorge
/cc @richardsliu
This change is