Add Katib Bundle for Juju #1403
Conversation
Adds Python operators for Katib, corresponding to the latest Katib manifests. Adds an `operators` folder with an OWNERS file to hold the operators.
There was a problem hiding this comment.
Thank you so much for doing this @knkski!
I left few comments.
/cc @gaocegege @johnugeorge @RFMVasconcelos
/cc @aronchick
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "medianstop": { | |||
| "image": "docker.io/kubeflowkatib/earlystopping-medianstop", | |||
There was a problem hiding this comment.
Do we want to maintain the latest Katib release or the latest pushed commit, e.g. 91e4996 for the Juju bundle ?
In the kubeflow/manifests we maintain the latest release version. Right now, it is v0.10.0 with commit a96ff59, check here: https://github.com/kubeflow/manifests/blob/master/katib/components/katib-controller/kustomization.yaml#L16-L22.
What do you think @gaocegege @johnugeorge @knkski @RFMVasconcelos ?
There was a problem hiding this comment.
Ideally, I'd like to do both. If someone deploys the latest/stable version, they'd get the latest stable release. If they deploy latest/edge instead, they'd get the latest pushed commit.
There was a problem hiding this comment.
@knkski Can we set-up it via Juju somehow ?
There was a problem hiding this comment.
That is a very good question, my view is that we'll always want to be consistent with the latest manifests on the repo, so we are aligned for every Katib release.
As @knkski mentions, from a Juju standpoint, it is possible to deploy a stable version and an edge version. The easiest IMO would be to have this repo always represent edge and point the stable version to the latest Katib release commit.
There was a problem hiding this comment.
Sounds good @RFMVasconcelos. Let's keep the edge version in this repo.
What do you think about adding info for the user how to change image version to switch from edge to stable version in the README ?
There was a problem hiding this comment.
That's a good point @andreyvelich!
The best UX in my opinion would be that juju deploy katib would as a default install stable and only if the user wants to try edge he will have to add the --channel=edge handle.
Adding this to the README!
k8s-ci-robot
requested review from
aronchick,
gaocegege,
johnugeorge and
rui-vas
Update README.md
|
@andreyvelich: Thanks for reviewing, I believe everything's been addressed and updated |
Thanks, I have only one question to the README update. What do you think about the OWNERS file, as I mentioned here: #1403 (comment) ? I will create a separate PR to fix it for the root OWNERS. |
|
@andreyvelich: Sure, pushed that fix up. And sorry, misunderstood what you said, thought you meant to leave as-is and both |
There was a problem hiding this comment.
@knkski @RFMVasconcelos Thank you again for this contribution, it is great!
LGTM from my side.
/lgtm
Hold for the review from @gaocegege or @johnugeorge.
/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, knkski 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 |
|
/lgtm |
|
Let's merge it. |
Adds Python operators for Katib, corresponding to the latest Katib manifests.
What this PR does / why we need it:
As per #1400, we are contributing the python operators for Katib, updated to match the latest katib manifests. Creates an
operatorsfolder with anOWNERSfile with @knkski and @RFMVasconcelos, since the KatibOWNERSfile will have overarching permissions.Which issue(s) this PR fixes:
Fixes #1400
Release note: