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

enable standalone artifacts #376

Merged
merged 7 commits into from
Sep 18, 2024
Merged

Conversation

isinyaaa
Copy link
Contributor

@isinyaaa isinyaaa commented Sep 10, 2024

Fixes: #231

Description

To undo some confusion around opendatahub-io/model-registry-bf4-kf#289 and re-enable standalone artifact support on both the /model_artifacts endpoint and the go core API.

TODO:

  • Test standalone artifacts through REST API (via Python)
  • Test late binding of standalone artifacts to model versions (in Go and via REST)
  • Update REST response status code

How Has This Been Tested?

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages; the author will squash them after approval or in case of manual merges 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.
  • Code changes follow the kubeflow contribution guidelines.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

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.

the PR description mentions:

TODO:

Test standalone artifacts through REST API (via Python)
Test late binding of standalone artifacts to model versions (in Go and via REST)

as not yet fully done? 🤔

Also, before merging this PR, I would suggest checking for non-regression by also making use of the container image in a MR Deployment and check with the UI. (only after we're aligned on lgtm for this PR).

api/openapi/model-registry.yaml Show resolved Hide resolved
pkg/api/api.go Show resolved Hide resolved
@tarilabs
Copy link
Member

thank you for walking through the changes together @isinyaaa ,

well noted the modifications in fdd0209 are needed so that the POST modelversion/../artifact start to behave in upsert fashion

while none of the "base resource" are used in payload for std POST or PATCH, so the modification from that commit is compatible

nor impose any change on the goverter generated code

docs/mr_go_library.md Outdated Show resolved Hide resolved
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.

/lgtm

I'd appreciate as discussed if this modifications can be tested by producing a container image based on this PR, and making sure the basic UI screens still work as intended. I expect they should, given only 1 endpoint is changing semantic to become upsert, but given all the moving pieces, I'd rather be safe than sorry in this sprint. cc @isinyaaa @Al-Pragliola wdyt?

@google-oss-prow google-oss-prow bot added lgtm and removed lgtm labels Sep 16, 2024
@isinyaaa isinyaaa force-pushed the standart branch 2 times, most recently from dace215 to 1081ea9 Compare September 16, 2024 20:31
@isinyaaa
Copy link
Contributor Author

isinyaaa commented Sep 17, 2024

Before:
image

After:
image

Looks fine to me. And seems to be working the same.

isinyaaa and others added 7 commits September 17, 2024 18:35
Fixes: kubeflow#231
Signed-off-by: Isabella do Amaral <idoamara@redhat.com>
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
Co-authored-by: Alessio Pragliola <seth.pro@gmail.com>
Signed-off-by: Isabella do Amaral <idoamara@redhat.com>
Signed-off-by: Isabella do Amaral <idoamara@redhat.com>
This is a workaround to allow upserting models using
a `POST` request to `/model_versions/{id}/artifacts`.

Signed-off-by: Isabella do Amaral <idoamara@redhat.com>
Signed-off-by: Isabella do Amaral <idoamara@redhat.com>
Signed-off-by: Isabella do Amaral <idoamara@redhat.com>
Signed-off-by: Isabella do Amaral <idoamara@redhat.com>
Copy link
Contributor

@Al-Pragliola Al-Pragliola left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

@Al-Pragliola: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@isinyaaa isinyaaa requested a review from tarilabs September 18, 2024 14:40
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.

thank you @isinyaaa for all the changes, the discussions for clarifications, and making a smoke-test with the deployed UI

/lgtm

as previously discussed and

/approve

@google-oss-prow google-oss-prow bot added the lgtm label Sep 18, 2024
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Al-Pragliola, 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 e5422f7 into kubeflow:main Sep 18, 2024
17 checks passed
@isinyaaa isinyaaa deleted the standart branch September 18, 2024 15:35
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.

Impossible to handle standalone artifacts
3 participants