RHOAIENG-3458: Introduce edge client and model registry client

devguyio commented 5 months ago

Description

Using the model registry to implement the edge use case includes a lot of custom logic.

This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly.

This PR also refactors the models command to use the newly introduced edge client and streamlines the command design.

[!TIP] This PR is reviewed best commit-by-commit. Instead of using the Files changed tab:

Use the Commits tab

Click on the top commit and review that commit

Use n and p to go to the next and previous commits respectively to review each commit.

How Has This Been Tested?

cd cli
make cli-build
./odh models

./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1"

Merge criteria:

[x] The commits are squashed in a cohesive manner and have meaningful messages.
[x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
[x] The developer has manually tested the changes and verified that the changes work

openshift-ci-robot commented 5 months ago

@devguyio: This pull request references RHOAIENG-3458 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/234): > >## Description > >Using the model registry to implement the edge use case includes a lot of custom logic. > >This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly. > >This PR also refactors the models command to use the newly introduced edge client and streamlines the command design. >>[!TIP] >> This PR is reviewed best commit-by-commit. Instead of using the `Files changed` tab: >> - Use the `Commits` tab >> - Click on the top commit and review that commit >> - Use `n` and `p` to go to the next and previous commits respectively to review each commit. > > >## How Has This Been Tested? >```shell >cd cli >make cli-build >./odh models >``` >![image](https://github.com/opendatahub-io/ai-edge/assets/1930204/8c0199b7-fea1-4516-885e-9dd594de4f84) > >```shell >./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1" >``` >![image](https://github.com/opendatahub-io/ai-edge/assets/1930204/8a0b25c2-020b-4cfb-a6af-53847973729d) > > >## Merge criteria: > > > >- [ ] The commits are squashed in a cohesive manner and have meaningful messages. >- [ ] 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 > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

openshift-ci-robot commented 5 months ago

@devguyio: This pull request references RHOAIENG-3458 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/234): > >## Description > >Using the model registry to implement the edge use case includes a lot of custom logic. > >This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly. > >This PR also refactors the models command to use the newly introduced edge client and streamlines the command design. >>[!TIP] >> This PR is reviewed best commit-by-commit. Instead of using the `Files changed` tab: >> - Use the `Commits` tab >> - Click on the top commit and review that commit >> - Use `n` and `p` to go to the next and previous commits respectively to review each commit. > > >## How Has This Been Tested? >```shell >cd cli >make cli-build >./odh models >``` >![image](https://github.com/opendatahub-io/ai-edge/assets/1930204/8c0199b7-fea1-4516-885e-9dd594de4f84) > >```shell >./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1" >``` >![image](https://github.com/opendatahub-io/ai-edge/assets/1930204/8a0b25c2-020b-4cfb-a6af-53847973729d) > > >## Merge criteria: > > > >- [x] The commits are squashed in a cohesive manner and have meaningful messages. >- [ ] 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 > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

openshift-ci-robot commented 5 months ago

@devguyio: This pull request references RHOAIENG-3458 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/234): > >## Description > >Using the model registry to implement the edge use case includes a lot of custom logic. > >This PR adds a model registry client and an edge client as two abstractions that encapsulate such logic and can be used by the CLI directly. > >This PR also refactors the models command to use the newly introduced edge client and streamlines the command design. >>[!TIP] >> This PR is reviewed best commit-by-commit. Instead of using the `Files changed` tab: >> - Use the `Commits` tab >> - Click on the top commit and review that commit >> - Use `n` and `p` to go to the next and previous commits respectively to review each commit. > > >## How Has This Been Tested? >```shell >cd cli >make cli-build >./odh models >``` >![image](https://github.com/opendatahub-io/ai-edge/assets/1930204/8c0199b7-fea1-4516-885e-9dd594de4f84) > >```shell >./odh models add "voice-recognition-model" "A ML model for voice recognition" "v1" >``` >![image](https://github.com/opendatahub-io/ai-edge/assets/1930204/8a0b25c2-020b-4cfb-a6af-53847973729d) > > >## Merge criteria: > > > >- [x] The commits are squashed in a cohesive manner and have meaningful messages. >- [x] Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious). >- [x] The developer has manually tested the changes and verified that the changes work > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=opendatahub-io%2Fai-edge). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

devguyio commented 5 months ago

@biswassri FYI

Sara4994 commented 5 months ago

Can we have an Readme containing instructions to build and run the cli, also list of available commands and its syntax? it will be easy for others to try it out. wdyt?

grdryn commented 5 months ago

/lint

grdryn commented 5 months ago

This PR is reviewed best commit-by-commit. Instead of using the Files changed tab [...]

One drawback of this approach is that I don't seem to be able to leave inline suggestions as easily on all files; and also I don't seem to be able to mark all files as viewed, although I can for some (I presume if a file is changed in more than one commit in the PR, it can't be marked as viewed in any single one of those).

grdryn commented 4 months ago

/remove-approve

(new process)

devguyio commented 4 months ago

/lint

devguyio commented 4 months ago

/lint

devguyio commented 4 months ago

Can we have an Readme containing instructions to build and run the cli, also list of available commands and its syntax? it will be easy for others to try it out. wdyt?

@Sara4994 I added a readme.

devguyio commented 4 months ago

Is the JIRA ID correct in the title? It appears to reference a closed issue RHOAIENG-3458

@grdryn The JIRA ID is correct, this can be considered a follow-up work relevant to that JIRA, which got closed when #227 was merged. Both PRs are about the models command.

devguyio commented 4 months ago

We should also include this in the Makefile and in the PR check as well under the make test target.

@MarianMacik can you please clarify what should be included in the Makefile?

Sara4994 commented 4 months ago

@devguyio There is a confusion around JIRA number for this PR. I see RHOAIENG-3458 is closed already.

Sara4994 commented 4 months ago

This PR looks good to me, leaving this to Gerard and Landon now :)

devguyio commented 4 months ago

Holding for other reviewers

/hold

devguyio commented 4 months ago

This PR looks good to me, leaving this to Gerard and Landon now :)

And @MarianMacik

LaVLaS commented 4 months ago

/unhold

MarianMacik commented 4 months ago

We should also include this in the Makefile and in the PR check as well under the make test target.

@MarianMacik can you please clarify what should be included in the Makefile?

@devguyio I meant to include cli tests under the root makefile test target.

devguyio commented 4 months ago

We should also include this in the Makefile and in the PR check as well under the make test target.

@MarianMacik can you please clarify what should be included in the Makefile?

@devguyio I meant to include cli tests under the root makefile test target.

@MarianMacik I've pushed the following changes:

Added the cli-test as part of the root test target
Changed the flags packaged by renaming the rootFlag to parentFlag and now it means that this is a flag that will be declared by a parent so we're only interested in getting its value, not in declaring it.
Fixed a bug that was preventing adding two version with two different names under the same model:
- pkg/modelregistry package
  - Added FindRegisteredModel & FindModelVersion
  - Updated AutoRegisterModelVersionArtifact method to check if the model or version exist first, and create them only if they don't exist.

devguyio commented 4 months ago

/override ci/prow/test-ai-edge

openshift-ci[bot] commented 4 months ago

@devguyio: Overrode contexts on behalf of devguyio: ci/prow/test-ai-edge

In response to [this](https://github.com/opendatahub-io/ai-edge/pull/234#issuecomment-2059508532): >/override ci/prow/test-ai-edge Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

openshift-ci[bot] commented 4 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn

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: - ~~[OWNERS](https://github.com/opendatahub-io/ai-edge/blob/main/OWNERS)~~ [grdryn] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

opendatahub-io / ai-edge