Katib ksonnet package #873
Conversation
|
@gaocegege @YujiOshima @jlewi PTAL |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/cc @jlewi |
|
@lluunn Sorry for the delay. Just noticed ithis because it popped up in my dashboard finally. If you want me to review something /assign me. @ mentions will get buried in my inbox. |
| }, | ||
| }, // dbDeployment | ||
|
|
||
| frontendService: { |
There was a problem hiding this comment.
Either in this PR or a follow on one we should add an Ambassador prefix to access the front end.
There was a problem hiding this comment.
I plan to do that in next PR. We have a different issue kubeflow/katib#88
kubeflow/katib/modeldb.libsonnet
Outdated
| value: "/katib", | ||
| }, | ||
| ], | ||
| image: "katib/katib-frontend", |
There was a problem hiding this comment.
Docker images should be exposed by parameters to make it easy to try new images.
kubeflow/katib/modeldb.libsonnet
Outdated
| }, | ||
| ], | ||
| image: "katib/katib-frontend", | ||
| imagePullPolicy: "Always", |
There was a problem hiding this comment.
Default imagePullPolicy should be IfNotPresent. Always is not good practice; we want to use the local cache.
kubeflow/katib/modeldb.libsonnet
Outdated
| }, | ||
| }, // frontendDeployment | ||
|
|
||
| frontendIngress: { |
There was a problem hiding this comment.
Lets get rid of the ingress. Our networking story is that services are exposed via the Ambassador reverse proxy. I don't want to start creating ingress per service until we better understand the motivation for doing so.
|
|
||
| // updatedParams uses the environment namespace if | ||
| // the namespace parameter is not explicitly set | ||
| local updatedParams = params { |
There was a problem hiding this comment.
You do not need this stanza. env should always contain environment because we are now require versions of ksonnet that ensure its true.
There was a problem hiding this comment.
I don't get it. Do you mean we should always use namespace from env?
I think the current logic is correct?
There was a problem hiding this comment.
Correct you can just get namespace from env.
The logic is correct but unnecessary.
In earlier versions of ksonnet env wasn't available. So the way to get namespace was to make it available via a params.
A later version of ksonnet introduced env which contains namespace. But for backwards compatibility we still wanted to respect namespace in params. But we no longer need to do that.
There was a problem hiding this comment.
But then all components in one environment will have to have the same namespace?
Shouldn't we allow setting namespace per component?
There was a problem hiding this comment.
Perhaps in the future. There's an implicit assumption that everything is already in the same namespace. So I wouldn't expect setting different namespaces would just work.
kubeflow/katib/modeldb.libsonnet
Outdated
| args: [ | ||
| "modeldb-db", | ||
| ], | ||
| image: "mitdbg/modeldb-backend:latest", |
There was a problem hiding this comment.
parameterize the image please
kubeflow/katib/vizier.libsonnet
Outdated
| "-i", | ||
| "k-cluster.example.net", | ||
| ], | ||
| image: "katib/vizier-core", |
There was a problem hiding this comment.
parameterize the image please
|
/assign jlewi |
|
Thanks for reviewing, PTAL. Addressed the comments, except one question. |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi 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 |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
* wip * fix jsonnet * formatting * address comments * fix * remove namespace from param
* wip * fix jsonnet * formatting * address comments * fix * remove namespace from param
* feat: Support random search Signed-off-by: Ce Gao <gaoce@caicloud.io> * feat: Update docs Signed-off-by: Ce Gao <gaoce@caicloud.io>
Katib ksonnet components, following what are deploying in
https://github.com/kubeflow/katib/blob/master/scripts/deploy.sh
Fix kubeflow/katib#32
This change is