Skip to content
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

WIP: support worker and metricsCollector template spec #375

Closed
wants to merge 1 commit into from
Closed

WIP: support worker and metricsCollector template spec #375

wants to merge 1 commit into from

Conversation

hougangliu
Copy link
Member

@hougangliu hougangliu commented Feb 14, 2019

Fixes: #349


This change is Reviewable

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: hougangliu

If they are not already assigned, you can assign the PR to them by writing /assign @hougangliu in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hougangliu hougangliu changed the title support worker and metricsCollector template spec WIP: support worker and metricsCollector template spec Feb 14, 2019
wtp, err = template.New("Worker").Parse(wt)
tName, tNamespace, tPath := getDefaultTemplateSpec()
if goTemplate.TemplateSpec != nil {
tName = goTemplate.TemplateSpec.ConfigMapName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the field of TemplateSpec is emply, we should use the default value.
For example, we can omit the namespace of ConfigMap when we want to use the same namespace as the studyjobcontroller.

@@ -114,19 +114,25 @@ const (
OptimizationTypeMaximize OptimizationType = "maximize"
)

type GoTemplate struct {
type TemplateSpec struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extension of studyjob API would be for v1alpha2? #370

@YujiOshima
Copy link
Contributor

This issue is listed v1alpha2 API #370 .
@johnugeorge Should we make a branch for v1alpha2 and this will be merged to the branch?

@k8s-ci-robot
Copy link

@hougangliu: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
kubeflow-katib-presubmit 0b76b20 link /test kubeflow-katib-presubmit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@johnugeorge
Copy link
Member

Yes. I would suggest that only common changes(that won't break the current config) go into v1alpha1 at this point. @richardsliu is working to get v1alpha2 structure. Once it is merged, we can get it in v1alpha2

In general, I feel that we are moving away from the data scientist point of view, instead thinking from a kubernetes developer perspective. Currently, a data scientist will find it extremely difficult to write a katib study job config correctly without good kubernetes experience. For eg: Currently in the studyjob config, there is a namespace for overall studyjob, a namespace for the workerspec, namespace for ConfigMap, namespace for metric collector . Won't this be complicated for the users by exposing much details? WDYT?

@hougangliu hougangliu closed this Feb 14, 2019
@hougangliu
Copy link
Member Author

will submit another PR for v1alpha2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants