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

Follow KF manifests guidelines #69

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Apr 20, 2024

As discussed in the last Kubeflow Manifest WG meeting (2024-04-18), this PR aims to properly setup manifests folder to follow the Kubeflow manifests guidelines

Description

Given that, with kubeflow/manifests#2682 we are going to add a custom script to sync Kubeflow Model Registry manifests from this repository to the Kubeflow Manifests one. In this PR I am adding missing files to follow the aforementioned Kubeflow manifests guidelines.

These are the requirements for all components under /contrib:

  • There must be a README.md file that documents:
    • Instructions on how someone can install the component in a Kubeflow cluster
      • Since Kubeflow manifests have standardized on Kustomize
        we expect all manifests to be a kustomize packages
    • How to use the component as part of Kubeflow (examples)
    • The problems it tries to solve and the value it brings
    • Links to the official documentation of the component
  • There must be an OWNERS file with at least 2 users
  • There must be an UPGRADE.md file that documents any instructions users need
    to follow when applying manifests of a newer version
  • There needs to be sufficient work on testing
    • There must be a script file [python, bash etc] that vefiries the component
      is working as expected. This can be something very simple, like submitting a
      CustomResource and waiting for it to become Ready
    • The maintainers will need to work with the leads of Manifests WG to ensure
      there's some basic automation in place that will be running the above script(s)
    • [edit] This is going to be addressed with Setup KF Model Registry GH tests manifests#2694

How Has This Been Tested?

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
@lampajr
Copy link
Member Author

lampajr commented Apr 20, 2024

Hi @tarilabs @rimolive , this is a first draft aiming to fill most of the gaps in order to follow Kubeflow Manifests guideline.

In the PR description, I summarized what can be done as part of this PR but as initial step I created a simple README and OWNERS file (lmk which members you'd like to put there).

What I don't know yet are:

  • If we want to include a simple test.sh script
  • If we have an upgrade script (don't think so but let me know)

NOTE: I decided to create the PR directly in Model Registry repository as the script that will be added as part of kubeflow/manifests#2682 will copy the whole manifests/kustomize folder.. so easier to keep everything here such that we have a single source of truth.

NOTE2: I linked the documentation of model registry based on the currently open pull request kubeflow/website#3698, this means that those links will work only when the Kubeflow website will be updated with those changes

@tarilabs
Copy link
Member

There needs to be sufficient work on testing

When I was looking into this I was thinking of:

- name: Create Test Registry
env:
IMG: "${{ env.IMG_ORG }}/${{ env.IMG_REPO }}:${{ steps.tags.outputs.tag }}"
run: |
echo "Download kustomize 5.2.1"
mkdir $GITHUB_WORKSPACE/kustomize
curl -s "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s "5.2.1" "$GITHUB_WORKSPACE/kustomize"
PATH=$GITHUB_WORKSPACE/kustomize:$PATH
echo "Display Kustomize version"
kustomize version
echo "Deploying Model Registry using Manifests; branch ${BRANCH}"
kubectl create namespace kubeflow
cd manifests/kustomize/overlays/db
kustomize edit set image kubeflow/model-registry:latest $IMG
kustomize build | kubectl apply -f -
- name: Wait for Test Registry Deployment
run: |
kubectl wait --for=condition=available -n kubeflow deployment/model-registry-db --timeout=5m
kubectl wait --for=condition=available -n kubeflow deployment/model-registry-deployment --timeout=5m
kubectl logs -n kubeflow deployment/model-registry-deployment

which is done with every PR, mimics what there is in the readme which you contributed, and for me it's spot-on; what are your thoughts on this point @rimolive please?

@lampajr
Copy link
Member Author

lampajr commented Apr 22, 2024

There must be an UPGRADE.md file that documents any instructions users need
to follow when applying manifests of a newer version

@tarilabs regarading this point, is there any upgrade process at the moment? I don't think so, as we do not have a release yet, but I'd like a confirmation on this point.

Anyway based on the other components, I think that for this we could simply rely on the official Kubeflow documentation (and put any instruction there, or even here in the README that I am adding with this PR).

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

thanks @lampajr again for this PR, to me:

  • the OWNERS file is redundant (reasons below)
  • wrt to testing, unless comments coming from Manifest WG/leads, I'd assume Follow KF manifests guidelines #69 (comment)
  • wrt to update, at this moment we don't have any update procedure documented, further it's only Alpha so there is no b/c guarantee regardless atm

manifests/kustomize/OWNERS Show resolved Hide resolved
@lampajr
Copy link
Member Author

lampajr commented Apr 28, 2024

thanks @lampajr again for this PR, to me:

yeah I agree, don't know if we want to extract those checks in a separate script such that it can be easily run as sanity check while deploying KF from manifests. In that case we could place that script here.

  • wrt to update, at this moment we don't have any update procedure documented, further it's only Alpha so there is no b/c guarantee regardless atm

+1, I think we can skip this req, at least at the moment.

Anyway, happy to discuss and set how to proceed on this topic in the next KFMR meeting 🚀

@tarilabs
Copy link
Member

as discussed in KF bi-weekly Model Registry meeting

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lampajr, tarilabs

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

@google-oss-prow google-oss-prow bot merged commit 3aa0ece into kubeflow:main Apr 30, 2024
10 checks passed
@lampajr lampajr deleted the kf_manifests_guidelines branch May 1, 2024 08:55
dhirajsb pushed a commit to dhirajsb/model-registry-kfp that referenced this pull request May 22, 2024
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.14.0 to 0.17.0.
- [Commits](golang/net@v0.14.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
tarilabs pushed a commit to tarilabs/model-registry that referenced this pull request Jun 19, 2024
[pull] main from kubeflow:main
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.

None yet

3 participants